Horton

Thread

Commits

  1. Mark more nodes with attribute no_query_jumble

  2. Include values of A_Const nodes in query jumbling

  3. Generate code for query jumbling through gen_node_support.pl

  4. Rework format of comments in headers for nodes

  5. Move queryjumble.c code to src/backend/nodes/

  6. Fix some compiler warnings in aset.c and generation.c

  7. Reformat some node comments

  1. Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2022-12-07T07:56:41Z

    Hi all,

This thread is a follow-up of the recent discussion about query
jumbling with DDL statements, where the conclusion was that we'd want
to generate all this code automatically for all the nodes:
https://www.postgresql.org/message-id/36e5bffe-e989-194f-85c8-06e7bc88e6f7@amazon.com

What this patch allows to do it to compute the same query ID for
utility statements using their parsed Node state instead of their
string, meaning that things like "BEGIN", "bEGIN" or "begin" would be
treated the same, for example.  But the main idea is not only that.

I have implemented that as of the attached, where the following things
are done:
- queryjumble.c is moved to src/backend/nodes/, to stick with the
other things for node equal/read/write/copy, renamed to
jumblefuncs.c.
- gen_node_support.c is extended to generate the functions and the
switch for the jumbling.  There are a few exceptions, as of the Lists
and RangeTblEntry to do the jumbling consistently.
- Two pg_node_attr() are added in consistency with the existing ones:
no_jumble to discard completely a node from the the query jumbling
and jumble_ignore to discard one field from the jumble.

The patch is in a rather good shape, passes check-world and the CI,
but there are a few things that need to be discussed IMO.  Things
could be perhaps divided in more patches, now the areas touched are
quite different so it did not look like a big deal to me as the
changes touch different areas and are straight-forward.

The location of the Nodes is quite invasive because we only care about
that for T_Const now in the query jumbling, and this could be
compensated with a third pg_node_attr() that tracks for the "int 
location" of a Node whether it should participate in the jumbling or
not.  There is also an argument where we would want to not include by
default new fields added to a Node, but that would require added more
pg_node_attr() than what's done here.

Note that the plan is to extend the normalization to some other parts
of the Nodes, like CALL and SET as mentioned on the other thread.  I
have done nothing about that yet but doing so can be done in a few
lines with the facility presented here (aka just add a location
field).  Hence, the normalization is consistent with the existing
queryjumble.c for the fields and the nodes processed.

In this patch, things are done so as the query ID is not computed
anymore from the query string but from the Query.  I still need to
study the performance impact of that with short queries.  If things
prove to be noticeable in some cases, this stuff could be switched to
use a new GUC where we could have a code path for the computation of
utilityStmt using its string as a fallback.  I am not sure that I want
to enter in this level of complications, though, to keep things
simple, but that's yet to be done.

A bit more could be cut but pg_ident got in the way..  There are also
a few things for pg_stat_statements where a query ID of 0 can be
implied for utility statements in some cases.

Generating this code leads to an overall removal of code as what
 queryjumble.c is generated automatically:
 13 files changed, 901 insertions(+), 1113 deletions(-)

I am adding that to the next commit fest.

Thoughts?
--
Michael

  2. Re: Generating code for query jumbling through gen_node_support.pl

    vignesh C <vignesh21@gmail.com> — 2023-01-06T06:07:32Z

    On Wed, 7 Dec 2022 at 13:27, Michael Paquier <michael@paquier.xyz> wrote:
>
> Hi all,
>
> This thread is a follow-up of the recent discussion about query
> jumbling with DDL statements, where the conclusion was that we'd want
> to generate all this code automatically for all the nodes:
> https://www.postgresql.org/message-id/36e5bffe-e989-194f-85c8-06e7bc88e6f7@amazon.com
>
> What this patch allows to do it to compute the same query ID for
> utility statements using their parsed Node state instead of their
> string, meaning that things like "BEGIN", "bEGIN" or "begin" would be
> treated the same, for example.  But the main idea is not only that.
>
> I have implemented that as of the attached, where the following things
> are done:
> - queryjumble.c is moved to src/backend/nodes/, to stick with the
> other things for node equal/read/write/copy, renamed to
> jumblefuncs.c.
> - gen_node_support.c is extended to generate the functions and the
> switch for the jumbling.  There are a few exceptions, as of the Lists
> and RangeTblEntry to do the jumbling consistently.
> - Two pg_node_attr() are added in consistency with the existing ones:
> no_jumble to discard completely a node from the the query jumbling
> and jumble_ignore to discard one field from the jumble.
>
> The patch is in a rather good shape, passes check-world and the CI,
> but there are a few things that need to be discussed IMO.  Things
> could be perhaps divided in more patches, now the areas touched are
> quite different so it did not look like a big deal to me as the
> changes touch different areas and are straight-forward.
>
> The location of the Nodes is quite invasive because we only care about
> that for T_Const now in the query jumbling, and this could be
> compensated with a third pg_node_attr() that tracks for the "int
> location" of a Node whether it should participate in the jumbling or
> not.  There is also an argument where we would want to not include by
> default new fields added to a Node, but that would require added more
> pg_node_attr() than what's done here.
>
> Note that the plan is to extend the normalization to some other parts
> of the Nodes, like CALL and SET as mentioned on the other thread.  I
> have done nothing about that yet but doing so can be done in a few
> lines with the facility presented here (aka just add a location
> field).  Hence, the normalization is consistent with the existing
> queryjumble.c for the fields and the nodes processed.
>
> In this patch, things are done so as the query ID is not computed
> anymore from the query string but from the Query.  I still need to
> study the performance impact of that with short queries.  If things
> prove to be noticeable in some cases, this stuff could be switched to
> use a new GUC where we could have a code path for the computation of
> utilityStmt using its string as a fallback.  I am not sure that I want
> to enter in this level of complications, though, to keep things
> simple, but that's yet to be done.
>
> A bit more could be cut but pg_ident got in the way..  There are also
> a few things for pg_stat_statements where a query ID of 0 can be
> implied for utility statements in some cases.
>
> Generating this code leads to an overall removal of code as what
>  queryjumble.c is generated automatically:
>  13 files changed, 901 insertions(+), 1113 deletions(-)
>
> I am adding that to the next commit fest.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
b82557ecc2ebbf649142740a1c5ce8d19089f620 ===
=== applying patch
./0001-Support-for-automated-query-jumble-with-all-Nodes.patch
...
patching file src/backend/utils/misc/queryjumble.c
Hunk #1 FAILED at 1.
Not deleting file src/backend/utils/misc/queryjumble.c as content
differs from patch
1 out of 1 hunk FAILED -- saving rejects to file
src/backend/utils/misc/queryjumble.c.rej

[1] - http://cfbot.cputube.org/patch_41_4047.log

Regards,
Vignesh

  3. Re: Generating code for query jumbling through gen_node_support.pl

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-07T06:37:49Z

    On 07.12.22 08:56, Michael Paquier wrote:
> The location of the Nodes is quite invasive because we only care about
> that for T_Const now in the query jumbling, and this could be
> compensated with a third pg_node_attr() that tracks for the "int
> location" of a Node whether it should participate in the jumbling or
> not.

The generation script already has a way to identify location fields, by 
($t eq 'int' && $f =~ 'location$'), so you could use that as well.

> There is also an argument where we would want to not include by
> default new fields added to a Node, but that would require added more
> pg_node_attr() than what's done here.

I'm concerned about the large number of additional field annotations 
this adds.  We have been careful so far to document the use of each 
attribute, e.g., *why* does a field not need to be copied etc.  This 
patch adds dozens and dozens of annotations without any explanation at 
all.  Now, the code this replaces also has no documentation, but maybe 
this is the time to add some.

  4. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-07T10:47:59Z

    On Sat, Jan 07, 2023 at 07:37:49AM +0100, Peter Eisentraut wrote:
> The generation script already has a way to identify location fields, by ($t
> eq 'int' && $f =~ 'location$'), so you could use that as well.

I recall that some of the nodes may need renames to map with this
choice.  That could be just one patch on top of the actual feature.

> I'm concerned about the large number of additional field annotations this
> adds.  We have been careful so far to document the use of each attribute,
> e.g., *why* does a field not need to be copied etc.  This patch adds dozens
> and dozens of annotations without any explanation at all.  Now, the code
> this replaces also has no documentation, but maybe this is the time to add
> some.

In most cases, the addition of the node marker would be enough to
self-explain why they are included, but there is a trend for a lot of
the nodes when it comes to collations and typmods where we don't want
to see these in the jumbling calculations.  I'll look at providing
more info for all that.  (Note: I'm out for now.)
--
Michael

  5. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-13T07:54:58Z

    On Sat, Jan 07, 2023 at 07:37:49AM +0100, Peter Eisentraut wrote:
> On 07.12.22 08:56, Michael Paquier wrote:
>> The location of the Nodes is quite invasive because we only care about
>> that for T_Const now in the query jumbling, and this could be
>> compensated with a third pg_node_attr() that tracks for the "int
>> location" of a Node whether it should participate in the jumbling or
>> not.
> 
> The generation script already has a way to identify location fields, by ($t
> eq 'int' && $f =~ 'location$'), so you could use that as well.

I did not recall exactly everything here, but there are two parts to
the logic:
- gen_node_support.pl uses exactly this condition when scanning the
nodes to put the correct macro to mark a location to track, calling
down RecordConstLocation().
- Marking a bunch of nodes as jumble_ignore is actually necessary, or
we may finish by silencing parts of queries that should be
semantically unrelevant to the queries jumbled (ColumnRef is one).
Using a "jumble_ignore" flag is equally invasive to using an
"jumble_include" flag for each field, I am afraid, as the number of
fields in the nodes included in jumbles is pretty equivalent to the
number of fields ignored.  I tend to prefer the approach of ignoring
things though, which is more consistent with the practive for node
read, write and copy.

Anyway, when it comes to the location, another thing that can be
considered here would be to require a node-level flag for the nodes on
which we want to track the location.  This overlaps a bit with the
fields satisfying "($t eq 'int' && $f =~ 'location$')", but it removes
most of the code changes like this one as at the end we only care
about the location for Const nodes:
-   int         location;       /* token location, or -1 if unknown */
+   int         location pg_node_attr(jumble_ignore);   /* token location, or -1
+                                                        * if unknown */

I have taken this approach in v2 of the patch, shaving ~100 lines of
more code as there is no need to mark all these location fields with
"jumble_ignore" anymore, except for Const, of course.

>> There is also an argument where we would want to not include by
>> default new fields added to a Node, but that would require added more
>> pg_node_attr() than what's done here.
> 
> I'm concerned about the large number of additional field annotations this
> adds.  We have been careful so far to document the use of each attribute,
> e.g., *why* does a field not need to be copied etc.  This patch adds dozens
> and dozens of annotations without any explanation at all.  Now, the code
> this replaces also has no documentation, but maybe this is the time to add
> some.

That's fair, though it is not doing to buy us much to update all the
nodes with similar small comments, as well.  As far as I know, there
are basiscally three things here: typmods, collation information, and
internal data of the nodes stored during parse-analyze.  I have added
more documentation to track what looks like the most relevant areas.

I have begun running some performance tests with this stuff and HEAD
to see if this leads to any difference in the query ID compilation
(compute_query_id = on, on scissors roughly) with a simple set of
short commands (like BEGIN/COMMIT) or longer ones, and I am seeing a
speedup trend actually (?).  I still need to think more about a set of
tests here, but I think that micro-benchmarking of JumbleQuery() is
the most adapted approach to minimize the noise, with a few nodes of
various sizes (Const, Query, ColumnRef, anything..).

Thoughts?
--
Michael

  6. Re: Generating code for query jumbling through gen_node_support.pl

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-16T14:13:35Z

    On 13.01.23 08:54, Michael Paquier wrote:
> Using a "jumble_ignore" flag is equally invasive to using an
> "jumble_include" flag for each field, I am afraid, as the number of
> fields in the nodes included in jumbles is pretty equivalent to the
> number of fields ignored.  I tend to prefer the approach of ignoring
> things though, which is more consistent with the practive for node
> read, write and copy.

I concur that jumble_ignore is better.  I suppose you placed the 
jumble_ignore markers to maintain parity with the existing code, but I 
think that some the markers are actually wrong and are just errors of 
omission in the existing code (such as Query.override, to pick a random 
example).  The way you have structured this would allow us to find and 
analyze such errors better.

> Anyway, when it comes to the location, another thing that can be
> considered here would be to require a node-level flag for the nodes on
> which we want to track the location.  This overlaps a bit with the
> fields satisfying "($t eq 'int' && $f =~ 'location$')", but it removes
> most of the code changes like this one as at the end we only care
> about the location for Const nodes:
> -   int         location;       /* token location, or -1 if unknown */
> +   int         location pg_node_attr(jumble_ignore);   /* token location, or -1
> +                                                        * if unknown */
> 
> I have taken this approach in v2 of the patch, shaving ~100 lines of
> more code as there is no need to mark all these location fields with
> "jumble_ignore" anymore, except for Const, of course.

I don't understand why you chose that when we already have an 
established way.  This would just make the jumble annotations 
inconsistent with the other annotations.

I also have two suggestions to make this patch more palatable:

1. Make a separate patch to reformat long comments, like 
835d476fd21bcfb60b055941dee8c3d9559af14c.

2. Make a separate patch to move the jumble support to 
src/backend/nodes/jumblefuncs.c.  (It was probably a mistake that it 
wasn't there to begin with, and some of the errors of omission alluded 
to above are probably caused by it having been hidden away in the wrong 
place.)

  7. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-17T03:48:32Z

    On Mon, Jan 16, 2023 at 03:13:35PM +0100, Peter Eisentraut wrote:
> On 13.01.23 08:54, Michael Paquier wrote:
>> Using a "jumble_ignore" flag is equally invasive to using an
>> "jumble_include" flag for each field, I am afraid, as the number of
>> fields in the nodes included in jumbles is pretty equivalent to the
>> number of fields ignored.  I tend to prefer the approach of ignoring
>> things though, which is more consistent with the practive for node
>> read, write and copy.
> 
> I concur that jumble_ignore is better.  I suppose you placed the
> jumble_ignore markers to maintain parity with the existing code, but I think
> that some the markers are actually wrong and are just errors of omission in
> the existing code (such as Query.override, to pick a random example).  The
> way you have structured this would allow us to find and analyze such errors
> better.

Thanks.  Yes, I have made an effort of going down to maintain an exact
compatibility with the existing code for now.  My take is that
removing or adding more things into the jumble deserves its own
discussion.  I think that's a bit better once this code is automated
with the nodes, now it would not be difficult either to adjust HEAD
depending on that.  Only the analysis effort is different.

>> Anyway, when it comes to the location, another thing that can be
>> considered here would be to require a node-level flag for the nodes on
>> which we want to track the location.  This overlaps a bit with the
>> fields satisfying "($t eq 'int' && $f =~ 'location$')", but it removes
>> most of the code changes like this one as at the end we only care
>> about the location for Const nodes:
>> -   int         location;       /* token location, or -1 if unknown */
>> +   int         location pg_node_attr(jumble_ignore);   /* token location, or -1
>> +                                                        * if unknown */
>> 
>> I have taken this approach in v2 of the patch, shaving ~100 lines of
>> more code as there is no need to mark all these location fields with
>> "jumble_ignore" anymore, except for Const, of course.
> 
> I don't understand why you chose that when we already have an established
> way.  This would just make the jumble annotations inconsistent with the
> other annotations.

Because we don't want to track the location of all the nodes!  If we
do that, pg_stat_statements would begin to parameterize a lot more
things, from column aliases to full contents of IN or WITH clauses.
The root point is that we only want to track the jumble location for
Const nodes now.  In order to do that, there are two approaches:
- Mark all the locations with jumble_ignore: more invasive, but
it requires only one per-field attribute with "jumble_ignore".  This
is what v1 did.
- Mark only the fields where we want to track the location with a
second node type, like "jumble_location".  We could restrict that
depending on the field type or its name on top of checking the field
attribute, whatever.  This is what v2 did.

Which approach do you prefer?  Marking all the locations we don't want
with jumble_ignore, or introduce a second attribute with
jumble_location.  I'd tend to prefer the approach of minimizing the
number of node and field attributes, FWIW.  Now you have worked on
this area previously, so your view may be more adapted than my
thinking process

The long-term perspective is that I'd like to extend the tracking of
the locations to more DDL nodes, like parameters of SET statements or
parts of CALL statements.  Not to mention that this makes the work of
forks easier.  This is a separate discussion.

> I also have two suggestions to make this patch more palatable:
> 
> 1. Make a separate patch to reformat long comments, like
> 835d476fd21bcfb60b055941dee8c3d9559af14c.
> 
> 2. Make a separate patch to move the jumble support to
> src/backend/nodes/jumblefuncs.c.  (It was probably a mistake that it wasn't
> there to begin with, and some of the errors of omission alluded to above are
> probably caused by it having been hidden away in the wrong place.)

Both suggestions make sense.  I'll shape the next series of the patch
to do something among these lines.

My question about the location tracking greatly influences the first
patch (comment reformating) and third patch (automated generation of
the code) of the series, so do you have a preference about that? 
--
Michael

  8. Re: Generating code for query jumbling through gen_node_support.pl

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-17T07:43:44Z

    On 17.01.23 04:48, Michael Paquier wrote:
>>> Anyway, when it comes to the location, another thing that can be
>>> considered here would be to require a node-level flag for the nodes on
>>> which we want to track the location.  This overlaps a bit with the
>>> fields satisfying "($t eq 'int' && $f =~ 'location$')", but it removes
>>> most of the code changes like this one as at the end we only care
>>> about the location for Const nodes:
>>> -   int         location;       /* token location, or -1 if unknown */
>>> +   int         location pg_node_attr(jumble_ignore);   /* token location, or -1
>>> +                                                        * if unknown */
>>>
>>> I have taken this approach in v2 of the patch, shaving ~100 lines of
>>> more code as there is no need to mark all these location fields with
>>> "jumble_ignore" anymore, except for Const, of course.
>>
>> I don't understand why you chose that when we already have an established
>> way.  This would just make the jumble annotations inconsistent with the
>> other annotations.
> 
> Because we don't want to track the location of all the nodes!  If we
> do that, pg_stat_statements would begin to parameterize a lot more
> things, from column aliases to full contents of IN or WITH clauses.
> The root point is that we only want to track the jumble location for
> Const nodes now.  In order to do that, there are two approaches:
> - Mark all the locations with jumble_ignore: more invasive, but
> it requires only one per-field attribute with "jumble_ignore".  This
> is what v1 did.

Ok, I understand now, and I agree with this approach over the opposite. 
I was confused because the snippet you showed above used 
"jumble_ignore", but your patch is correct as it uses "jumble_location".

That said, the term "jumble" is really weird, because in the sense that 
we are using it here it means, approximately, "to mix together", "to 
unify".  So what we are doing with the Const nodes is really to *not* 
jumble the location, but for all other node types we are jumbling the 
location.  At least that is my understanding.

  9. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-17T07:52:28Z

    On Tue, Jan 17, 2023 at 08:43:44AM +0100, Peter Eisentraut wrote:
> Ok, I understand now, and I agree with this approach over the opposite. I
> was confused because the snippet you showed above used "jumble_ignore", but
> your patch is correct as it uses "jumble_location".

Okay.  I'll refresh the patch set so as we have only "jumble_ignore",
then, like v1, with preparatory patches for what you mentioned and
anything that comes into mind.

> That said, the term "jumble" is really weird, because in the sense that we
> are using it here it means, approximately, "to mix together", "to unify".
> So what we are doing with the Const nodes is really to *not* jumble the
> location, but for all other node types we are jumbling the location.  At
> least that is my understanding.

I am quite familiar with this term, FWIW.  That's what we've inherited
from the days where this has been introduced in pg_stat_statements.
--
Michael

  10. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-18T07:04:23Z

    On Tue, Jan 17, 2023 at 04:52:28PM +0900, Michael Paquier wrote:
> On Tue, Jan 17, 2023 at 08:43:44AM +0100, Peter Eisentraut wrote:
>> Ok, I understand now, and I agree with this approach over the opposite. I
>> was confused because the snippet you showed above used "jumble_ignore", but
>> your patch is correct as it uses "jumble_location".
> 
> Okay.  I'll refresh the patch set so as we have only "jumble_ignore",
> then, like v1, with preparatory patches for what you mentioned and
> anything that comes into mind.

This is done as of the patch series v3 attached:
- 0001 reformats all the comments of the nodes.
- 0002 moves the current files for query jumble as of queryjumble.c ->
queryjumblefuncs.c and utils/queryjumble.h -> nodes/queryjumble.h.
- 0003 is the core feature, where I have done a second pass over the
nodes to make sure that things map with HEAD, incorporating the extra
docs coming from v2, adding a bit more.

>> That said, the term "jumble" is really weird, because in the sense that we
>> are using it here it means, approximately, "to mix together", "to unify".
>> So what we are doing with the Const nodes is really to *not* jumble the
>> location, but for all other node types we are jumbling the location.  At
>> least that is my understanding.
> 
> I am quite familiar with this term, FWIW.  That's what we've inherited
> from the days where this has been introduced in pg_stat_statements.

I have renamed the node attributes to query_jumble_ignore and
no_query_jumble at the end, after considering Peter's point that only
"jumble" could be fuzzy here.  The file names are changed in
consequence.

While doing all that, I have done some micro-benchmarking of
JumbleQuery(), making it loop 50M times on my laptop each time a query
ID is computed (hideous hack with a loop in queryjumble.c):
- For non-utility queries, aka queries that go through
JumbleQueryInternal(), I am measuring a repeatable ~10% improvement
with the generated code over HEAD, which is kind of nice.  I have
tested a few DMLs and simple SELECTs, still it looks like a trend.
- For utility queries, the new code is competing against
hash_any_extended() with the query string, which is going to be hard
to beat..  I am measuring what looks like a 5 times slowdown, at
least, and more depending on the depth of the query tree.  That's not
surprising.  On a 50M loop, this comes down to compare a computation
of 100ns to 5ns for a 20-time slowdown for example, still this could
justify the addition of a GUC to control whether utility queries have
their query ID compiled depending on their nodes or their string
hash, as this could become noticeable in OLTP workloads with loads
of short statements and BEGIN/COMMIT queries?

Thoughts or comments?
--
Michael

  11. Re: Generating code for query jumbling through gen_node_support.pl

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-19T08:42:03Z

    On 18.01.23 08:04, Michael Paquier wrote:
> On Tue, Jan 17, 2023 at 04:52:28PM +0900, Michael Paquier wrote:
>> On Tue, Jan 17, 2023 at 08:43:44AM +0100, Peter Eisentraut wrote:
>>> Ok, I understand now, and I agree with this approach over the opposite. I
>>> was confused because the snippet you showed above used "jumble_ignore", but
>>> your patch is correct as it uses "jumble_location".
>>
>> Okay.  I'll refresh the patch set so as we have only "jumble_ignore",
>> then, like v1, with preparatory patches for what you mentioned and
>> anything that comes into mind.
> 
> This is done as of the patch series v3 attached:
> - 0001 reformats all the comments of the nodes.
> - 0002 moves the current files for query jumble as of queryjumble.c ->
> queryjumblefuncs.c and utils/queryjumble.h -> nodes/queryjumble.h.
> - 0003 is the core feature, where I have done a second pass over the
> nodes to make sure that things map with HEAD, incorporating the extra
> docs coming from v2, adding a bit more.

This patch structure looks good.

>>> That said, the term "jumble" is really weird, because in the sense that we
>>> are using it here it means, approximately, "to mix together", "to unify".
>>> So what we are doing with the Const nodes is really to *not* jumble the
>>> location, but for all other node types we are jumbling the location.  At
>>> least that is my understanding.
>>
>> I am quite familiar with this term, FWIW.  That's what we've inherited
>> from the days where this has been introduced in pg_stat_statements.
> 
> I have renamed the node attributes to query_jumble_ignore and
> no_query_jumble at the end, after considering Peter's point that only
> "jumble" could be fuzzy here.  The file names are changed in
> consequence.

I see that in the 0003 patch, most location fields now have an explicit 
markup with query_jumble_ignore.  I thought we had previously resolved 
to consider location fields to be automatically ignored unless 
explicitly included (like for the Const node).  This appears to invert 
that?  Am I missing something?

  12. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-19T08:46:37Z

    On Thu, Jan 19, 2023 at 09:42:03AM +0100, Peter Eisentraut wrote:
> I see that in the 0003 patch, most location fields now have an explicit
> markup with query_jumble_ignore.  I thought we had previously resolved to
> consider location fields to be automatically ignored unless explicitly
> included (like for the Const node).  This appears to invert that?  Am I
> missing something?

My misunderstanding then, I thought that you were OK with what was
part of v1, where all these fields was marked as "ignore".  But you
actually prefer v2, with the second field "location" on top of
"ignore".  I can update 0003 to refresh that.

Would you be OK if I apply 0001 (with the comments of the locations
still reshaped to ease future property additions) and 0002?
--
Michael

  13. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-20T04:35:15Z

    On Thu, Jan 19, 2023 at 09:42:03AM +0100, Peter Eisentraut wrote:
> I see that in the 0003 patch, most location fields now have an explicit
> markup with query_jumble_ignore.  I thought we had previously resolved to
> consider location fields to be automatically ignored unless explicitly
> included (like for the Const node).  This appears to invert that?  Am I
> missing something?

As a result, I have rebased the patch set to use the two-attribute
approach: query_jumble_ignore and query_jumble_location.

On top of the three previous patches, I am adding 0004 to implement a
GUC able to switch the computation of the utility statements between
what I am calling "string" to compute the query IDs based on the hash
of the query string and the previous default, or "jumble", to use the
parsed tree, with a few more tests to see the difference.  Perhaps it
is not worth bothering, but it could be possible that some users don't
want to pay the penalty of doing the query jumbling with the parsed
tree for utilities, as well..
--
Michael

  14. Re: Generating code for query jumbling through gen_node_support.pl

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-20T10:56:00Z

    On 20.01.23 05:35, Michael Paquier wrote:
> On Thu, Jan 19, 2023 at 09:42:03AM +0100, Peter Eisentraut wrote:
>> I see that in the 0003 patch, most location fields now have an explicit
>> markup with query_jumble_ignore.  I thought we had previously resolved to
>> consider location fields to be automatically ignored unless explicitly
>> included (like for the Const node).  This appears to invert that?  Am I
>> missing something?
> As a result, I have rebased the patch set to use the two-attribute
> approach: query_jumble_ignore and query_jumble_location.

Structurally, this looks okay to me now.

In your 0001 patch, most of the comment reformattings for location 
fields are no longer needed, so you should undo those.

The 0002 patch looks good.

Those two could be committed with those adjustments, I think.

I'll read the 0003 again more carefully.  I haven't studied the new 0004 
yet.

  15. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-21T03:35:52Z

    On Fri, Jan 20, 2023 at 11:56:00AM +0100, Peter Eisentraut wrote:
> In your 0001 patch, most of the comment reformattings for location fields
> are no longer needed, so you should undo those.
> 
> The 0002 patch looks good.

Okay, I have gone through these two again and applied what I had.
0001 has been cleaned up of the extra comment moves for the
locations.  Now, I have kept a few changes for some of the nodes to
have some consistency with the other fields, in the case where most of
the fields at the end of the structures have to be marked with new
node attributes.  This made the style of the header a bit more
elegant, IMV.

> I'll read the 0003 again more carefully.  I haven't studied the new 0004
> yet.

Thanks, again.  Rebased version attached.
--
Michael

  16. Re: Generating code for query jumbling through gen_node_support.pl

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-23T13:27:13Z

    On 21.01.23 04:35, Michael Paquier wrote:
>> I'll read the 0003 again more carefully.  I haven't studied the new 0004
>> yet.
> 
> Thanks, again.  Rebased version attached.

A couple of small fixes are attached.

There is something weird in _jumbleNode().  There are two switch 
(nodeTag(expr)) statements.  Maybe that's intentional, but then it 
should be commented better, because now it looks more like an editing 
mistake.

The handling of T_RangeTblEntry could be improved.  In other contexts we 
have for example a custom_copy attribute, which generates the switch 
entry but not the function.  Something like this could be useful here too.

Otherwise, this looks ok.  I haven't checked whether it maintains the 
exact behavior from before.  What is the test coverage situation for this?

For the 0004 patch, it should be documented why one would want one 
behavior or the other.  That's totally unclear right now.

I think if we are going to accept 0004, then it might be better to 
combine it with 0003.  Otherwise, 0004 is just undoing a lot of the code 
structure changes in JumbleQuery() that 0003 did.

  17. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-24T06:57:56Z

    On Mon, Jan 23, 2023 at 02:27:13PM +0100, Peter Eisentraut wrote:
> A couple of small fixes are attached.

Thanks.

> There is something weird in _jumbleNode().  There are two switch
> (nodeTag(expr)) statements.  Maybe that's intentional, but then it should be
> commented better, because now it looks more like an editing mistake.

This one is intentional, so as it is possible to track correctly the
highest param ID found while browsing the nodes.  IMO it would be
confusing to add that into gen_node_support.pl.  Another thing that
could be done is to switch Param to have a custom implementation, like
RangeTblEntry, though this removes the automation around the creation
of _jumbleParam().  I have clarified the comments around that.

> The handling of T_RangeTblEntry could be improved.  In other contexts we
> have for example a custom_copy attribute, which generates the switch entry
> but not the function.  Something like this could be useful here too.

Hmm.  Okay.  Fine by me.

> Otherwise, this looks ok.  I haven't checked whether it maintains the exact
> behavior from before.  What is the test coverage situation for this?

0003 taken in isolation has some minimal coverage through
pg_stat_statements, though it turns around 15% with compute_query_id =
auto that would enforce the jumbling path only when pg_stat_statements
uses it.  Still, my plan here is to enforce the loading of 
pg_stat_statements with compute_query_id = regress and
utility_query_id = jumble (if needed) in a new buildfarm machine,
because that's the cheapest path.  An extra possibility is to have
pg_regress kicked in a new TAP test with these settings, but that's
costly and we have already two of these :/  Another possibility is to
plug in that into 027_stream_regress or the pg_upgrade test suite with
new settings :/ 

Anyway, the regression tests of pg_stat_statements should be extended
a bit to cover more node types by default (Say COPY with DMLs for the
InsertStmt & co) to look at how these are showing up once normalized
using their parsed query, and we don't do much around that now.
Normalizing more DDLs should use this path, as well.

> For the 0004 patch, it should be documented why one would want one behavior
> or the other.  That's totally unclear right now.

I am not 100% sure whether we should have this part at the end, but as
an exit path in case somebody complains about the extra load with the
automated jumbling compared to the hash of the query strings, that can
be used as a backup.  Anyway, attached is what would be a
clarification.

> I think if we are going to accept 0004, then it might be better to combine
> it with 0003.  Otherwise, 0004 is just undoing a lot of the code structure
> changes in JumbleQuery() that 0003 did.

Makes sense.  That would be my intention if 0004 is the most
acceptable and splitting things makes things a bit easier to review.
--
Michael

  18. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-25T00:08:17Z

    On Tue, Jan 24, 2023 at 03:57:56PM +0900, Michael Paquier wrote:
> Makes sense.  That would be my intention if 0004 is the most
> acceptable and splitting things makes things a bit easier to review.

There was a silly mistake in 0004 where the jumbling code relied on
compute_query_id rather than utility_query_id, so fixed and rebased as
of v7 attached.
--
Michael

  19. Re: Generating code for query jumbling through gen_node_support.pl

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-26T08:37:13Z

    On 24.01.23 07:57, Michael Paquier wrote:
>> For the 0004 patch, it should be documented why one would want one behavior
>> or the other.  That's totally unclear right now.
> I am not 100% sure whether we should have this part at the end, but as
> an exit path in case somebody complains about the extra load with the
> automated jumbling compared to the hash of the query strings, that can
> be used as a backup.  Anyway, attached is what would be a
> clarification.

Ok, the documentation make sense now.  I wonder what the performance 
impact is.  Probably, nobody cares about microoptimizing CREATE TABLE 
statements.  But BEGIN/COMMIT could matter.  However, whatever you do in 
between the BEGIN and COMMIT will already be jumbled, so you're already 
paying the overhead.  Hopefully, jumbling such simple commands will have 
no noticeable overhead.

In other words, we should test this and hopefully get rid of the 
'string' method.

  20. Re: Generating code for query jumbling through gen_node_support.pl

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-26T08:39:05Z

    On 25.01.23 01:08, Michael Paquier wrote:
> On Tue, Jan 24, 2023 at 03:57:56PM +0900, Michael Paquier wrote:
>> Makes sense.  That would be my intention if 0004 is the most
>> acceptable and splitting things makes things a bit easier to review.
> 
> There was a silly mistake in 0004 where the jumbling code relied on
> compute_query_id rather than utility_query_id, so fixed and rebased as
> of v7 attached.

Overall, this looks good to me.

There are a couple of repetitive comments, like "typmod and collation 
information are irrelevant for the query jumbling".  This applies to all 
nodes, so we don't need to repeat it for a number of nodes (and then not 
mention it for other nodes).  Maybe there should be a central place 
somewhere that describes "these kinds of fields should normally be ignored".

  21. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-27T02:59:47Z

    On Thu, Jan 26, 2023 at 09:37:13AM +0100, Peter Eisentraut wrote:
> Ok, the documentation make sense now.  I wonder what the performance impact
> is.  Probably, nobody cares about microoptimizing CREATE TABLE statements.
> But BEGIN/COMMIT could matter.  However, whatever you do in between the
> BEGIN and COMMIT will already be jumbled, so you're already paying the
> overhead.  Hopefully, jumbling such simple commands will have no noticeable
> overhead.
> 
> In other words, we should test this and hopefully get rid of the 'string'
> method.

Yep.  I have mentioned a few numbers upthread, and this deserves
discussion.

FYI, I have done more micro-benchmarking to compare both methods for
utility queries by hijacking JumbleQuery() to run the computation in a
tight loop run N times (could not come up with a better idea to avoid
the repeated palloc/pfree overhead), as the path to stress is
_jumbleNode().  See the attached, that should be able to apply on top
of the latest patch set (named as .txt to not feed it to the CF bot,
and need to recompile to switch the iteration).

Using that, I can compile the following results for various cases (-O2
and compute_query_id=on):
          query          |  mode  | iterations | avg_runtime_ns | avg_jumble_ns 
-------------------------+--------+------------+----------------+---------------
 begin                   | string |   50000000 |        4.53116 |          4.54
 begin                   | jumble |   50000000 |       30.94578 |         30.94
 commit                  | string |   50000000 |        4.76004 |          4.74
 commit                  | jumble |   50000000 |        31.4791 |         31.48
 create table 1 column   | string |   50000000 |        7.22836 |          7.08
 create table 1 column   | jumble |   50000000 |      152.10852 |        151.96
 create table 5 columns  | string |   50000000 |       12.43412 |         12.28
 create table 5 columns  | jumble |   50000000 |      352.88976 |         349.1
 create table 20 columns | string |    5000000 |         49.591 |          48.2
 create table 20 columns | jumble |    5000000 |      2272.4066 |          2271
 drop table 1 column     | string |   50000000 |        6.70538 |          6.56
 drop table 1 column     | jumble |   50000000 |          50.38 |         50.24
 drop table 5 columns    | string |   50000000 |        6.88256 |          6.74
 drop table 5 columns    | jumble |   50000000 |       50.02898 |          49.9
 SET work_mem            | string |   50000000 |        7.28752 |          7.28
 SET work_mem            | jumble |   50000000 |       91.66588 |         91.64
(16 rows)

avg_runtime_ns is (query runtime / iterations) and avg_jumble_ns is
the same with the difference between the start/end logs in the txt
patch attached.  The overhead to run the query does not matter much if
you compare both.  The time it takes to run a jumble is correlated to
the number of nodes to go through for each query, and there is a
larger gap for more nodes to go through.  Well, a simple "begin" or
"commit" query has its computation time increase from 4ns to 30ns in
average which would be unnoticeable.  The gap is larger for larger
nodes, like SET, still we jump from 7ns to 90ns in this case.  DDLs
take the most hit with this method, where a 20-column CREATE TABLE
jumps from 50ns to 2us (note that the iteration is 10 times lower
here).

At the end, that would be unnoticeable for the average user, I guess,
but here are the numbers I get on my laptop :)
--
Michael

  22. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-27T03:07:36Z

    On Thu, Jan 26, 2023 at 09:39:05AM +0100, Peter Eisentraut wrote:
> There are a couple of repetitive comments, like "typmod and collation
> information are irrelevant for the query jumbling".  This applies to all
> nodes, so we don't need to repeat it for a number of nodes (and then not
> mention it for other nodes).  Maybe there should be a central place
> somewhere that describes "these kinds of fields should normally be ignored".

The place that would make the most sense to me to centralize this
knowlegde is nodes.h itself, because that's where the node attributes
are defined?
--
Michael

  23. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-28T03:32:21Z

    On Tue, Jan 24, 2023 at 03:57:56PM +0900, Michael Paquier wrote:
> Still, my plan here is to enforce the loading of 
> pg_stat_statements with compute_query_id = regress and
> utility_query_id = jumble (if needed) in a new buildfarm machine,

Actually, about this specific point, I have been able to set up a
buildfarm machine that uses shared_preload_libraries =
pg_stat_statements and compute_query_id = regress in the base
configuration, which is uncovered yet.  This works as long as one sets
up EXTRA_INSTALL => "contrib/pg_stat_statements" in build_env.
--
Michael

  24. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-30T10:39:30Z

    On Fri, Jan 27, 2023 at 11:59:47AM +0900, Michael Paquier wrote:
> Using that, I can compile the following results for various cases (-O2
> and compute_query_id=on):
>           query          |  mode  | iterations | avg_runtime_ns | avg_jumble_ns 
> -------------------------+--------+------------+----------------+---------------
>  begin                   | string |   50000000 |        4.53116 |          4.54
>  begin                   | jumble |   50000000 |       30.94578 |         30.94
>  commit                  | string |   50000000 |        4.76004 |          4.74
>  commit                  | jumble |   50000000 |        31.4791 |         31.48
>  create table 1 column   | string |   50000000 |        7.22836 |          7.08
>  create table 1 column   | jumble |   50000000 |      152.10852 |        151.96
>  create table 5 columns  | string |   50000000 |       12.43412 |         12.28
>  create table 5 columns  | jumble |   50000000 |      352.88976 |         349.1
>  create table 20 columns | string |    5000000 |         49.591 |          48.2
>  create table 20 columns | jumble |    5000000 |      2272.4066 |          2271
>  drop table 1 column     | string |   50000000 |        6.70538 |          6.56
>  drop table 1 column     | jumble |   50000000 |          50.38 |         50.24
>  drop table 5 columns    | string |   50000000 |        6.88256 |          6.74
>  drop table 5 columns    | jumble |   50000000 |       50.02898 |          49.9
>  SET work_mem            | string |   50000000 |        7.28752 |          7.28
>  SET work_mem            | jumble |   50000000 |       91.66588 |         91.64
> (16 rows)

Just to close the loop here, I have done more measurements to compare
the jumble done for some DMLs and some SELECTs between HEAD and the
patch (forgot to post some last Friday).  Both methods show comparable
results:
        query         |  mode  | iterations | avg_runtime_ns | avg_jumble_ns 
----------------------+--------+------------+----------------+---------------
 insert table 10 cols | master |   50000000 |      377.17878 |        377.04
 insert table 10 cols | jumble |   50000000 |      409.47924 |        409.34
 insert table 20 cols | master |   50000000 |      692.94924 |         692.8
 insert table 20 cols | jumble |   50000000 |       710.0901 |        709.96
 insert table 5 cols  | master |   50000000 |      232.44308 |         232.3
 insert table 5 cols  | jumble |   50000000 |      253.49854 |        253.36
 select 10 cols       | master |   50000000 |      449.13608 |        383.36
 select 10 cols       | jumble |   50000000 |      491.61912 |        323.86
 select 5 cols        | master |   50000000 |        277.477 |        277.46
 select 5 cols        | jumble |   50000000 |      323.88152 |        323.86
(10 rows)

The averages are in ns, so the difference does not bother me much.
There may be some noise mixed in that ;)

(Attached is the tweak I have applied on HEAD to get some numbers.)
--
Michael

  25. Re: Generating code for query jumbling through gen_node_support.pl

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-30T10:46:06Z

    On 27.01.23 04:07, Michael Paquier wrote:
> On Thu, Jan 26, 2023 at 09:39:05AM +0100, Peter Eisentraut wrote:
>> There are a couple of repetitive comments, like "typmod and collation
>> information are irrelevant for the query jumbling".  This applies to all
>> nodes, so we don't need to repeat it for a number of nodes (and then not
>> mention it for other nodes).  Maybe there should be a central place
>> somewhere that describes "these kinds of fields should normally be ignored".
> 
> The place that would make the most sense to me to centralize this
> knowlegde is nodes.h itself, because that's where the node attributes
> are defined?

Either that or src/backend/nodes/README.

  26. Re: Generating code for query jumbling through gen_node_support.pl

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-30T10:48:45Z

    On 27.01.23 03:59, Michael Paquier wrote:
> At the end, that would be unnoticeable for the average user, I guess,
> but here are the numbers I get on my laptop :)

Personally, I think we do not want the two jumble methods in parallel.

Maybe there are other opinions.

I'm going to set this thread as "Ready for Committer".  Either wait a 
bit for more feedback on this topic, or just go ahead with either 
solution.  We can leave it as a semi-open item for reconsideration later.

  27. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-31T04:21:40Z

    On Mon, Jan 30, 2023 at 11:46:06AM +0100, Peter Eisentraut wrote:
> Either that or src/backend/nodes/README.

The README holds nothing about the node attributes currently, so
nodes.h feels most adapted here at the end..

Thanks,
--
Michael

  28. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-01-31T06:40:56Z

    On Mon, Jan 30, 2023 at 11:48:45AM +0100, Peter Eisentraut wrote:
> I'm going to set this thread as "Ready for Committer".  Either wait a bit
> for more feedback on this topic, or just go ahead with either solution.  We
> can leave it as a semi-open item for reconsideration later.

All the measurements I have done for the last couple of days point me
in the direction that there is no need for an extra node based on the
averaged computation times (did a few more today with some long CREATE
FUNCTION, VIEW or event trigger queries, for example).  Agreed to add
the extra option as something to consider at some point during beta,
as long as it is fresh.  I am not convinced that it will be necessary
but let's see how it goes.

While reviewing all the nodes, I have noticed two mistakes for a few
things marked as query_jumble_ignore but they should not:
- orderClause in WindowClause
- aliascolnames in CommonTableExpr
The rest was fine.

varnullingrels has been added very recently, and there was in
queryjumblefuncs.c a comment explaining why it should be ignored.
This has been moved to nodes.h, like the others.

With all that in mind, I have spent my day polishing that and doing a
close lookup, and the patch has been applied.  Thanks a lot!
--
Michael

  29. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-02-05T10:40:57Z

    On Tue, Jan 31, 2023 at 03:40:56PM +0900, Michael Paquier wrote:
> With all that in mind, I have spent my day polishing that and doing a
> close lookup, and the patch has been applied.  Thanks a lot!

While working on a different patch, I have noticed a small issue in
the way the jumbling happens for A_Const, where ValUnion was not
getting jumbled correctly.  This caused a few statements that rely on
this node to compile the same query IDs when using different values.	
The full contents of pg_stat_statements for a regression database
point to: 
- SET.
- COPY with queries.
- CREATE TABLE with partition bounds and default expressions.

This was causing some confusion in pg_stat_statements where some
utility queries would be incorrectly reported, and at this point the
intention is to keep this area compatible with the string-based method
when it comes to the values.  Like read, write and copy nodes, we need
to compile the query ID based on the type of the value, which cannot
be automated.  Attached is a patch to fix this issue with some
regression tests, that I'd like to get fixed before moving on with
more business in pg_stat_statements (aka properly show Const nodes for
utilities with normalized queries).

Comments or objections are welcome, of course.

(FWIW, I'd like to think that there is an argument to normalize the
A_Const nodes for a portion of the DDL queries, by ignoring their
values in the query jumbling and mark a location, which would be
really useful for some workloads, but that's a separate discussion I
am keeping for later.)
--
Michael

  30. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-02-07T00:16:36Z

    On Sun, Feb 05, 2023 at 07:40:57PM +0900, Michael Paquier wrote:
> Comments or objections are welcome, of course.

Done this part.

> (FWIW, I'd like to think that there is an argument to normalize the
> A_Const nodes for a portion of the DDL queries, by ignoring their
> values in the query jumbling and mark a location, which would be
> really useful for some workloads, but that's a separate discussion I
> am keeping for later.)

And I have a patch pretty much OK for this part of the discussion.
Will post soon..
--
Michael

  31. Re: Generating code for query jumbling through gen_node_support.pl

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-07T22:32:07Z

    Michael Paquier <michael@paquier.xyz> writes:
> With all that in mind, I have spent my day polishing that and doing a
> close lookup, and the patch has been applied.  Thanks a lot!

I have just noticed that this patch is generating useless jumbling
code for node types such as Path nodes and other planner infrastructure
nodes.  That no doubt contributes to the miserable code coverage rating
for queryjumblefuncs.*.c, which have enough dead lines to drag down the
overall rating for all of backend/nodes/.  Shouldn't a little more
attention have been paid to excluding entire node classes if they can
never appear in Query?

			regards, tom lane

  32. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-02-08T06:47:51Z

    On Tue, Feb 07, 2023 at 05:32:07PM -0500, Tom Lane wrote:
> I have just noticed that this patch is generating useless jumbling
> code for node types such as Path nodes and other planner infrastructure
> nodes.  That no doubt contributes to the miserable code coverage rating
> for queryjumblefuncs.*.c, which have enough dead lines to drag down the
> overall rating for all of backend/nodes/.  Shouldn't a little more
> attention have been paid to excluding entire node classes if they can
> never appear in Query?

This one was intentional to let extensions play with jumbling of such
nodes, but perhaps you are right that it makes little sense at this
stage.  If there is an ask for it later, though..  Using
shared_preload_libraries = pg_stat_statements and compute_query_id =
regress shows that numbers go up to 60% for funcs.c and 30% for
switch.c.  Removing nodes like as of the attached brings these numbers
respectively up to 94.5% and 93.5% for a check.  With a check-world, I
measure respectively 96.7% and 96.1% because there is more coverage
for extensions, ALTER SYSTEM and database commands, roughly.

This could also be a file-level policy by enforcing no_query_jumble in
gen_node_support.pl by looking at the header name, still I favor
no_query_jumble to keep all the pg_node_attr() in a single area with
the headers.  Note that the attached includes in 0002 the tweak to
enforce the computation with compute_query_id if you want to test it
yourself and check my numbers.  This is useful IMO as we could detect
missing nodes for all queries (utilities or not), still doing this
change may deserve a separate discussion.  Note that I am not seeing
any "unrecognized node type" in any of the logs for any queries.

As a side note, should we be more aggressive with the tests related to
the jumbling code since it is now in core?  For example, XmlExpr or
MinMaxExpr, which are part of Query nodes that can be jumbled even in
older branches, have zero coverage by default as
coverage.postgresql.org reports, because everything goes through
pg_stat_statements and it includes no queries with such nodes.  My
buildfarm member batta makes sure to stress that no nodes are missing
by overloading pg_stat_statements and compute_query_id = regress in 
the configuration, so no nodes are missing from the computation, still
the coverage could be better across the board.  Expanding the tests of
pg_stat_statements is needed in this area for some time, still could
there be a point in switching compute_query_id = regress so as it is a
synonym of "on" without the EXPLAIN output rather than "auto"?  If the
setting is enforced by pg_regress, the coverage of queryjumble.c would
be so much better, at least..
--
Michael

  33. Re: Generating code for query jumbling through gen_node_support.pl

    Andres Freund <andres@anarazel.de> — 2023-02-08T07:01:03Z

    Hi,

On 2023-02-08 15:47:51 +0900, Michael Paquier wrote:
> This one was intentional to let extensions play with jumbling of such
> nodes, but perhaps you are right that it makes little sense at this
> stage.  If there is an ask for it later, though..  Using
> shared_preload_libraries = pg_stat_statements and compute_query_id =
> regress shows that numbers go up to 60% for funcs.c and 30% for
> switch.c.  Removing nodes like as of the attached brings these numbers
> respectively up to 94.5% and 93.5% for a check.  With a check-world, I
> measure respectively 96.7% and 96.1% because there is more coverage
> for extensions, ALTER SYSTEM and database commands, roughly.

Given that we already pay the price of multiple regress runs, and that
jumbling is now really a core feature, perhaps we should enable
pg_stat_statements in pg_upgrade or 027_stream_regress.pl? I'd hope it
wouldn't add a meaningful amount of time?  A tiny bit of verification at the
end should also be ok.

Both pg_upgrade and 027_stream_regress.pl have some advantages. The former
would test pg_upgrade interactions with shared_preload_libraries, the latter
could do some basic checks of pg_stat_statements on a standby.

Greetings,

Andres Freund

  34. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-02-08T07:16:29Z

    On Tue, Feb 07, 2023 at 11:01:03PM -0800, Andres Freund wrote:
> Given that we already pay the price of multiple regress runs, and that
> jumbling is now really a core feature, perhaps we should enable
> pg_stat_statements in pg_upgrade or 027_stream_regress.pl? I'd hope it
> wouldn't add a meaningful amount of time?  A tiny bit of verification at the
> end should also be ok.

Yeah, I have briefly mentioned this part upthread:
https://www.postgresql.org/message-id/Y8+BdCOjxykre5es@paquier.xyz

It would not, I guess, as long as pg_stat_statements.max is set large 
enough in the TAP test.  There are currently 21k~22k entries in the
regression database, much larger than the default of 5000 so this may
become an issue on small-ish machines if left untouched even in a TAP
test.

> Both pg_upgrade and 027_stream_regress.pl have some advantages. The former
> would test pg_upgrade interactions with shared_preload_libraries, the latter
> could do some basic checks of pg_stat_statements on a standby.

Yes, there could be more checks, potentially useful for both cases, so
I may choose both at the end of the day.  Checking the consistency of
the contents of pg_stat_statements across a pg_upgrade run for the
same version may be one thing?  I am not sure if it is that
interesting, TBH, still that's one idea :)
--
Michael

  35. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-02-09T22:49:45Z

    On Wed, Feb 08, 2023 at 03:47:51PM +0900, Michael Paquier wrote:
> This one was intentional to let extensions play with jumbling of such
> nodes, but perhaps you are right that it makes little sense at this
> stage.  If there is an ask for it later, though..  Using
> shared_preload_libraries = pg_stat_statements and compute_query_id =
> regress shows that numbers go up to 60% for funcs.c and 30% for
> switch.c.  Removing nodes like as of the attached brings these numbers
> respectively up to 94.5% and 93.5% for a check.  With a check-world, I
> measure respectively 96.7% and 96.1% because there is more coverage
> for extensions, ALTER SYSTEM and database commands, roughly.

Tom, did you get a chance to look at what is proposed here and expand
the use of query_jumble_ignore in the definitions of the nodes rather
than have an enforced per-file policy in gen_node_support.pl?  The
attached improves the situation by as much as we can, still the
numbers reported by coverage.postgresql.org won't go that up until we
enforce query jumbling testing on the regression database (something
we should have done since this code moved to core, I guess).
--
Michael

  36. Re: Generating code for query jumbling through gen_node_support.pl

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-09T23:12:50Z

    Michael Paquier <michael@paquier.xyz> writes:
> Tom, did you get a chance to look at what is proposed here and expand
> the use of query_jumble_ignore in the definitions of the nodes rather
> than have an enforced per-file policy in gen_node_support.pl?

Sorry, didn't look at it before.

I'm okay with the pathnodes.h changes --- although surely you don't need
changes like this:

-	pg_node_attr(abstract)
+	pg_node_attr(abstract, no_query_jumble)

"abstract" should already imply "no_query_jumble".

I wonder too if you could shorten the changes by making no_query_jumble
an inheritable attribute, and then just applying it to Path and Plan.

The changes in parsenodes.h seem wrong, except for RawStmt.  Those node
types are used in parsed queries, aren't they?

			regards, tom lane

  37. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-02-10T02:28:53Z

    On Thu, Feb 09, 2023 at 06:12:50PM -0500, Tom Lane wrote:
> I'm okay with the pathnodes.h changes --- although surely you don't need
> changes like this:
> 
> -	pg_node_attr(abstract)
> +	pg_node_attr(abstract, no_query_jumble)
> 
> "abstract" should already imply "no_query_jumble".

Okay, understood.  Following this string of thoughts, I am a bit
surprised for two cases, though:
- PartitionPruneStep.
- Plan.
Both are abstract and both are marked with no_equal.  I guess that
applying no_query_jumble to both of them is fine, and that's what you
mean?

> I wonder too if you could shorten the changes by making no_query_jumble
> an inheritable attribute, and then just applying it to Path and Plan.

Ah.  I did not catch what you meant here at first, but I think that I
do now.  Are you referring to the part of gen_node_support.pl where we
propagate properties when a node is a supertype?  This part would be
taken into account when a node is parsed but we find that its first
member is already tracked as a node:
	# Propagate some node attributes from supertypes
	if ($supertype)
	{
		push @no_copy, $in_struct
		  if elem $supertype, @no_copy;
		push @no_equal, $in_struct
		  if elem $supertype, @no_equal;
		push @no_read, $in_struct
		  if elem $supertype, @no_read;
+		push @no_query_jumble, $in_struct
+		  if elem $supertype, @no_query_jumble;
	}

A benefit of doing that would also discard all the Scan and Sort
nodes.  So like the other no_* attributes, it makes sense to force the
inheritance here.

> The changes in parsenodes.h seem wrong, except for RawStmt.  Those node
> types are used in parsed queries, aren't they?

RTEPermissionInfo is a recent addition, as of a61b1f7.  This commit
documents it as a plan node, still it is part of a Query while being
ignored in the query jumbling since its introduction, so I am a bit
confused by this one.

Anyway, none of these need to be included in the query jumbling
currently because they are ignored, but I'd be fine to generate their
code by default as they could become relevant if other nodes begin to
rely on them more heavily, as being part of queries.  Peter E. has
mentioned upthread that a few nodes should include more jumbling while
some other parts should be ignored.  This should be analyzed
separately because ~15 does not seem to be strictly right, either.

Attached is a patch refreshed with all that.  Feel free to ignore 0002
as that's just useful to enforce the tests to go through the jumbling
code.  The attached reaches 95.0% of line coverage after a check-world
in funcs.c.

Thoughts?
--
Michael

  38. Re: Generating code for query jumbling through gen_node_support.pl

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-10T21:40:08Z

    Michael Paquier <michael@paquier.xyz> writes:
> Okay, understood.  Following this string of thoughts, I am a bit
> surprised for two cases, though:
> - PartitionPruneStep.
> - Plan.
> Both are abstract and both are marked with no_equal.  I guess that
> applying no_query_jumble to both of them is fine, and that's what you
> mean?

On second thought, the point of that is to allow the no_equal property
to automatically inherit to child node types, so doing likewise
for no_query_jumble is sensible.

>> The changes in parsenodes.h seem wrong, except for RawStmt.  Those node
>> types are used in parsed queries, aren't they?

> RTEPermissionInfo is a recent addition, as of a61b1f7.  This commit
> documents it as a plan node, still it is part of a Query while being
> ignored in the query jumbling since its introduction, so I am a bit
> confused by this one.

Hmm ... it is part of Query, so that documentation is wrong, and the
fact that it's not reached by query jumbling kind of seems like a bug.
However, it might be that everything in it is derived from something
else that *is* covered by jumbling, in which case that's okay, if
underdocumented.

> ... Peter E. has
> mentioned upthread that a few nodes should include more jumbling while
> some other parts should be ignored.  This should be analyzed
> separately because ~15 does not seem to be strictly right, either.

Yeah.  It'd surprise me not at all if people have overlooked that.

v2 looks good to me as far as it goes.  I agree these other questions
deserve a separate look.

			regards, tom lane

  39. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-02-13T00:11:53Z

    On Fri, Feb 10, 2023 at 04:40:08PM -0500, Tom Lane wrote:
> v2 looks good to me as far as it goes.

Thanks.  I have applied that after an extra lookup.

> I agree these other questions deserve a separate look.

Okay, I may be able to come back to that.  Another point is that we
need to do a better job in forcing the execution of the query jumbling
in one of the TAP tests running pg_regress, outside
pg_stat_statements, to maximize coverage.  Will see to that on a
separate thread.
--
Michael

  40. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-07-10T22:35:43Z

    On Mon, Jan 30, 2023 at 11:48:45AM +0100, Peter Eisentraut wrote:
> On 27.01.23 03:59, Michael Paquier wrote:
>> At the end, that would be unnoticeable for the average user, I guess,
>> but here are the numbers I get on my laptop :)
> 
> Personally, I think we do not want the two jumble methods in parallel.
> 
> Maybe there are other opinions.

(Thanks Jonathan for the poke.)

Now that we are in mid-beta for 16, it would be a good time to
conclude on this open item:
"Reconsider a utility_query_id GUC to control if query jumbling of
utilities can go through the past string-only mode and the new mode?"

In Postgres ~15, utility commands used a hash of the query string to
compute their query ID.  The current query jumbling code uses a Query
instead, like any other queries.  I have registered this open item as
a self-reminder, mostly in case there would be an argument to have a
GUC where users could switch from one mode to another.  See here as
well for some computation times for each method (table is in ns, wiht
millions of iterations):
https://www.postgresql.org/message-id/Y9eeYinDb1AcpWrG@paquier.xyz

I still don't think that we need both methods based on these numbers,
but there may be more opinions about that?  Are people OK if this open
item is discarded?
--
Michael

  41. Re: Generating code for query jumbling through gen_node_support.pl

    Andrei Lepikhov <a.lepikhov@postgrespro.ru> — 2023-07-11T05:29:29Z

    On 11/7/2023 05:35, Michael Paquier wrote:
> On Mon, Jan 30, 2023 at 11:48:45AM +0100, Peter Eisentraut wrote:
>> On 27.01.23 03:59, Michael Paquier wrote:
>>> At the end, that would be unnoticeable for the average user, I guess,
>>> but here are the numbers I get on my laptop :)
>>
>> Personally, I think we do not want the two jumble methods in parallel.
>>
>> Maybe there are other opinions.
> 
> (Thanks Jonathan for the poke.)
> 
> Now that we are in mid-beta for 16, it would be a good time to
> conclude on this open item:
> "Reconsider a utility_query_id GUC to control if query jumbling of
> utilities can go through the past string-only mode and the new mode?"
> 
> In Postgres ~15, utility commands used a hash of the query string to
> compute their query ID.  The current query jumbling code uses a Query
> instead, like any other queries.  I have registered this open item as
> a self-reminder, mostly in case there would be an argument to have a
> GUC where users could switch from one mode to another.  See here as
> well for some computation times for each method (table is in ns, wiht
> millions of iterations):
> https://www.postgresql.org/message-id/Y9eeYinDb1AcpWrG@paquier.xyz
> 
> I still don't think that we need both methods based on these numbers,
> but there may be more opinions about that?  Are people OK if this open
> item is discarded?
I vote for only one method based on a query tree structure.
BTW, did you think about different algorithms of queryId generation? 
Auto-generated queryId code can open a way for extensions to have 
easy-supporting custom queryIds.

-- 
regards,
Andrey Lepikhov
Postgres Professional

  42. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-07-11T05:35:55Z

    On Tue, Jul 11, 2023 at 12:29:29PM +0700, Andrey Lepikhov wrote:
> I vote for only one method based on a query tree structure.

Noted

> BTW, did you think about different algorithms of queryId generation?

Not really, except if you are referring to the possibility of being
able to handle differently different portions of the nodes depending
on a context given by the callers willing to do a query jumbling
computation.  (For example, choose to *not* silence the Const nodes,
etc.)

> Auto-generated queryId code can open a way for extensions to have
> easy-supporting custom queryIds.

Extensions can control that at some extent, already.
--
Michael

  43. Re: Generating code for query jumbling through gen_node_support.pl

    Andrei Lepikhov <a.lepikhov@postgrespro.ru> — 2023-07-11T06:44:39Z

    On 11/7/2023 12:35, Michael Paquier wrote:
> On Tue, Jul 11, 2023 at 12:29:29PM +0700, Andrey Lepikhov wrote:
>> I vote for only one method based on a query tree structure.
> 
> Noted
> 
>> BTW, did you think about different algorithms of queryId generation?
> 
> Not really, except if you are referring to the possibility of being
> able to handle differently different portions of the nodes depending
> on a context given by the callers willing to do a query jumbling
> computation.  (For example, choose to *not* silence the Const nodes,
> etc.)
Yes, I have two requests on different queryId algorithms:
1. With suppressed Const nodes.
2. With replacement of Oids with full names - to give a chance to see 
the same queryId at different instances for the same query.

It is quite trivial to implement, but not easy in support.
> 
>> Auto-generated queryId code can open a way for extensions to have
>> easy-supporting custom queryIds.
> 
> Extensions can control that at some extent, already.
> --
> Michael

-- 
regards,
Andrey Lepikhov
Postgres Professional

  44. Re: Generating code for query jumbling through gen_node_support.pl

    Michael Paquier <michael@paquier.xyz> — 2023-07-19T03:30:57Z

    On Tue, Jul 11, 2023 at 07:35:43AM +0900, Michael Paquier wrote:
> I still don't think that we need both methods based on these numbers,
> but there may be more opinions about that?  Are people OK if this open
> item is discarded?

Hearing nothing about this point, removed from the open item list,
then.
--
Michael