public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Geier <[email protected]>
To: Dmitry Dolgov <[email protected]>
To: Sergei Kornilov <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Marcos Pegoraro <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Zhihong Yu <[email protected]>
Cc: David Steele <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Cc: Greg Stark <[email protected]>
Cc: Pavel Trukhanov <[email protected]>
Cc: Tom Lane <[email protected]>
Subject: Re: pg_stat_statements and "IN" conditions
Date: Sat, 11 Feb 2023 11:03:36 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
Hi,
On 2/9/23 16:02, Dmitry Dolgov wrote:
>> Unfortunately, rebase is needed again due to recent changes in queryjumblefuncs ( 9ba37b2cb6a174b37fc51d0649ef73e56eae27fc )
I reviewed the last patch applied to some commit from Feb. 4th.
>> It seems a little strange to me that with const_merge_threshold = 1, such a test case gives the same result as with const_merge_threshold = 2
>>
>> select pg_stat_statements_reset();
>> set const_merge_threshold to 1;
>> select * from test where i in (1,2,3);
>> select * from test where i in (1,2);
>> select * from test where i in (1);
>> select query, calls from pg_stat_statements order by query;
>>
>> query | calls
>> -------------------------------------+-------
>> select * from test where i in (...) | 2
>> select * from test where i in ($1) | 1
>>
>> Probably const_merge_threshold = 1 should produce only "i in (...)"?
> Well, it's not intentional, probably I need to be more careful with
> off-by-one. Although I agree to a certain extent with Peter questioning
Please add tests for all the corner cases. At least for (1) IN only
contains a single element and (2) const_merge_threshold = 1.
Beyond that:
- There's a comment about find_const_walker(). I cannot find that
function anywhere. What am I missing?
- What about renaming IsConstList() to IsMergableConstList().
- Don't you intend to use the NUMERIC data column in SELECT * FROM
test_merge_numeric WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10)?
Otherwise, the test is identical to previous test cases and you're not
checking for what happens with NUMERICs which are wrapped in FuncExpr
because of the implicit coercion.
- Don't we want to extend IsConstList() to allow a list of all
implicitly coerced constants? It's inconsistent that otherwise e.g.
NUMERICs don't work.
- Typo in /* The firsts merged constant */ (first not firsts)
- Prepared statements are not supported as they contain INs with Param
instead of Const nodes. While less likely, I've seen applications that
use prepared statements in conjunction with queries generated through a
UI which ended up with tons of prepared queries with different number of
elements in the IN clause. Not necessarily something that must go into
this patch but maybe worth thinking about.
- The setting name const_merge_threshold is not very telling without
knowing the context. While being a little longer, what about
jumble_const_merge_threshold or queryid_const_merge_threshold or similar?
- Why do we actually only want to merge constants? Why don't we ignore
the type of element in the IN and merge whatever is there? Is this
because the original jumbling logic as of now only has support for
constants?
- Ideally we would even remove duplicates. That would even improve
cardinality estimation but I guess we don't want to spend the cycles on
doing so in the planner?
- Why did you change palloc() to palloc0() for clocations array? The
length is initialized to 0 and FWICS RecordConstLocation() initializes
all members. Seems to me like we don't have to spend these cycles.
- Can't the logic at the end of IsConstList() not be simplified to:
foreach(temp, elements)
if (!IsA(lfirst(temp), Const))
return false;
// All elements are of type Const
*firstConst = linitial_node(Const, elements);
*lastConst = llast_node(Const, elements);
return true;
--
David Geier
(ServiceNow)
view thread (12+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: pg_stat_statements and "IN" conditions
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox