Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix typo in comment

  2. Make query jumbling also squash PARAM_EXTERN params

  3. Fix squashing algorithm for query texts

  4. pg_stat_statements: Fix parameter number gaps in normalized queries

  1. queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-04-30T21:52:06Z

    62d712ec added the ability to squash constants from an IN LIST
for queryId computation purposes. This means that a similar
queryId will be generated for the same queries that only
different on the number of values in the IN-LIST.

The patch lacks the ability to apply this optimization to values
passed in as parameters ( i.e. parameter kind = PARAM_EXTERN )
which will be the case for SQL prepared statements and protocol level
prepared statements, i.e.

```
select from t where id in (1, 2, 3) \bind
```
or
```
prepare prp(int, int, int) as select from t where id in ($1, $2, $3);
```

Here is the current state,

```
postgres=# create table t (id int);
CREATE TABLE
postgres=# prepare prp(int, int, int) as select from t where id in ($1, $2, $3);
PREPARE
postgres=# execute prp(1, 2, 3);
postgres=# select from t where id in (1, 2, 3);
--
(0 rows)
postgres=# SELECT query, calls FROM pg_stat_statements ORDER BY query
COLLATE "C";
                                                   query
                                    | calls
-----------------------------------------------------------------------------------------------------------+-------
...
....
 select from t where id in ($1 /*, ... */)
                                    |     1
 select from t where id in ($1, $2, $3)
                                 |     1 <<- prepared statement
(6 rows)
```

but with the attached patch, the optimization applies.

```
 create table t (id int)
                                            |     1
 select from t where id in ($1 /*, ... */)
                                     |     2
(3 rows)
```

I think this is a pretty big gap as many of the common drivers such as JDBC,
which use extended query protocol, will not be able to take advantage of
the optimization in 18, which will be very disappointing.

Thoughts?

Sami Imseih
Amazon Web Services (AWS)

  2. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-01T00:29:13Z

    On Wed, Apr 30, 2025 at 04:52:06PM -0500, Sami Imseih wrote:
> 62d712ec added the ability to squash constants from an IN LIST
> for queryId computation purposes. This means that a similar
> queryId will be generated for the same queries that only
> different on the number of values in the IN-LIST.
> 
> The patch lacks the ability to apply this optimization to values
> passed in as parameters ( i.e. parameter kind = PARAM_EXTERN )
> which will be the case for SQL prepared statements and protocol level
> prepared statements, i.e.
> 
> I think this is a pretty big gap as many of the common drivers such as JDBC,
> which use extended query protocol, will not be able to take advantage of
> the optimization in 18, which will be very disappointing.
> 
> Thoughts?

Yes.  Long IN/ANY clauses are as far as a more common pattern caused
by ORMs, and I'd like to think that application developers would not
hardcode such clauses in their right minds (well, okay, I'm likely
wrong about this assumption, feel free to counter-argue).  These also 
like relying on the extended query protocol.  Not taking into account
JDBC in that is a bummer, and it is very popular.

I agree that the current solution we have in the tree feels incomplete
because we are not taking into account the most common cases that
users would care about.  Now, allowing PARAM_EXTERN means that we
allow any expression to be detected as a squashable thing, and this
kinds of breaks the assumption of IsSquashableConst() where we want
only constants to be allowed because EXECUTE parameters can be any
kind of Expr nodes.  At least that's the intention of the code on
HEAD.

Now, I am not actually objecting about PARAM_EXTERN included or not if
there's a consensus behind it and my arguments are considered as not
relevant.  The patch is written so as it claims that a PARAM_EXTERN
implies the expression to be a Const, but it may not be so depending
on what the execution path is given for the parameter.  Or at least
the patch could be clearer and rename the parts about the "Const"
squashable APIs around queryjumblefuncs.c.

To be honest, the situation of HEAD makes me question whether we are
using the right approach for this facility.  I did mention a couple of
months ago about an alternative, but it comes down to accept that any
expressions would be normalized, unfortunately I never got down to
study it in details as this touches around expr_list in the parser: we
could detect in the parser the start and end locations of an
expression list in a query string, then group all of them together
based on their location in the string.  This would be also cheaper
than going through all the elements in the list, tweaking things when
dealing with a subquery..

The PARAM_EXTERN part has been mentioned a couple of weeks ago here,
btw:
https://www.postgresql.org/message-id/CAA5RZ0tu6_KRiYJCFptf4_--wjFSu9cZMj1XNmOCqTNxu=VpEA@mail.gmail.com
--
Michael

  3. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-01T19:55:47Z

    > On Thu, May 01, 2025 at 09:29:13AM GMT, Michael Paquier wrote:
>
> I agree that the current solution we have in the tree feels incomplete
> because we are not taking into account the most common cases that
> users would care about.  Now, allowing PARAM_EXTERN means that we
> allow any expression to be detected as a squashable thing, and this
> kinds of breaks the assumption of IsSquashableConst() where we want
> only constants to be allowed because EXECUTE parameters can be any
> kind of Expr nodes.  At least that's the intention of the code on
> HEAD.
>
> Now, I am not actually objecting about PARAM_EXTERN included or not if
> there's a consensus behind it and my arguments are considered as not
> relevant.  The patch is written so as it claims that a PARAM_EXTERN
> implies the expression to be a Const, but it may not be so depending
> on what the execution path is given for the parameter.  Or at least
> the patch could be clearer and rename the parts about the "Const"
> squashable APIs around queryjumblefuncs.c.
>
> [...]
>
> The PARAM_EXTERN part has been mentioned a couple of weeks ago here,
> btw:
> https://www.postgresql.org/message-id/CAA5RZ0tu6_KRiYJCFptf4_--wjFSu9cZMj1XNmOCqTNxu=VpEA@mail.gmail.com

In fact, this has been discussed much earlier in the thread above, as
essentially the same implementation with T_Params, which is submitted
here, was part of the original patch. The concern was always whether or
not it will break any assumption about query identification, because
this way much broader scope of expressions will be considered equivalent
for query id computation purposes.

At the same time after thinking about this concern more, I presume it
already happens at a smaller scale -- when two queries happen to have
the same number of parameters, they will be indistinguishable even if
parameters are different in some way.

> To be honest, the situation of HEAD makes me question whether we are
> using the right approach for this facility.  I did mention a couple of
> months ago about an alternative, but it comes down to accept that any
> expressions would be normalized, unfortunately I never got down to
> study it in details as this touches around expr_list in the parser: we
> could detect in the parser the start and end locations of an
> expression list in a query string, then group all of them together
> based on their location in the string.  This would be also cheaper
> than going through all the elements in the list, tweaking things when
> dealing with a subquery..

Not entirely sure how that would work exactly, but after my experiments
with the squashing patch I found it could be very useful to be able to
identify the end location of an expression list in the parser.

  4. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-01T20:57:16Z

    I spent a few hours looking into this today and to your points below:

> > I agree that the current solution we have in the tree feels incomplete
> > because we are not taking into account the most common cases that
> > users would care about.  Now, allowing PARAM_EXTERN means that we
> > allow any expression to be detected as a squashable thing, and this
> > kinds of breaks the assumption of IsSquashableConst() where we want
> > only constants to be allowed because EXECUTE parameters can be any
> > kind of Expr nodes.  At least that's the intention of the code on
> > HEAD.
> >
> > Now, I am not actually objecting about PARAM_EXTERN included or not if
> > there's a consensus behind it and my arguments are considered as not
> > relevant.  The patch is written so as it claims that a PARAM_EXTERN
> > implies the expression to be a Const, but it may not be so depending
> > on what the execution path is given for the parameter.  Or at least
> > the patch could be clearer and rename the parts about the "Const"
> > squashable APIs around queryjumblefuncs.c.
> >
> > [...]
> >
> > The PARAM_EXTERN part has been mentioned a couple of weeks ago here,
> > btw:
> > https://www.postgresql.org/message-id/CAA5RZ0tu6_KRiYJCFptf4_--wjFSu9cZMj1XNmOCqTNxu=VpEA@mail.gmail.com
>
> In fact, this has been discussed much earlier in the thread above, as
> essentially the same implementation with T_Params, which is submitted
> here, was part of the original patch. The concern was always whether or
> not it will break any assumption about query identification, because
> this way much broader scope of expressions will be considered equivalent
> for query id computation purposes.
>
> At the same time after thinking about this concern more, I presume it
> already happens at a smaller scale -- when two queries happen to have
> the same number of parameters, they will be indistinguishable even if
> parameters are different in some way.

I don't think limiting this feature to Const only will suffice.

I think what we should really allow the broader scope of expressions that
are allowed via prepared statements, and this will make this implementation
consistent between prepared vs non-prepared statements. I don't see why
not. In fact, when we are examining the ArrayExpr, I think the only
thing we should
not squash is if we find a Sublink ( i.e. SELECT statement inside the array ).

> > To be honest, the situation of HEAD makes me question whether we are
> > using the right approach for this facility.  I did mention a couple of
> > months ago about an alternative, but it comes down to accept that any
> > expressions would be normalized, unfortunately I never got down to
> > study it in details as this touches around expr_list in the parser: we
> > could detect in the parser the start and end locations of an
> > expression list in a query string, then group all of them together
> > based on their location in the string.  This would be also cheaper
> > than going through all the elements in the list, tweaking things when
> > dealing with a subquery..
>
> Not entirely sure how that would work exactly, but after my experiments
> with the squashing patch I found it could be very useful to be able to
> identify the end location of an expression list in the parser.

I also came to the same conclusion, that we should track the start '('
and end ')'
location of a expression list to allow us to hide the fields. But, I
will look into
other approaches as well.

I am really leaning towards that we should revert this feature as the
limitation we have now with parameters is a rather large one and I think
we need to go back and address this issue.

-- 

Sami

  5. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-01T22:10:19Z

    On Thu, May 01, 2025 at 03:57:16PM -0500, Sami Imseih wrote:
> I think what we should really allow the broader scope of expressions that
> are allowed via prepared statements, and this will make this implementation
> consistent between prepared vs non-prepared statements. I don't see why
> not. In fact, when we are examining the ArrayExpr, I think the only
> thing we should
> not squash is if we find a Sublink ( i.e. SELECT statement inside the array ).

Likely so.  I don't have anything else than Sublink in mind that would
be worth a special case..

> I am really leaning towards that we should revert this feature as the
> limitation we have now with parameters is a rather large one and I think
> we need to go back and address this issue.

I am wondering if this would not be the best move to do on HEAD.
Let's see where the discussion drives us.
--
Michael

  6. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-02T07:13:39Z

    > On Fri, May 02, 2025 at 07:10:19AM GMT, Michael Paquier wrote:
> > I am really leaning towards that we should revert this feature as the
> > limitation we have now with parameters is a rather large one and I think
> > we need to go back and address this issue.
>
> I am wondering if this would not be the best move to do on HEAD.
> Let's see where the discussion drives us.

Squashing constants was ment to be a first step towards doing the same
for other types of queries (params, rte_values), reverting it to
implement everything at once makes very little sense to me.

  7. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-02T07:18:37Z

    On Fri, May 02, 2025 at 09:13:39AM +0200, Dmitry Dolgov wrote:
> Squashing constants was ment to be a first step towards doing the same
> for other types of queries (params, rte_values), reverting it to
> implement everything at once makes very little sense to me.

That depends.  If we conclude that tracking this information through
the parser based on the start and end positions in a query string
for a set of values is more relevant, then we would be redesigning the
facility from the ground, so the old approach would not be really
relevant..
--
Michael

  8. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-02T07:24:15Z

    > On Fri, May 02, 2025 at 04:18:37PM GMT, Michael Paquier wrote:
> On Fri, May 02, 2025 at 09:13:39AM +0200, Dmitry Dolgov wrote:
> > Squashing constants was ment to be a first step towards doing the same
> > for other types of queries (params, rte_values), reverting it to
> > implement everything at once makes very little sense to me.
>
> That depends.  If we conclude that tracking this information through
> the parser based on the start and end positions in a query string
> for a set of values is more relevant, then we would be redesigning the
> facility from the ground, so the old approach would not be really
> relevant..

If I understand you correctly, changing the way how element list is
identified is not going to address the question whether or not to squash
parameters, right?

  9. Re: queryId constant squashing does not support prepared statements

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-05-02T12:56:56Z

    On 2025-May-02, Michael Paquier wrote:

> That depends.  If we conclude that tracking this information through
> the parser based on the start and end positions in a query string
> for a set of values is more relevant, then we would be redesigning the
> facility from the ground, so the old approach would not be really
> relevant..

I disagree that a revert is warranted for this reason.  If you want to
change the implementation later, that's fine, as long as the user
interface doesn't change.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Doing what he did amounts to sticking his fingers under the hood of the
implementation; if he gets his fingers burnt, it's his problem."  (Tom Lane)

  10. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-04T10:19:27Z

    > On Thu, May 01, 2025 at 09:55:47PM GMT, Dmitry Dolgov wrote:
> > On Thu, May 01, 2025 at 09:29:13AM GMT, Michael Paquier wrote:
> >
> > I agree that the current solution we have in the tree feels incomplete
> > because we are not taking into account the most common cases that
> > users would care about.  Now, allowing PARAM_EXTERN means that we
> > allow any expression to be detected as a squashable thing, and this
> > kinds of breaks the assumption of IsSquashableConst() where we want
> > only constants to be allowed because EXECUTE parameters can be any
> > kind of Expr nodes.  At least that's the intention of the code on
> > HEAD.
> >
> > Now, I am not actually objecting about PARAM_EXTERN included or not if
> > there's a consensus behind it and my arguments are considered as not
> > relevant.  The patch is written so as it claims that a PARAM_EXTERN
> > implies the expression to be a Const, but it may not be so depending
> > on what the execution path is given for the parameter.  Or at least
> > the patch could be clearer and rename the parts about the "Const"
> > squashable APIs around queryjumblefuncs.c.
> >
> > [...]
> >
> > The PARAM_EXTERN part has been mentioned a couple of weeks ago here,
> > btw:
> > https://www.postgresql.org/message-id/CAA5RZ0tu6_KRiYJCFptf4_--wjFSu9cZMj1XNmOCqTNxu=VpEA@mail.gmail.com
>
> In fact, this has been discussed much earlier in the thread above, as
> essentially the same implementation with T_Params, which is submitted
> here, was part of the original patch. The concern was always whether or
> not it will break any assumption about query identification, because
> this way much broader scope of expressions will be considered equivalent
> for query id computation purposes.
>
> At the same time after thinking about this concern more, I presume it
> already happens at a smaller scale -- when two queries happen to have
> the same number of parameters, they will be indistinguishable even if
> parameters are different in some way.

Returning to the topic of whether to squash list of Params.

Originally squashing of Params wasn't included into the squashing patch due to
concerns from reviewers about treating quite different queries as the same for
the purposes of query identification. E.g. there is some assumption somewhere,
which will be broken if we treat query with a list of integer parameters same
as a query with a list of float parameters. For the sake of making progress
I've decided to postpone answering this question and concentrate on more simple
scenario. Now, as the patch was applied, I think it's a good moment to reflect
on those concerns. It's not enough to say that we don't see any problems with
squashing of Param, some more sound argumentation is needed. So, what will
happen if parameters are squashed as constants?

1. One obvious impact is that more queries, that were considered distinct
before, will have the same normalized query and hence the entry in
pg_stat_statements. Since a Param could be pretty much anything, this can lead
to a situation when two queries with quiet different performance profiles (e.g.
one contains a parameter value, which is a heavy function, another one doesn't)
are matched to one entry, making it less useful.

But at the same time this already can happen if those two queries have the same
number of parameters, since query parametrizing is intrinsically lossy in this
sense. The only thing we do by squashing such queries is we loose information
about the number of parameters, not properties of the parameters themselves.

2. Another tricky scenario is when queryId is used by some extension, which in
turn makes assumption about it that are going to be rendered incorrect by
squashing. The only type of assumptions I can imagine falling into this
category is anything about equivalence of queries. For example, an extension
can capture two queries, which have the same normalized entry in pgss, and
assume all properties of those queries are the same.

It's worth noting that normalized query is not transitive, i.e. if a query1 has
the normalized version query_norm, and a query2 has the same normalized version
query_norm, it doesn't mean query1 is equivalent query2 in all senses (e.g.
they could have list of parameter values with different type and the same
size). That means that such assumptions are already faulty, and could work most
of the time only because it takes queries with a list of the same size to break
the assumption. Squashing such queries will make them wrong more often.

One can argue that we might want to be friendly to such extensions, and do not
"break" them even further. But I don't think it's worth it, as number of such
extensions is most likely low, if any. One more extreme case would be when an
extension assumes that queries with the same entry in pgss have the same number
of parameters, but I don't see how such assumption could be useful.

3. More annoying is the consequence that parameters are going to be treated as
constants in pg_stat_statements. While mostly harmless, that would mean they're
going to be replaced in the same way as constants. This means that the
parameter order is going to be lost, e.g.:

SELECT * FROM test_squash WHERE data IN ($4, $3, $2, $1) \bind 1 2 3 4
-- output
SELECT * FROM test_squash WHERE data IN ($1 /*, ... */)

SELECT * FROM test_squash WHERE data IN ($1, $2, $3, $4)
    AND id IN ($5, $6, $7, $8) \bind 1 2 3 4 5 6 7 8
-- output
SELECT * FROM test_squash WHERE data IN ($1 /*, ... */)
    AND id IN ($2 /*, ... */)

This representation could be confusing of course. It could be either explained
in the documentation, or LocationLen has to be extended to carry information
about whether it's a constant or a parameter, and do not replace the latter. In
any case, anything more than the first parameter number will be lost, but it's
probably not so dramatic.

At the end of the day, I think the value of squashing for parameters outweighs
the problems described above. As long as there is an agreement about that, it's
fine by me. I've attached the more complete version of the patch (but without
modifying LocationLen to not replace Param yet) in case if such agreemeng will
be achieved.

  11. Re: queryId constant squashing does not support prepared statements

    Jeremy Schneider <schneider@ardentperf.com> — 2025-05-06T02:44:03Z

    On Fri, 2 May 2025 14:56:56 +0200
Álvaro Herrera <alvherre@kurilemu.de> wrote:

> On 2025-May-02, Michael Paquier wrote:
> 
> > That depends.  If we conclude that tracking this information through
> > the parser based on the start and end positions in a query string
> > for a set of values is more relevant, then we would be redesigning
> > the facility from the ground, so the old approach would not be
> > really relevant..  
> 
> I disagree that a revert is warranted for this reason.  If you want to
> change the implementation later, that's fine, as long as the user
> interface doesn't change.
> 

FWIW, i'm +1 on leaving it in pg18. Prepared statements often look a
little different in other ways, and there are a bunch of other quirks
in how queryid's are calculated too. Didn't there used to be something
with CALL being handled as a utility statement making stored procs look
different from functions?



-- 
To know the thoughts and deeds that have marked man's progress is to
feel the great heart throbs of humanity through the centuries; and if
one does not feel in these pulsations a heavenward striving, one must
indeed be deaf to the harmonies of life.

Helen Keller. Let Us Have Faith. Doubleday, Doran & Company, 1940.

  12. Re: queryId constant squashing does not support prepared statements

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-06T03:09:43Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Thu, May 01, 2025 at 03:57:16PM -0500, Sami Imseih wrote:
>> I think what we should really allow the broader scope of expressions that
>> are allowed via prepared statements, and this will make this implementation
>> consistent between prepared vs non-prepared statements. I don't see why
>> not. In fact, when we are examining the ArrayExpr, I think the only
>> thing we should
>> not squash is if we find a Sublink ( i.e. SELECT statement inside the array ).

> Likely so.  I don't have anything else than Sublink in mind that would
> be worth a special case..

I think this is completely wrong.  As simple examples, there is
nothing even a little bit comparable between the behaviors of

	t1.x IN (1, 2, 3)

	t1.x IN (1, 2, t2.y)

	t1.x IN (1, 2, random())

Squashing these to look the same would be doing nobody any favors.

I do agree that treating PARAM_EXTERN Params the same as constants
for this purpose is a reasonable thing to do, on three arguments:

1. A PARAM_EXTERN Param actually behaves largely the same as a Const
so far as a query is concerned: it does not change value across
the execution of the query.  (This is not true of other kinds of
Params.)

2. It's very much dependent on the client-side stack whether a given
value that is constant in the mind of the application will be passed
to the backend as a Const or a Param.  (This is okay because #1.)

3. Even if the value is passed as a Param, the planner might replace
it by a Const by means of generating a custom query plan.

So the boundary between PARAM_EXTERN Params and Consts is actually
mighty squishy, and thus I think it makes sense for pg_stat_statements
to mash them together.  But this logic does not extend to Vars or
function calls or much of anything else.

Maybe in the future we could have a discussion about whether
expressions involving only Params, Consts, and immutable functions
(say, "$1 + 1") could be mashed as though they were constants, on
the grounds that they'd have been reduced to a single constant if the
planner had chosen to generate a custom plan.  But I think it's too
late to consider that for v18.  I'd be okay with the rule "treat any
list of Consts and PARAM_EXTERN Params the same as any other" for v18.

I also agree with Alvaro that this discussion doesn't justify a
revert.  If the pre-v18 behavior wasn't chiseled on stone tablets,
the new behavior isn't either.  We can improve it some more later.

			regards, tom lane

  13. Re: queryId constant squashing does not support prepared statements

    Junwang Zhao <zhjwpku@gmail.com> — 2025-05-06T15:50:07Z

    Hi Dmitry,

On Sun, May 4, 2025 at 6:19 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> > On Thu, May 01, 2025 at 09:55:47PM GMT, Dmitry Dolgov wrote:
> > > On Thu, May 01, 2025 at 09:29:13AM GMT, Michael Paquier wrote:
> > >
> > > I agree that the current solution we have in the tree feels incomplete
> > > because we are not taking into account the most common cases that
> > > users would care about.  Now, allowing PARAM_EXTERN means that we
> > > allow any expression to be detected as a squashable thing, and this
> > > kinds of breaks the assumption of IsSquashableConst() where we want
> > > only constants to be allowed because EXECUTE parameters can be any
> > > kind of Expr nodes.  At least that's the intention of the code on
> > > HEAD.
> > >
> > > Now, I am not actually objecting about PARAM_EXTERN included or not if
> > > there's a consensus behind it and my arguments are considered as not
> > > relevant.  The patch is written so as it claims that a PARAM_EXTERN
> > > implies the expression to be a Const, but it may not be so depending
> > > on what the execution path is given for the parameter.  Or at least
> > > the patch could be clearer and rename the parts about the "Const"
> > > squashable APIs around queryjumblefuncs.c.
> > >
> > > [...]
> > >
> > > The PARAM_EXTERN part has been mentioned a couple of weeks ago here,
> > > btw:
> > > https://www.postgresql.org/message-id/CAA5RZ0tu6_KRiYJCFptf4_--wjFSu9cZMj1XNmOCqTNxu=VpEA@mail.gmail.com
> >
> > In fact, this has been discussed much earlier in the thread above, as
> > essentially the same implementation with T_Params, which is submitted
> > here, was part of the original patch. The concern was always whether or
> > not it will break any assumption about query identification, because
> > this way much broader scope of expressions will be considered equivalent
> > for query id computation purposes.
> >
> > At the same time after thinking about this concern more, I presume it
> > already happens at a smaller scale -- when two queries happen to have
> > the same number of parameters, they will be indistinguishable even if
> > parameters are different in some way.
>
> Returning to the topic of whether to squash list of Params.
>
> Originally squashing of Params wasn't included into the squashing patch due to
> concerns from reviewers about treating quite different queries as the same for
> the purposes of query identification. E.g. there is some assumption somewhere,
> which will be broken if we treat query with a list of integer parameters same
> as a query with a list of float parameters. For the sake of making progress
> I've decided to postpone answering this question and concentrate on more simple
> scenario. Now, as the patch was applied, I think it's a good moment to reflect
> on those concerns. It's not enough to say that we don't see any problems with
> squashing of Param, some more sound argumentation is needed. So, what will
> happen if parameters are squashed as constants?
>
> 1. One obvious impact is that more queries, that were considered distinct
> before, will have the same normalized query and hence the entry in
> pg_stat_statements. Since a Param could be pretty much anything, this can lead
> to a situation when two queries with quiet different performance profiles (e.g.
> one contains a parameter value, which is a heavy function, another one doesn't)
> are matched to one entry, making it less useful.
>
> But at the same time this already can happen if those two queries have the same
> number of parameters, since query parametrizing is intrinsically lossy in this
> sense. The only thing we do by squashing such queries is we loose information
> about the number of parameters, not properties of the parameters themselves.
>
> 2. Another tricky scenario is when queryId is used by some extension, which in
> turn makes assumption about it that are going to be rendered incorrect by
> squashing. The only type of assumptions I can imagine falling into this
> category is anything about equivalence of queries. For example, an extension
> can capture two queries, which have the same normalized entry in pgss, and
> assume all properties of those queries are the same.
>
> It's worth noting that normalized query is not transitive, i.e. if a query1 has
> the normalized version query_norm, and a query2 has the same normalized version
> query_norm, it doesn't mean query1 is equivalent query2 in all senses (e.g.
> they could have list of parameter values with different type and the same
> size). That means that such assumptions are already faulty, and could work most
> of the time only because it takes queries with a list of the same size to break
> the assumption. Squashing such queries will make them wrong more often.
>
> One can argue that we might want to be friendly to such extensions, and do not
> "break" them even further. But I don't think it's worth it, as number of such
> extensions is most likely low, if any. One more extreme case would be when an
> extension assumes that queries with the same entry in pgss have the same number
> of parameters, but I don't see how such assumption could be useful.
>
> 3. More annoying is the consequence that parameters are going to be treated as
> constants in pg_stat_statements. While mostly harmless, that would mean they're
> going to be replaced in the same way as constants. This means that the
> parameter order is going to be lost, e.g.:
>
> SELECT * FROM test_squash WHERE data IN ($4, $3, $2, $1) \bind 1 2 3 4
> -- output
> SELECT * FROM test_squash WHERE data IN ($1 /*, ... */)
>
> SELECT * FROM test_squash WHERE data IN ($1, $2, $3, $4)
>     AND id IN ($5, $6, $7, $8) \bind 1 2 3 4 5 6 7 8
> -- output
> SELECT * FROM test_squash WHERE data IN ($1 /*, ... */)
>     AND id IN ($2 /*, ... */)
>
> This representation could be confusing of course. It could be either explained
> in the documentation, or LocationLen has to be extended to carry information
> about whether it's a constant or a parameter, and do not replace the latter. In
> any case, anything more than the first parameter number will be lost, but it's
> probably not so dramatic.
>
> At the end of the day, I think the value of squashing for parameters outweighs
> the problems described above. As long as there is an agreement about that, it's
> fine by me. I've attached the more complete version of the patch (but without
> modifying LocationLen to not replace Param yet) in case if such agreemeng will
> be achieved.

Would it make sense to rename `RecordConstLocation` to something like
`RecordExpressionLocation` instead?

- /* Array of locations of constants that should be removed */
+ /* Array of locations of constants that should be removed and parameters */
  LocationLen *clocations;

should be

+ /* Array of locations of constants and parameters that should be removed */

You could also consider renaming `clocations` to `elocations`, this
may introduce
some additional churn though.

-- 
Regards
Junwang Zhao

  14. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-06T17:37:11Z

    > On Tue, May 06, 2025 at 11:50:07PM GMT, Junwang Zhao wrote:
> Would it make sense to rename `RecordConstLocation` to something like
> `RecordExpressionLocation` instead?

Yeah, naming is hard. RecordExpressionLocation is somehow more vague,
but I see what you mean, maybe something along these lines would be
indeed a better fit.

> - /* Array of locations of constants that should be removed */
> + /* Array of locations of constants that should be removed and parameters */
>   LocationLen *clocations;
>
> should be
>
> + /* Array of locations of constants and parameters that should be removed */

That was clumsy but intentional, because contrary to constants
parameters do not need to be removed. I guess I have to change the
wording a bit to make it clear.

  15. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-06T18:32:48Z

    > I also agree with Alvaro that this discussion doesn't justify a
> revert.  If the pre-v18 behavior wasn't chiseled on stone tablets,
> the new behavior isn't either.  We can improve it some more later.

As I was looking further into what we currently have in v18 and HEAD
the normalization could break if we pass a function.

For example,
"""
select where 1 in (1, 2, int4(1));
"""
the normalized string is,

"""
select where $1 in ($2 /*, ... */))
"""

Notice the extra close parenthesis that is added after the comment. This is
because although int4(1) is a function call it is rewritten as a Const
and that breaks the assumptions being made by the location of the
last expression.

Also, something like:
"""
select where 1 in (1, 2,                            cast(4 as int));
"""
is normalized as:
"""
select where $1 in ($2 /*, ... */ as int))
"""

I don't think the current state is acceptable, if it results in pg_s_s
storing an invalid normalized version of the sql.

Now, with the attached v2 supporting external params, we see other normalization
anomalies such as

"""
postgres=# select where $1 in ($3, $2) and 1 in ($4, cast($5 as int))
\bind 0 1 2 3 4
postgres-# ;
--
(0 rows)

postgres=# select toplevel, query, calls from pg_stat_statements;
 toplevel |                                  query
             | calls
----------+-------------------------------------------------------------------------+-------
 t        | select where $1 in ($2 /*, ... */) and $3 in ($4 /*, ...
*/($5 as int)) |     1
(1 row)
"""

Without properly accounting for the boundaries of the list of expressions, i.e.,
the start and end positions of '(' and ')' or '[' and ']' and normalizing the
expressions in between, it will be very difficult for the normalization to
behave sanely.

thoughts?

--
Sami Imseih
Amazon Web Services (AWS)

  16. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-06T19:42:10Z

    > On Tue, May 06, 2025 at 01:32:48PM GMT, Sami Imseih wrote:
> > I also agree with Alvaro that this discussion doesn't justify a
> > revert.  If the pre-v18 behavior wasn't chiseled on stone tablets,
> > the new behavior isn't either.  We can improve it some more later.
>
> As I was looking further into what we currently have in v18 and HEAD
> the normalization could break if we pass a function.
>
> [...]
>
> Without properly accounting for the boundaries of the list of expressions, i.e.,
> the start and end positions of '(' and ')' or '[' and ']' and normalizing the
> expressions in between, it will be very difficult for the normalization to
> behave sanely.

I don't think having the end location in this case would help -- when it
comes to ParseFuncOrColumn, looks like for coerce functions it just
replaces the original FuncCall with the argument expression. Meaning
that when jumbling we have only the coerce argument expression (Const),
which ends before the closing brace, not the parent expression.

Maybe it would be possible to address thins in not too complicated way
in fill_in_constant_lengths, since it already operates with parsed
tokens.

  17. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-06T20:01:32Z

    > > Without properly accounting for the boundaries of the list of expressions, i.e.,
> > the start and end positions of '(' and ')' or '[' and ']' and normalizing the
> > expressions in between, it will be very difficult for the normalization to
> > behave sanely.
>
> I don't think having the end location in this case would help -- when it
> comes to ParseFuncOrColumn, looks like for coerce functions it just
> replaces the original FuncCall with the argument expression. Meaning
> that when jumbling we have only the coerce argument expression (Const),
> which ends before the closing brace, not the parent expression.

If we are picking up the start and end points from gram.c and we add these
positions to A_Expr or A_ArrayExpr and then make them available to ArrayExpr,
then we know the exact boundary of the IN list. Even if a function
call is simplified down
to a constant, it will not really matter because we are going to normalize
between the original opening and closing parentheses of the IN list.

(Actually, we can even track the actual textual starting and end point of a List
as well)

Attached ( not in patch form ) is the idea for this.

```
postgres=# select where 1 in (1, int4(1));
--
(1 row)

postgres=# select where 1 in (1, int4($1::int)) \bind 1
postgres-# ;
--
(1 row)

postgres=# select toplevel, query, calls from pg_stat_statements;
 toplevel |               query                | calls
----------+------------------------------------+-------
 t        | select where $1 in ($2 /*, ... */) |     2
(1 row)
```

What do you think?

--
Sami Imseih

  18. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-06T23:31:15Z

    On Tue, May 06, 2025 at 01:32:48PM -0500, Sami Imseih wrote:
> Without properly accounting for the boundaries of the list of expressions, i.e.,
> the start and end positions of '(' and ')' or '[' and ']' and normalizing the
> expressions in between, it will be very difficult for the normalization to
> behave sanely.

FWIW, this is exactly the kind of issues we have spent time on when
improving the location detection of sub-queries for some DDL patterns,
and the parser is the only path I am aware of where this can be done
sanely because the extra characters that may, or may not, be included
in some of the expressions would be naturally discarded based on the @
locations we retrieve.

For reference, this story is part of 499edb09741b, more precisely
around this part of the thread:
https://www.postgresql.org/message-id/CACJufxF9hqyfmKEdpiG%3DPbrGdKVNP2BQjHFJh4q6639sV7NmvQ%40mail.gmail.com

(FWIW, I've seen assumptions around the detection of specific
locations done outside the parser in pg_hint_plan as well, that did
not finish well because the code makes assumptions that natural
parsers are just better at because they're designed to detect such
cases.)
--
Michael

  19. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-07T08:41:22Z

    > On Tue, May 06, 2025 at 03:01:32PM GMT, Sami Imseih wrote:
> > > Without properly accounting for the boundaries of the list of expressions, i.e.,
> > > the start and end positions of '(' and ')' or '[' and ']' and normalizing the
> > > expressions in between, it will be very difficult for the normalization to
> > > behave sanely.
> >
> > I don't think having the end location in this case would help -- when it
> > comes to ParseFuncOrColumn, looks like for coerce functions it just
> > replaces the original FuncCall with the argument expression. Meaning
> > that when jumbling we have only the coerce argument expression (Const),
> > which ends before the closing brace, not the parent expression.
>
> If we are picking up the start and end points from gram.c and we add these
> positions to A_Expr or A_ArrayExpr and then make them available to ArrayExpr,
> then we know the exact boundary of the IN list. Even if a function
> call is simplified down
> to a constant, it will not really matter because we are going to normalize
> between the original opening and closing parentheses of the IN list.
> What do you think?

Ah, I see what you mean. I think the idea is fine, it will simplify
certain things as well as address the issue. But I'm afraid adding
start/end location to A_Expr is a bit too invasive, as it's being used
for many other purposes. How about introducing a new expression for this
purposes, and use it only in in_expr/array_expr, and wrap the
corresponding expressions into it? This way the change could be applied
in a more targeted fashion.

  20. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-08T05:22:00Z

    On Wed, May 07, 2025 at 10:41:22AM +0200, Dmitry Dolgov wrote:
> Ah, I see what you mean. I think the idea is fine, it will simplify
> certain things as well as address the issue. But I'm afraid adding
> start/end location to A_Expr is a bit too invasive, as it's being used
> for many other purposes. How about introducing a new expression for this
> purposes, and use it only in in_expr/array_expr, and wrap the
> corresponding expressions into it? This way the change could be applied
> in a more targeted fashion.

Yes, that feels invasive.  The use of two static variables to track
the start and the end positions in an expression list can also be a
bit unstable to rely on, I think.  It seems to me that this part
could be handled in a new Node that takes care of tracking the two
positions, instead, be it a start/end couple or a location/length
couple?  That seems necessary to have when going through
jumbleElements().
--
Michael

  21. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-08T19:36:55Z

    > On Thu, May 08, 2025 at 02:22:00PM GMT, Michael Paquier wrote:
> On Wed, May 07, 2025 at 10:41:22AM +0200, Dmitry Dolgov wrote:
> > Ah, I see what you mean. I think the idea is fine, it will simplify
> > certain things as well as address the issue. But I'm afraid adding
> > start/end location to A_Expr is a bit too invasive, as it's being used
> > for many other purposes. How about introducing a new expression for this
> > purposes, and use it only in in_expr/array_expr, and wrap the
> > corresponding expressions into it? This way the change could be applied
> > in a more targeted fashion.
>
> Yes, that feels invasive.  The use of two static variables to track
> the start and the end positions in an expression list can also be a
> bit unstable to rely on, I think.  It seems to me that this part
> could be handled in a new Node that takes care of tracking the two
> positions, instead, be it a start/end couple or a location/length
> couple?  That seems necessary to have when going through
> jumbleElements().

To clarify, I had in mind something like in the attached patch. The
idea is to make start/end location capturing relatively independent from
the constants squashing. The new parsing node conveys the location
information, which is then getting transformed to be a part of an
ArrayExpr. It's done for in_expr only here, something similar would be
needed for array_expr as well. Feedback is appreciated.

  22. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-08T20:50:32Z

    On Thu, May 8, 2025 at 2:36 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> > On Thu, May 08, 2025 at 02:22:00PM GMT, Michael Paquier wrote:
> > On Wed, May 07, 2025 at 10:41:22AM +0200, Dmitry Dolgov wrote:
> > > Ah, I see what you mean. I think the idea is fine, it will simplify
> > > certain things as well as address the issue. But I'm afraid adding
> > > start/end location to A_Expr is a bit too invasive, as it's being used
> > > for many other purposes. How about introducing a new expression for this
> > > purposes, and use it only in in_expr/array_expr, and wrap the
> > > corresponding expressions into it? This way the change could be applied
> > > in a more targeted fashion.
> >
> > Yes, that feels invasive.  The use of two static variables to track
> > the start and the end positions in an expression list can also be a
> > bit unstable to rely on, I think.  It seems to me that this part
> > could be handled in a new Node that takes care of tracking the two
> > positions, instead, be it a start/end couple or a location/length
> > couple?  That seems necessary to have when going through
> > jumbleElements().
>
> To clarify, I had in mind something like in the attached patch. The
> idea is to make start/end location capturing relatively independent from
> the constants squashing. The new parsing node conveys the location
> information, which is then getting transformed to be a part of an
> ArrayExpr. It's done for in_expr only here, something similar would be
> needed for array_expr as well. Feedback is appreciated.

Thanks! I took a quick look at v1-0001 and it feels like a much better approach
than the quick hack I put together earlier. I will look thoroughly.

--
Sami

  23. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-08T23:47:58Z

    On Thu, May 08, 2025 at 03:50:32PM -0500, Sami Imseih wrote:
> On Thu, May 8, 2025 at 2:36 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>> To clarify, I had in mind something like in the attached patch. The
>> idea is to make start/end location capturing relatively independent from
>> the constants squashing. The new parsing node conveys the location
>> information, which is then getting transformed to be a part of an
>> ArrayExpr. It's done for in_expr only here, something similar would be
>> needed for array_expr as well. Feedback is appreciated.
> 
> Thanks! I took a quick look at v1-0001 and it feels like a much better approach
> than the quick hack I put together earlier. I will look thoroughly.

 SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
-                       query                        | calls 
-----------------------------------------------------+-------
- SELECT ARRAY[$1 /*, ... */]                        |     1
- SELECT pg_stat_statements_reset() IS NOT NULL AS t |     1
+                         query                         | calls 
+-------------------------------------------------------+-------
+ SELECT ARRAY[$1, $2, $3, $4, $5, $6, $7, $8, $9, $10] |     1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t    |     1
 (2 rows)

Yes, we are going to need more than that for such cases if we want to
cover all the ground we're aiming for.

Putting that aside, the test coverage for ARRAY[] elements is also
very limited on HEAD with one single test only with a set of
constants.  We really should improve that, tracking more patterns and
more mixed combinations to see what gets squashed and what is not.  So
this should be extended with more cases, including expressions,
parameters and sublinks, with checks on pg_stat_statements.calls to
see how the counters are aggregated.  That's going to be important
when people play with this code to track how things change when
manipulating the element jumbling.  I'd suggest to do that separately
of the rest.
--
Michael

  24. Re: queryId constant squashing does not support prepared statements

    Junwang Zhao <zhjwpku@gmail.com> — 2025-05-09T03:05:43Z

    Hi Dmitry,

On Fri, May 9, 2025 at 3:36 AM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> > On Thu, May 08, 2025 at 02:22:00PM GMT, Michael Paquier wrote:
> > On Wed, May 07, 2025 at 10:41:22AM +0200, Dmitry Dolgov wrote:
> > > Ah, I see what you mean. I think the idea is fine, it will simplify
> > > certain things as well as address the issue. But I'm afraid adding
> > > start/end location to A_Expr is a bit too invasive, as it's being used
> > > for many other purposes. How about introducing a new expression for this
> > > purposes, and use it only in in_expr/array_expr, and wrap the
> > > corresponding expressions into it? This way the change could be applied
> > > in a more targeted fashion.
> >
> > Yes, that feels invasive.  The use of two static variables to track
> > the start and the end positions in an expression list can also be a
> > bit unstable to rely on, I think.  It seems to me that this part
> > could be handled in a new Node that takes care of tracking the two
> > positions, instead, be it a start/end couple or a location/length
> > couple?  That seems necessary to have when going through
> > jumbleElements().
>
> To clarify, I had in mind something like in the attached patch. The
> idea is to make start/end location capturing relatively independent from
> the constants squashing. The new parsing node conveys the location
> information, which is then getting transformed to be a part of an
> ArrayExpr. It's done for in_expr only here, something similar would be
> needed for array_expr as well. Feedback is appreciated.

+/*
+ * A wrapper expression to record start and end location
+ */
+typedef struct LocationExpr
+{
+ NodeTag type;
+
+ /* the node to be wrapped */
+ Node    *expr;
+ /* token location, or -1 if unknown */
+ ParseLoc start_location;
+ /* token location, or -1 if unknown */
+ ParseLoc end_location;
+} LocationExpr;

Why not a location and a length, it should be more natural, it
seems we use this convention in some existing nodes, like
RawStmt, InsertStmt etc.

-- 
Regards
Junwang Zhao

  25. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-09T05:35:33Z

    On Fri, May 09, 2025 at 11:05:43AM +0800, Junwang Zhao wrote:
> Why not a location and a length, it should be more natural, it
> seems we use this convention in some existing nodes, like
> RawStmt, InsertStmt etc.

These are new concepts as of Postgres 18 (aka only on HEAD), chosen
mainly to match with the internals of pg_stat_statements as far as I
recall.  Doing the same here would not hurt, but it may be better
depending on the cases to rely on a start/end.  I suspect that
switching from one to the other should not change much the internal
squashing logic.
--
Michael

  26. Re: queryId constant squashing does not support prepared statements

    Junwang Zhao <zhjwpku@gmail.com> — 2025-05-09T06:48:35Z

    On Fri, May 9, 2025 at 1:35 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, May 09, 2025 at 11:05:43AM +0800, Junwang Zhao wrote:
> > Why not a location and a length, it should be more natural, it
> > seems we use this convention in some existing nodes, like
> > RawStmt, InsertStmt etc.
>
> These are new concepts as of Postgres 18 (aka only on HEAD), chosen
> mainly to match with the internals of pg_stat_statements as far as I
> recall.  Doing the same here would not hurt, but it may be better
> depending on the cases to rely on a start/end.

ISTM that for string manipulation, start_pos/length are more appropriate,
start/end are often better suited for iterator use, where start refers to the
first element and end marks the position one past the last element.

Just my opinion, I can live with either way though.

> I suspect that
> switching from one to the other should not change much the internal
> squashing logic.

Yeah, not much difference, one can easily be computed from the other.

> --
> Michael



-- 
Regards
Junwang Zhao

  27. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-09T08:10:46Z

    > On Fri, May 09, 2025 at 02:35:33PM GMT, Michael Paquier wrote:
> On Fri, May 09, 2025 at 11:05:43AM +0800, Junwang Zhao wrote:
> > Why not a location and a length, it should be more natural, it
> > seems we use this convention in some existing nodes, like
> > RawStmt, InsertStmt etc.
>
> These are new concepts as of Postgres 18 (aka only on HEAD), chosen
> mainly to match with the internals of pg_stat_statements as far as I
> recall.  Doing the same here would not hurt, but it may be better
> depending on the cases to rely on a start/end.  I suspect that
> switching from one to the other should not change much the internal
> squashing logic.

Right, switching from start/length to start/end wouldn't change much for
squashing. I didn't have any strong reason to go with start/end from my
side, so if start/length is more aligned with other nodes, let's change
that.

  28. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-09T08:12:24Z

    > On Fri, May 09, 2025 at 08:47:58AM GMT, Michael Paquier wrote:
>  SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
> -                       query                        | calls
> -----------------------------------------------------+-------
> - SELECT ARRAY[$1 /*, ... */]                        |     1
> - SELECT pg_stat_statements_reset() IS NOT NULL AS t |     1
> +                         query                         | calls
> +-------------------------------------------------------+-------
> + SELECT ARRAY[$1, $2, $3, $4, $5, $6, $7, $8, $9, $10] |     1
> + SELECT pg_stat_statements_reset() IS NOT NULL AS t    |     1
>  (2 rows)
>
> Yes, we are going to need more than that for such cases if we want to
> cover all the ground we're aiming for.
>
> Putting that aside, the test coverage for ARRAY[] elements is also
> very limited on HEAD with one single test only with a set of
> constants.  We really should improve that, tracking more patterns and
> more mixed combinations to see what gets squashed and what is not.  So
> this should be extended with more cases, including expressions,
> parameters and sublinks, with checks on pg_stat_statements.calls to
> see how the counters are aggregated.  That's going to be important
> when people play with this code to track how things change when
> manipulating the element jumbling.  I'd suggest to do that separately
> of the rest.

Agree, I'll try to extend number of test cases here as a separate patch.

  29. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-09T17:47:19Z

    > > To clarify, I had in mind something like in the attached patch. The
> > idea is to make start/end location capturing relatively independent from
> > the constants squashing. The new parsing node conveys the location
> > information, which is then getting transformed to be a part of an
> > ArrayExpr. It's done for in_expr only here, something similar would be
> > needed for array_expr as well. Feedback is appreciated.
>
> Thanks! I took a quick look at v1-0001 and it feels like a much better approach
> than the quick hack I put together earlier. I will look thoroughly.

I took a look at v1-0001 and I am wondering if this can be further simplified.

We really need a new Node just to wrap the start/end locations of the List
coming from the in_expr and this node should not really be needed past parsing.

array_expr is even simpler because we have the boundaries available
when makeAArrayExpr is called.

So, I think we can create a new parse node ( parsenode.h ) that will only be
used in parsing (and gram.c only ) to track the start/end locations
and List and
based on this node we can create A_ArrayExpr and A_Expr with the List
of boundaries,
and then all we have to do is update ArrayExpr with the boundaries during
the respective transformXExpr call. This seems like a much simpler approach
that also addresses Michael's concern of defining static variables in gram.y to
track the boundaries.

what do you think?

--
Sami

  30. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-12T08:11:15Z

    > On Fri, May 09, 2025 at 10:12:24AM GMT, Dmitry Dolgov wrote:
> Agree, I'll try to extend number of test cases here as a separate patch.

Here is the extended version, where start/end is replaced by
location/length, array_expr is handled as well, and more ARRAY cases are
added.

  31. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-12T08:15:59Z

    > On Fri, May 09, 2025 at 12:47:19PM GMT, Sami Imseih wrote:
> So, I think we can create a new parse node ( parsenode.h ) that will only be
> used in parsing (and gram.c only ) to track the start/end locations
> and List and
> based on this node we can create A_ArrayExpr and A_Expr with the List
> of boundaries,
> and then all we have to do is update ArrayExpr with the boundaries during
> the respective transformXExpr call. This seems like a much simpler approach
> that also addresses Michael's concern of defining static variables in gram.y to
> track the boundaries.

The static variables was only part of the concern, another part was
using A_Expr to carry this information, which will have impact on lots
of unrelated code.

  32. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-12T22:40:43Z

    > > On Fri, May 09, 2025 at 12:47:19PM GMT, Sami Imseih wrote:
> > So, I think we can create a new parse node ( parsenode.h ) that will only be
> > used in parsing (and gram.c only ) to track the start/end locations
> > and List and
> > based on this node we can create A_ArrayExpr and A_Expr with the List
> > of boundaries,
> > and then all we have to do is update ArrayExpr with the boundaries during
> > the respective transformXExpr call. This seems like a much simpler approach
> > that also addresses Michael's concern of defining static variables in gram.y to
> > track the boundaries.
>
> The static variables was only part of the concern, another part was
> using A_Expr to carry this information, which will have impact on lots
> of unrelated code.

What would be the problem if A_Expr carries an extra pointer to a List?
It already had other fields, rexpr, lexpr and location that could be no-op.

Also, LocationExpr is not really an expression node, but a wrapper to
an expression node, so I think it's wrong to define it as a Node and be
required to add the necessary handling for it in nodeFuncs.c. I think we
can just define it as a struct in gram.y so it can carry the locations of the
expression and then set the List of the location boundaries in
A_Expr and A_ArrayExpr. right?

typedef struct LocationExpr
{
       Node       *expr;
       ParseLoc        start_location;
       ParseLoc        end_location;
} LocationExpr;


--
Sami

  33. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-13T16:57:35Z

    > On Mon, May 12, 2025 at 06:40:43PM GMT, Sami Imseih wrote:
> > The static variables was only part of the concern, another part was
> > using A_Expr to carry this information, which will have impact on lots
> > of unrelated code.
>
> What would be the problem if A_Expr carries an extra pointer to a List?
> It already had other fields, rexpr, lexpr and location that could be no-op.

They can be empty sometimes, but the new fields will be empty 99% of the
time. This is a clear sign to me that this informaton does not belong to
a node for "infix, prefix, and postfix expressions", don't you think?

  34. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-19T21:30:25Z

    On Mon, May 12, 2025 at 06:40:43PM -0400, Sami Imseih wrote:
> Also, LocationExpr is not really an expression node, but a wrapper to
> an expression node, so I think it's wrong to define it as a Node and be
> required to add the necessary handling for it in nodeFuncs.c. I think we
> can just define it as a struct in gram.y so it can carry the locations of the
> expression and then set the List of the location boundaries in
> A_Expr and A_ArrayExpr. right?

Right.  LocationExpr is not a full Node, so if we can do these
improvements without it we have less maintenance to worry about across
the board with less code paths.  At the end, I think that we should
try to keep the amount of work done by PGSS as minimal as possible.

I was a bit worried about not using a Node but Sami has reminded me
last week that we already have in gram.y the concept of using some
private structures to track intermediate results done by the parsing
that we sometimes do not want to push down to the code calling the
parser.  If we can do the same, the result could be nicer.

By the way, the new test cases for ARRAY lists are sent in the last
patch of the series posted on this thread:
https://www.postgresql.org/message-id/7zbzwk4btnxoo4o3xbtzefoqvht54cszjj4bol22fmej5nmgkf@dbcn4wtakw4y

These should be first in the list, IMO, so as it is possible to track
what the behavior was before the new logic as of HEAD, and what the
behavior would become after the new logic.
--
Michael

  35. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-20T09:03:52Z

    > On Tue, May 20, 2025 at 06:30:25AM GMT, Michael Paquier wrote:
> On Mon, May 12, 2025 at 06:40:43PM -0400, Sami Imseih wrote:
> > Also, LocationExpr is not really an expression node, but a wrapper to
> > an expression node, so I think it's wrong to define it as a Node and be
> > required to add the necessary handling for it in nodeFuncs.c. I think we
> > can just define it as a struct in gram.y so it can carry the locations of the
> > expression and then set the List of the location boundaries in
> > A_Expr and A_ArrayExpr. right?
>
> Right.  LocationExpr is not a full Node, so if we can do these
> improvements without it we have less maintenance to worry about across
> the board with less code paths.  At the end, I think that we should
> try to keep the amount of work done by PGSS as minimal as possible.
>
> I was a bit worried about not using a Node but Sami has reminded me
> last week that we already have in gram.y the concept of using some
> private structures to track intermediate results done by the parsing
> that we sometimes do not want to push down to the code calling the
> parser.  If we can do the same, the result could be nicer.

I believe it's worth to not only to keep amount of work to support
LocationExpr as minimal as possible, but also impact on the existing
code. What I see as a problem is keeping such specific information as
the location boundaries in such a generic expression as A_Expr, where it
will almost never be used. Do I get it right, you folks are ok with
that?

At the same time AFAICT there isn't much more code paths to worry about
in case of a LocationExpr as a node -- in the end all options would have
to embed the location information into ArrayExpr during transformation,
independently from how this information was conveyed. Aside that the
only extra code we've got is node functions (exprType, etc). Is there
anything I'm missing here?

> By the way, the new test cases for ARRAY lists are sent in the last
> patch of the series posted on this thread:
> https://www.postgresql.org/message-id/7zbzwk4btnxoo4o3xbtzefoqvht54cszjj4bol22fmej5nmgkf@dbcn4wtakw4y
>
> These should be first in the list, IMO, so as it is possible to track
> what the behavior was before the new logic as of HEAD, and what the
> behavior would become after the new logic.

Sure, I can reshuffle that.

BTW, I'm going to be away for a couple of weeks soon. So if you want to
decide one way or another soonish, let's do it now.

  36. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-20T14:23:59Z

    > On Tue, May 20, 2025 at 11:03:52AM GMT, Dmitry Dolgov wrote:
> > By the way, the new test cases for ARRAY lists are sent in the last
> > patch of the series posted on this thread:
> > https://www.postgresql.org/message-id/7zbzwk4btnxoo4o3xbtzefoqvht54cszjj4bol22fmej5nmgkf@dbcn4wtakw4y
> >
> > These should be first in the list, IMO, so as it is possible to track
> > what the behavior was before the new logic as of HEAD, and what the
> > behavior would become after the new logic.
>
> Sure, I can reshuffle that.

Here is it, but the results are pretty much expected.

  37. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-20T21:50:12Z

    > At the same time AFAICT there isn't much more code paths
> to worry about in case of a LocationExpr as a node

I can imagine there are others like value expressions,
row expressions, json array expressions, etc. that we may
want to also normalize.

> I believe it's worth to not only to keep amount of work to support
> LocationExpr as minimal as possible, but also impact on the existing
> code. What I see as a problem is keeping such specific information as
> the location boundaries in such a generic expression as A_Expr, where it
> will almost never be used. Do I get it right, you folks are ok with
> that?

There are other examples of fields that are minimally used in other structs.
Here is one I randomly spotted in SelectStmt such as SortClause, Limit options,
etc. I think the IN list is quite a common case, otherwise we would not
care as much as we do.

There are other examples of fields that are minimally used in other structs.
Here is one I randomly spotted in SelectStmt such as SortClause, Limit options,
etc. I think the IN list is quite a common case, otherwise we would not
care as much as we do. Adding another 8 bytes to the struts does not seem
like that big of a problem to me, especially the structs will remain below
64 bytes.


```
(gdb) ptype /o A_Expr
type = struct A_Expr {
/*      0      |       4 */    NodeTag type;
/*      4      |       4 */    A_Expr_Kind kind;
/*      8      |       8 */    List *name;
/*     16      |       8 */    Node *lexpr;
/*     24      |       8 */    Node *rexpr;
/*     32      |       4 */    ParseLoc location;
/* XXX  4-byte padding   */

                               /* total size (bytes):   40 */
                             }


(gdb) ptype \o A_ArrayExpr
Invalid character '\' in expression.
(gdb) ptype /o A_ArrayExpr
type = struct A_ArrayExpr {
/*      0      |       4 */    NodeTag type;
/* XXX  4-byte hole      */
/*      8      |       8 */    List *elements;
/*     16      |       4 */    ParseLoc location;
/* XXX  4-byte padding   */

                               /* total size (bytes):   24 */
                             }
```


In general, Making something like T_LocationExpr as a query node
seems totally wrong to me. It's not a node, but rather a temporary
wrapper of some location information and it does not seem it has
business being used by the time we get to thee expression
transformations. It seems very odd considering location information
are simple fields in the parse node itself.


>I was a bit worried about not using a Node but Sami has reminded me
> last week that we already have in gram.y the concept of using some
> private structures to track intermediate results done by the parsing

Attached is a sketch of what I mean. There is a private struct that tracks
the list boundaries and this can wrap in_expr or whatever else makes
sense in the future.

+typedef struct ListWithBoundary
+{
+       Node       *expr;
+       ParseLoc        start;
+       ParseLoc        end;
+} ListWithBoundary;
+
 /* ConstraintAttributeSpec yields an integer bitmask of these flags: */
 #define CAS_NOT_DEFERRABLE                     0x01
 #define CAS_DEFERRABLE                         0x02
@@ -269,6 +276,7 @@ static Node *makeRecursiveViewSelect(char
*relname, List *aliases, Node *query);
        struct KeyAction *keyaction;
        ReturningClause *retclause;
        ReturningOptionKind retoptionkind;
+       struct ListWithBoundary *listwithboundary;
 }


+%type <listwithboundary> in_expr

The values are then added to start_location/end_location ParseLoc in
A_ArrayExpr and A_Expr. Doing it this will keep changes to the parse_expr.c
code to a minimum, only the IN transformation will need to set the values
of the A_Expr into the final A_ArrayExpr.

--
Sami Imseih
Amazon Web Services (AWS)

  38. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-21T15:00:06Z

    > On Tue, May 20, 2025 at 04:50:12PM GMT, Sami Imseih wrote:
> > At the same time AFAICT there isn't much more code paths
> > to worry about in case of a LocationExpr as a node
>
> I can imagine there are others like value expressions,
> row expressions, json array expressions, etc. that we may
> want to also normalize.

Exactly. When using a node, one can explicitly wrap whatever is needed
into it, while otherwise one would need to find a new way to piggy back
on A_Expr in a new context.

> There are other examples of fields that are minimally used in other structs.
> Here is one I randomly spotted in SelectStmt such as SortClause, Limit options,
> etc.

The way I see it, there is a difference -- I assume those structures
were designed for such cases, where the location range would be just
slapped on top of A_Expr.

> Attached is a sketch of what I mean. There is a private struct that tracks
> the list boundaries and this can wrap in_expr or whatever else makes
> sense in the future.

Just fyi, I don't think this thread is attached to any CF item, meaning
it will not be pulled by the CF bot. In that case feel free to post
diffs in the patch format. I'll take a look at the proposed change, but
a bit later.

  39. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-22T01:22:19Z

    > > > At the same time AFAICT there isn't much more code paths
> > > to worry about in case of a LocationExpr as a node
> >
> > I can imagine there are others like value expressions,
> > row expressions, json array expressions, etc. that we may
> > want to also normalize.

> Exactly. When using a node, one can explicitly wrap whatever is needed
> into it, while otherwise one would need to find a new way to piggy back
> on A_Expr in a new context.

Looking at the VALUES expression case, we will need to carry the info
with SelectStmt and ultimately to RangeTblEntry which is where the
values_list is, so either approach we take RangeTblEntry will need the
LocationExpr pointer or the additional ParseLoc info I am suggesting.
A_Expr is not used in the values list case.


> I'll take a look at the proposed change, but a bit later.

Here is a v4 to compare with v3.

0001- is the infrastructure to track the boundaries
0002- the changes to jumbling
0003 - the additional tests introduced in v3


--
Sami

  40. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-22T09:03:46Z

    > On Wed, May 21, 2025 at 08:22:19PM GMT, Sami Imseih wrote:
> > > > At the same time AFAICT there isn't much more code paths
> > > > to worry about in case of a LocationExpr as a node
> > >
> > > I can imagine there are others like value expressions,
> > > row expressions, json array expressions, etc. that we may
> > > want to also normalize.
>
> > Exactly. When using a node, one can explicitly wrap whatever is needed
> > into it, while otherwise one would need to find a new way to piggy back
> > on A_Expr in a new context.
>
> Looking at the VALUES expression case, we will need to carry the info
> with SelectStmt and ultimately to RangeTblEntry which is where the
> values_list is, so either approach we take RangeTblEntry will need the
> LocationExpr pointer or the additional ParseLoc info I am suggesting.
> A_Expr is not used in the values list case.

Right, that's precisely my point -- introducing a new node will allow to
to use the same generalized mechanism in such scenarios as well, instead
of every time inventing something new.

> > I'll take a look at the proposed change, but a bit later.
>
> Here is a v4 to compare with v3.
>
> 0001- is the infrastructure to track the boundaries
> 0002- the changes to jumbling

Just to call this out, I don't think there is an agreement on squashing
Params, which you have added into 0002. Let's discuss this change
separately from the 18 open item.

---

Here is a short summary of the open item:

* An issue has been discovered with the squashing feature in 18, which
  can lead to invalid normalized queries in pg_stat_statement.

* The proposed fix extends gram.y functionality capturing
  the end location for list expressions to address that.

* There is a disagreement on how exactly to capture the location, the
  options are introducing a new node LocationExpr or piggy back on an
  existing A_Expr. I find the former more flexible and less invasive,
  but looks like there are also other opinions.

Now, both flavour of the proposed solution could be still concidered too
invasive to be applied as a bug fix. I personally don't see it like
this, but I'm obviously biased. This leads us to following decisions to
be made:

* Is modifying parser (either adding a new node or modifying an existing
  one) acceptable at this stage? I guess it would be enough to collect
  couple of votes yes/no in this thread.

* If it's not acceptable, the feature could be reverted in 18, and the
  fix could be applied to the master branch only.

I'm fine with both outcomes (apply the fix to both 18 and master, or
revert in 18 and apply the fix on master), and leave the decision to
Álvaro (sorry for causing all the troubles). It's fair to say that
reverting the feature will be the least risky move.

  41. Re: queryId constant squashing does not support prepared statements

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-05-22T11:43:36Z

    On 2025-May-22, Dmitry Dolgov wrote:

> Just to call this out, I don't think there is an agreement on squashing
> Params, which you have added into 0002.

Actually I think we do have agreement on squashing PARAM_EXTERN Params.
https://postgr.es/m/3086744.1746500983@sss.pgh.pa.us

> Now, both flavour of the proposed solution could be still concidered too
> invasive to be applied as a bug fix. I personally don't see it like
> this, but I'm obviously biased. This leads us to following decisions to
> be made:
> 
> * Is modifying parser (either adding a new node or modifying an existing
>   one) acceptable at this stage? I guess it would be enough to collect
>   couple of votes yes/no in this thread.

IMO adding a struct as suggested is okay, especially if it reduces the
overall code complexity.  But we don't want a node, just a bare struct.
Adding a node would be more troublesome.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)

  42. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-22T20:10:34Z

    > IMO adding a struct as suggested is okay, especially if it reduces the
> overall code complexity.  But we don't want a node, just a bare struct.
> Adding a node would be more troublesome.

In v4, a new private struct is added in gram.y, but we are also adding
additional fields to track the expression boundaries to the required
nodes.

--
Sami

  43. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-22T23:37:20Z

    On Thu, May 22, 2025 at 03:10:34PM -0500, Sami Imseih wrote:
> > IMO adding a struct as suggested is okay, especially if it reduces the
> > overall code complexity.  But we don't want a node, just a bare struct.
> > Adding a node would be more troublesome.
> 
> In v4, a new private struct is added in gram.y, but we are also adding
> additional fields to track the expression boundaries to the required
> nodes.

Handling external parameters as something that gets squashed is also
the consensus I am understanding we've reached.  I'm OK with it.

Upthread, scenarios with multiple IN lists was mentioned to be broken:
https://www.postgresql.org/message-id/CAA5RZ0ts6zb-efiJ+K31Z_YDU=M7tHE43vv6ZBCqQxiABr3Yaw@mail.gmail.com

For example with bind queries like that:
select where $1 in ($3, $2) and 1 in ($4, cast($5 as int))
\bind 0 1 2 3 4

Should we have a bit more coverage, where we use multiple IN and/or
ARRAY lists with constants and/or external parameters?

v4-0003 with the extra tests for ARRAY can be applied first, with the
test output slightly adjusted and the casts showing up.  Now, looking
independently at v4-0001, it is a bit hard to say what's the direct
benefit of this patch, because nothing in the tests of pgss change
after applying it.  Could the benefit of this patch be demonstrated so
as it is possible to compare what's the current vs the what-would-be
new behavior?

The patterns generated when using casts is still a bit funny, but
perhaps nobody will bother much about the result generated as these
are uncommon.  For example, this gets squashed, with the end of the
cast included:
Q: select where 2 in (1, 4) and 1 in (5, (cast(7 as int)), 6, cast(8 as int));
R: select where $1 in ($2 /*, ... */) and $3 in ($4 /*, ... */ as int))

This does not get squashed:
Q: select where 2 in (1, 4) and
1 in (5, cast(7 as int), 6, (cast(8 as int)), 9, 10, (cast(8 as text))::int);
R: select where $1 in ($2 /*, ... */) and
$3 in ($4, cast($5 as int), $6, (cast($7 as int)), $8, $9, (cast($10 as text))::int) 

This is the kind of stuff that should also have coverage for, IMO, or
we will never keep track of what the existing behavior is, and if
things break in some way in the future.

FWIW, with v4-0002 applied, I am seeing one diff in the dml tests,
where a IN list is not squashed for pgss_dml_tab.

The squashing tests point to more issues in the v4 series:
- Some lists are not getting squashed anymore.
- Some spacing issues, like "( $5)::jsonb".
Am I missing something?
--
Michael

  44. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-23T03:23:31Z

    > For example with bind queries like that:
> select where $1 in ($3, $2) and 1 in ($4, cast($5 as int))
> \bind 0 1 2 3 4
>
> Should we have a bit more coverage, where we use multiple IN and/or
> ARRAY lists with constants and/or external parameters?

I will add more test coverage. All the tests we have for constants
should also have a external parameter counterpart.

> v4-0003 with the extra tests for ARRAY can be applied first, with the
> test output slightly adjusted and the casts showing up.

That was my mistake in rearranging the v3-0001 as v4-0003. I will
fix in the next revision.

> Now, looking
> independently at v4-0001, it is a bit hard to say what's the direct
> benefit of this patch, because nothing in the tests of pgss change
> after applying it.  Could the benefit of this patch be demonstrated so
> as it is possible to compare what's the current vs the what-would-be
> new behavior?

You're right, this should not be an independent patch. I had intended to
eventually merge these v4-0001 and v4-0002 but felt it was cleaner to
review separately. I'll just combine them in the next rev.

> The patterns generated when using casts is still a bit funny, but
> perhaps nobody will bother much about the result generated as these
> are uncommon.  For example, this gets squashed, with the end of the
> cast included:
> Q: select where 2 in (1, 4) and 1 in (5, (cast(7 as int)), 6, cast(8 as int));
> R: select where $1 in ($2 /*, ... */) and $3 in ($4 /*, ... */ as int))
>
> This does not get squashed:
> Q: select where 2 in (1, 4) and
> 1 in (5, cast(7 as int), 6, (cast(8 as int)), 9, 10, (cast(8 as text))::int);
> R: select where $1 in ($2 /*, ... */) and
> $3 in ($4, cast($5 as int), $6, (cast($7 as int)), $8, $9, (cast($10 as text))::int)
>
> This is the kind of stuff that should also have coverage for, IMO, or
> we will never keep track of what the existing behavior is, and if
> things break in some way in the future.

This is interesting actually. This is the behavior on HEAD, and I don't get why
the first list with the casts does not get squashed, while the second one does.
I will check IsSquashableConst tomorrow unless Dmitry gets to it first.

```
test=# select where 2 in (1, 4) and 1 in (5, cast(7 as int), 6,
(cast(8 as int)), 9, 10, (cast(8 as text))::int);
--
(0 rows)

test=# select where 1 in (5, cast(7 as int), 6);
--
(0 rows)

test=# select queryid, substr(query, 1, 100) as query from pg_stat_statements;
       queryid        |
query

----------------------+-----------------------------------------------------------------------------------
-------------------
  2125518472894925252 | select where $1 in ($2 /*, ... */) and $3 in
($4, cast($5 as int), $6, (cast($7 as
 int)), $8, $9, (c
 -4436613157077978160 | select where $1 in ($2 /*, ... */)

```

> FWIW, with v4-0002 applied, I am seeing one diff in the dml tests,
> where a IN list is not squashed for pgss_dml_tab.

hmm, I did not observe the same diff.

--
Sami

  45. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-23T07:06:16Z

    > On Thu, May 22, 2025 at 10:23:31PM GMT, Sami Imseih wrote:
> > This does not get squashed:
> > Q: select where 2 in (1, 4) and
> > 1 in (5, cast(7 as int), 6, (cast(8 as int)), 9, 10, (cast(8 as text))::int);
> > R: select where $1 in ($2 /*, ... */) and
> > $3 in ($4, cast($5 as int), $6, (cast($7 as int)), $8, $9, (cast($10 as text))::int)
>
> This is interesting actually. This is the behavior on HEAD, and I don't get why
> the first list with the casts does not get squashed, while the second one does.
> I will check IsSquashableConst tomorrow unless Dmitry gets to it first.

IsSquashableConst has intentionally a limited set of test for
"constantness", in particular it does not recurse. The case above

    (cast(8 as text))::int

features two CoerceViaIO expressions one inside another, hence
IsSquashableConst returns false.

  46. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-23T14:05:54Z

    > > On Thu, May 22, 2025 at 10:23:31PM GMT, Sami Imseih wrote:
> > > This does not get squashed:
> > > Q: select where 2 in (1, 4) and
> > > 1 in (5, cast(7 as int), 6, (cast(8 as int)), 9, 10, (cast(8 as text))::int);
> > > R: select where $1 in ($2 /*, ... */) and
> > > $3 in ($4, cast($5 as int), $6, (cast($7 as int)), $8, $9, (cast($10 as text))::int)
> >
> > This is interesting actually. This is the behavior on HEAD, and I don't get why
> > the first list with the casts does not get squashed, while the second one does.
> > I will check IsSquashableConst tomorrow unless Dmitry gets to it first.
>
> IsSquashableConst has intentionally a limited set of test for
> "constantness", in particular it does not recurse. The case above
>
>     (cast(8 as text))::int
>
> features two CoerceViaIO expressions one inside another, hence
> IsSquashableConst returns false.


Should we be doing something like this? to unwrap RelabelType or
CoerceViaIO until we have a different type of node to check
for later on. We can guard the loop and break out after x amount
of times as well. At minimum, we should try to unwrap at least
2 times for some of the common real-world scenarios.

What do you think?

```
while (IsA(element, RelabelType) || IsA(element, CoerceViaIO))
{
       if (IsA(element, RelabelType))
           element = (Node *) ((RelabelType *) element)->arg;
       else if (IsA(element, CoerceViaIO))
            element = (Node *) ((CoerceViaIO *) element)->arg;
}
```

--
Sami

  47. Re: queryId constant squashing does not support prepared statements

    Dmitry Dolgov <9erthalion6@gmail.com> — 2025-05-23T14:29:45Z

    > On Fri, May 23, 2025 at 09:05:54AM GMT, Sami Imseih wrote:
> > > On Thu, May 22, 2025 at 10:23:31PM GMT, Sami Imseih wrote:
> > > > This does not get squashed:
> > > > Q: select where 2 in (1, 4) and
> > > > 1 in (5, cast(7 as int), 6, (cast(8 as int)), 9, 10, (cast(8 as text))::int);
> > > > R: select where $1 in ($2 /*, ... */) and
> > > > $3 in ($4, cast($5 as int), $6, (cast($7 as int)), $8, $9, (cast($10 as text))::int)
> > >
> > > This is interesting actually. This is the behavior on HEAD, and I don't get why
> > > the first list with the casts does not get squashed, while the second one does.
> > > I will check IsSquashableConst tomorrow unless Dmitry gets to it first.
> >
> > IsSquashableConst has intentionally a limited set of test for
> > "constantness", in particular it does not recurse. The case above
> >
> >     (cast(8 as text))::int
> >
> > features two CoerceViaIO expressions one inside another, hence
> > IsSquashableConst returns false.
>
> Should we be doing something like this? to unwrap RelabelType or
> CoerceViaIO until we have a different type of node to check
> for later on. We can guard the loop and break out after x amount
> of times as well. At minimum, we should try to unwrap at least
> 2 times for some of the common real-world scenarios.
>
> What do you think?
>
> ```
> while (IsA(element, RelabelType) || IsA(element, CoerceViaIO))
> {
>        if (IsA(element, RelabelType))
>            element = (Node *) ((RelabelType *) element)->arg;
>        else if (IsA(element, CoerceViaIO))
>             element = (Node *) ((CoerceViaIO *) element)->arg;
> }
> ```

I think it's better to recursively call IsSquashableConst on the nested
expression (arg or args for FuncExpr). Something like that was done in
the original patch version and was concidered too much at that time, but
since it looks like all the past concerns are lifted, why not. Do not
forget check_stack_depth.

  48. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-24T00:17:14Z

    On Fri, May 23, 2025 at 04:29:45PM +0200, Dmitry Dolgov wrote:
> I think it's better to recursively call IsSquashableConst on the nested
> expression (arg or args for FuncExpr). Something like that was done in
> the original patch version and was concidered too much at that time, but
> since it looks like all the past concerns are lifted, why not. Do not
> forget check_stack_depth.

AFAIK, we have already a couple of check_stack_depth() calls during
some node transformations after-parsing.  At this level of the code
that would be a new thing..
--
Michael

  49. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-24T01:05:47Z

    > On Fri, May 23, 2025 at 04:29:45PM +0200, Dmitry Dolgov wrote:
> > I think it's better to recursively call IsSquashableConst on the nested
> > expression (arg or args for FuncExpr). Something like that was done in
> > the original patch version and was concidered too much at that time, but
> > since it looks like all the past concerns are lifted, why not. Do not
> > forget check_stack_depth.
>
> AFAIK, we have already a couple of check_stack_depth() calls during
> some node transformations after-parsing.  At this level of the code
> that would be a new thing..

I think the recursion will simplify the logic inside
IsSquashableConstants. I will
probably add that as a separate patch that maybe will get applied to HEAD only.

Something I want agreement on is the following.

Since we assign new parameter symbols based on the highest external param
from the original query, as stated in the docs [0] "The parameter
symbols used to replace
constants in representative query texts start from the next number after the
highest $n parameter in the original query text", we could have gaps
in assigning
symbol values, such as the case below.

```
test=# select where 1 in ($1, $2, $3) and 1 = $4
test-# \bind 1 2 3 4
test-# ;
--
(0 rows)

test=# select query from pg_stat_statements;
                     query
------------------------------------------------
 select where $5 in ($6 /*, ... */) and $7 = $4
```

I don't think there is much we can do here, without introducing some serious
complexity. I think the docs make this scenario clear.

Thoughts?

[0] https://www.postgresql.org/docs/current/pgstatstatements.html

--
Sami

  50. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-24T01:42:34Z

    On Fri, May 23, 2025 at 08:05:47PM -0500, Sami Imseih wrote:
> Since we assign new parameter symbols based on the highest external param
> from the original query, as stated in the docs [0] "The parameter
> symbols used to replace
> constants in representative query texts start from the next number after the
> highest $n parameter in the original query text", we could have gaps
> in assigning
> symbol values, such as the case below.
> 
> ```
> test=# select where 1 in ($1, $2, $3) and 1 = $4
> test-# \bind 1 2 3 4
> test-# ;
> --
> (0 rows)
> 
> test=# select query from pg_stat_statements;
>                      query
> ------------------------------------------------
>  select where $5 in ($6 /*, ... */) and $7 = $4
> ```
> 
> I don't think there is much we can do here, without introducing some serious
> complexity. I think the docs make this scenario clear.

In v17, we are a bit smarter with the numbering, with a normalization
giving the following, starting at $1:
select where $5 in ($1, $2, $3) and $6 = $4

So your argument about the $n parameters is kind of true, but I think
the numbering logic in v17 to start at $1 is a less-confusing result.
I would imagine that the squashed logic should give the following
result on HEAD in this case if we want a maximum of consistency with
the squashing of the IN elements taken into account:
select where $3 in ($1 /*, ... */) and $4 = $2

Starting the count of the parameters at $4 would be strange.
--
Michael

  51. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-24T14:35:24Z

    > In v17, we are a bit smarter with the numbering, with a normalization
> giving the following, starting at $1:
> select where $5 in ($1, $2, $3) and $6 = $4
>
> So your argument about the $n parameters is kind of true, but I think
> the numbering logic in v17 to start at $1 is a less-confusing result.
> I would imagine that the squashed logic should give the following
> result on HEAD in this case if we want a maximum of consistency with
> the squashing of the IN elements taken into account:
> select where $3 in ($1 /*, ... */) and $4 = $2
>
> Starting the count of the parameters at $4 would be strange.

yeah, I think the correct answer is we need to handle 2 cases.

1. If we don't have a squashed list, then we just do what we do now.

2. If we have 1 or more squashed lists, then we can't guarantee
the $n parameter as was supplied by the user and we simply rename
the $n starting from 1.

therefore, a user supplied query like this:
```
select where $5 in ($1, $2, $3) and $6 = $4 and 1 = 2
```

will be normalized to:
```
select where $1 in ($2 /*...*/) and $3 = $4 and $5 = $6
```

To accomplish this, we will need to track the locations of
external parameters to support the 2nd case, because we need
to re-write the original location of the parameter with
the new value. I played around with this this morning and
it works as I described above. Any concerns with the
behavior described above?

--
Sami

  52. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-27T00:58:34Z

    On Sat, May 24, 2025 at 09:35:24AM -0500, Sami Imseih wrote:
> 2. If we have 1 or more squashed lists, then we can't guarantee
> the $n parameter as was supplied by the user and we simply rename
> the $n starting from 1.
> 
> therefore, a user supplied query like this:
> ```
> select where $5 in ($1, $2, $3) and $6 = $4 and 1 = 2
> ```
> 
> will be normalized to:
> ```
> select where $1 in ($2 /*...*/) and $3 = $4 and $5 = $6
> ```
> 
> To accomplish this, we will need to track the locations of
> external parameters to support the 2nd case, because we need
> to re-write the original location of the parameter with
> the new value. I played around with this this morning and
> it works as I described above. Any concerns with the
> behavior described above?

That would be OK by me.  Not having gaps in the parameter numbers of
the normalized query just feels just like the natural thing to have in
the data reported by PGSS.
--
Michael

  53. Re: queryId constant squashing does not support prepared statements

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-05-27T15:58:57Z

    On 2025-May-24, Sami Imseih wrote:

> therefore, a user supplied query like this:
> ```
> select where $5 in ($1, $2, $3) and $6 = $4 and 1 = 2
> ```
> 
> will be normalized to:
> ```
> select where $1 in ($2 /*...*/) and $3 = $4 and $5 = $6
> ```

Hmm, interesting.

I think this renumbering should not be a problem in practice; users with
unordered parameters have little room to complain if the param numbers
change on query normalization.  At least that's how it seems to me.

If renumbering everything in physical order makes the code simpler, then
I don't disagree.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"

  54. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-27T22:05:39Z

    > > therefore, a user supplied query like this:
> > ```
> > select where $5 in ($1, $2, $3) and $6 = $4 and 1 = 2
> > ```
> >
> > will be normalized to:
> > ```
> > select where $1 in ($2 /*...*/) and $3 = $4 and $5 = $6
> > ```
>
> Hmm, interesting.
>
> I think this renumbering should not be a problem in practice; users with
> unordered parameters have little room to complain if the param numbers
> change on query normalization.  At least that's how it seems to me.
>
> If renumbering everything in physical order makes the code simpler, then
> I don't disagree.
>

It does make it simpler, otherwise we have to introduce O(n) behavior
to find eligible parameter numbers.

I've spent a bit of time looking at this, and I want to
propose the following patchset.

* 0001:

This is a normalization issue discovered when adding new
tests for squashing. This is also an issue that exists in
v17 and likely earlier versions and should probably be
backpatched.

The crux of the problem is if a constant location is
recorded multiple times, the values for $n don't take
into account the duplicate constant locations and end up
incorrectly incrementing the next value fro $n.

So, a query like

SELECT WHERE '1' IN ('2'::int, '3'::int::text)

ends up normalizing to

SELECT WHERE $1 IN ($3::int, $4::int::text)

I also added a few test cases as part of
this patch.

This does also feel like it should be backpatched.

* 0002:

Added some more tests to the ones initially proposed
by Dmitri in v3-0001 [0] including the "edge cases" which
led to the findings for 0001.


* 0003:

This fixes the normalization anomalies introduced by
62d712ec ( squashing feature ) mentioned here [1]

This patch therefore implements the fixes to track
the boundaries of an IN-list, Array expression.


* 0004: implements external parameter squashing.

While I think we should get all patches in for v18, I definitely
think we need to get the first 3 because they fix existing
bugs.

What do you think?


[0] https://www.postgresql.org/message-id/i635eozw2yjpzqxi5vgm4ceccqq3gv7ul4xj2xni2v6pfgtqlr%40vc5otquxmgjg
[1] https://www.postgresql.org/message-id/CAA5RZ0ts6zb-efiJ%2BK31Z_YDU%3DM7tHE43vv6ZBCqQxiABr3Yaw%40mail.gmail.com

--
Sami

  55. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-27T23:43:01Z

    > I've spent a bit of time looking at this, and I want to
> propose the following patchset.

Sorry about this, but I missed to add a comment in one of the
test cases for 0004 that describes the behavior of parameters
and constants that live outside of the squashed list.

The following 2 cases will result in different queryId's because
the 4th constant/parameter will be jumbled either as a type Const
or type Param.

select from tab where a in (1, 2, 3) and b = 4

select from tab where a in ($1, $2, $3) and b = $4


--
Sami

  56. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-28T03:44:49Z

    On Tue, May 27, 2025 at 05:05:39PM -0500, Sami Imseih wrote:
> * 0001:
> 
> This is a normalization issue discovered when adding new
> tests for squashing. This is also an issue that exists in
> v17 and likely earlier versions and should probably be
> backpatched.
> 
> The crux of the problem is if a constant location is
> recorded multiple times, the values for $n don't take
> into account the duplicate constant locations and end up
> incorrectly incrementing the next value from $n.
> 
> This does also feel like it should be backpatched.

Yes, this needs to be backpatched and it is actually a safe backpatch
because only the text representation is changed when adding a new
entry in the dshash; it only improves the reports without touching the
existing data.  I'm OK to take care of this one by myself, even in the
context of this thread.  It is an issue independent of what we're
discussing here for the list squashing.  As there is only one sprintf() in
generate_normalized_query() in ~17, the fix of the back-branches is
slightly simpler. 

You have mentioned the addition of tests, but v6-0001 includes nothing
of the kind.  Am I missing something?  How much coverage did you
intend to add here?  These seem to be included in squashing.sql in
patch v6-0002, but IMO this should be moved somewhere else to work
with the back-branches and make the whole backpatch story more
consistent.

> * 0002:
> 
> Added some more tests to the ones initially proposed
> by Dmitri in v3-0001 [0] including the "edge cases" which
> led to the findings for 0001.

Tests for CoerceViaIO with jsonb have been moved around.  Not a big
deal, but that makes the diffs of the patch confusing to read.

+-- if there is only one level of relabeltype, the list will be squashable

RelabelType perhaps?

A lot of the tests introduced in v6-0002 are copy-pastes of the
previous ones for IN clauses introduced for the ARRAY cases, with
comments explaining the reasons why lists are squashed or not also
copy-pasted.  Perhaps it would make sense to group the ARRAY and IN
clause cases together.  For example, group each of the two CoerceViaIO
cases together in a single query on pg_stat_statements, with a single
pg_stat_statements_reset().  That would make more difficult to miss
the fact that we need to care about IN clauses *and* arrays when
adding more test patterns, if we add some of course.

The cases where IN clauses are rewritten as ArrayExpr are OK kept at
the end.

> * 0003:
> 
> This fixes the normalization anomalies introduced by
> 62d712ec ( squashing feature ) mentioned here [1]
> 
> This patch therefore implements the fixes to track
> the boundaries of an IN-list, Array expression.

Nice simplifications in the PGSS part in terms of 

+       ListWithBoundary *n = $4;

I'd suggest to not use "n" for this one, but a different variable
name, leaving the internals for the SubLink cases minimally touched.

+typedef struct ListWithBoundary
+{
+	Node	   *expr;
+	ParseLoc	start;
+	ParseLoc	end;
+} ListWithBoundary;

Implementation-wise, I would choose a location with a query length
rather than start and end locations.  That's what we do for the nested
queries in the DMLs, so on consistency grounds..

> * 0004: implements external parameter squashing.

+static void
+_jumbleParam(JumbleState *jstate, Node *node)
+{
[...]
+	if (expr->paramkind == PARAM_EXTERN)
+	{
+		RecordExpressionLocation(jstate, expr->location, -1, true);
+
+		if (expr->paramid > jstate->highest_extern_param_id)
+			jstate->highest_extern_param_id = expr->paramid;
+	}

Using a custom implementation for Param nodes means that we are going
to apply a location record for all external parameters, not only the
ones in the lists..  Not sure if this is a good idea.  Something
smells a bit wrong with this approach.  Sorry, I cannot push my finger
on what exactly when typing this paragraph.

> While I think we should get all patches in for v18, I definitely
> think we need to get the first 3 because they fix existing
> bugs.
> 
> What do you think?

Patches 0002 and 0003 fix bugs in the squashing logic present only on
HEAD, nothing that impacts older branches already released, right?
--
Michael

  57. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-28T21:05:03Z

    >> * 0001:

> You have mentioned the addition of tests, but v6-0001 includes nothing
> of the kind.  Am I missing something?  How much coverage did you
> intend to add here?  These seem to be included in squashing.sql in
> patch v6-0002, but IMO this should be moved somewhere else to work
> with the back-branches and make the whole backpatch story more
> consistent.

That's my mistake. I added a new file called normalize.sql to test
specific normalization scenarios. Added in v7

> > * 0002:

> RelabelType perhaps?

Fixed.

> A lot of the tests introduced in v6-0002 are copy-pastes of the
> previous ones for IN clauses introduced for the ARRAY cases, with
> comments explaining the reasons why lists are squashed or not also
> copy-pasted.  Perhaps it would make sense to group the ARRAY and IN
> clause cases together.  For example, group each of the two CoerceViaIO
> cases together in a single query on pg_stat_statements, with a single
> pg_stat_statements_reset().  That would make more difficult to miss
> the fact that we need to care about IN clauses *and* arrays when
> adding more test patterns, if we add some of course.
>

I agree. I reorganized by grouping both for IN and ARRAY tests
together for a specific test area.

I also clarified some comments in the tests, etc.

> > * 0003:

> I'd suggest to not use "n" for this one, but a different variable
> name, leaving the internals for the SubLink cases minimally touched.

I agree. Fixed.

> Implementation-wise, I would choose a location with a query length
> rather than start and end locations.  That's what we do for the nested
> queries in the DMLs, so on consistency grounds..

This is different because the existing location field is tracking
something a bit different than what we want to track.

What the current location field is tracking is to assist in things
like error messages, like below, which wants to place the
caret (^) in the proper location, which is at the location of the
"IN".

```
ERROR:  operator does not exist: oid = text
LINE 1: select where 1::oid   IN (1::text, 2, 3);
                                              ^
HINT:  No operator matches the given name and argument types. You
might need to add explicit type casts.
test=#
```

What we need for squashing is to  track the start of the outer '(' and ')' of
the expression.

I could do something like fields to track list_start and list_length instead,
Will that be better to be closer in consistency?


> > * 0004: implements external parameter squashing.

> Using a custom implementation for Param nodes means that we are going
> to apply a location record for all external parameters, not only the
> ones in the lists..  Not sure if this is a good idea.  Something
> smells a bit wrong with this approach.  Sorry, I cannot push my finger
> on what exactly when typing this paragraph.

Actually, only the parameters outside of the squashed lists are
recorded. I added
a comment to make that clear. I would really want to only record parameter
locations if we know we have a squashed list, but it's impossible to
know that in
advance.

Also, the reason for a custom implementation for Param is to avoid having
to change the signature of JUMBLE_LOCATION because we have a
new bool argument to RecordExpressionLocation to set a location as an
external parameter. We will also need special handling in gen_node_support.pl
for Param to set the new argument. I was not too happy with doing that.


> Patches 0002 and 0003 fix bugs in the squashing logic present only on
> HEAD, nothing that impacts older branches already released, right?

That is correct.


-- 
Sami

  58. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-29T02:30:12Z

    On Wed, May 28, 2025 at 04:05:03PM -0500, Sami Imseih wrote:
> That's my mistake. I added a new file called normalize.sql to test
> specific normalization scenarios. Added in v7

Thanks.  I was not sure that a new file was worth having for these
tests, knowing that select.sql has similar coverage.  I have grouped
the new tests into select.sql at the end, and added a few more
scenarios for extended queries with \bindin extended.sql, where I've
reproduced the same issue while playing with external parameters.
Applied the result down to v13, down to where the problem exists for
supported branches.

I still need to review the rest of the patch series..
--
Michael

  59. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-05-30T05:03:19Z

    On Thu, May 29, 2025 at 11:30:12AM +0900, Michael Paquier wrote:
> I still need to review the rest of the patch series..

The test additions done in v7-0002 look sensible here.

--- In the following two queries the operator expressions (+) and (@) have
--- different oppno, and will be given different query_id if squashed, even though
--- the normalized query will be the same

In v7-0002, this comment is removed, but it still applies, isn't it?

+-- The casted ARRAY expressions will have the same queryId as the IN clause
+-- form of the query

Interesting distinction that explains the differences in counts.  Yes
it's a good idea to track this kind of behavior in the tests.

--- Bigint, explicit cast is not squashed
+-- Bigint, explicit cast is squashed

Seems incorrect with 0002 taken in isolation.  The last cast is still
present in the normalization.  It's not after v7-0003.

Already mentioned upthread, but applying only v7-0003 on top of
v7-0002 (not v7-0004) leads to various regression failures in dml.sql
and squashing.sql.  The failures persist with v7-0004 applied.  Please
see these as per the attached, the IN lists do not get squashed, the
array elements are.  Just to make sure that I am not missing
something, I've rebuilt from scratch with no success.

IsSquashableExpressionList() includes this comment, which is outdated,
probably because squashing was originally optional behind a GUC and
the parameter has been removed while the comment has not been
refreshed:
    /*
     * If squashing is disabled, or the list is too short, we don't try to
     * squash it.
     */

RecordExpressionLocation()'s top comment needs a refresh, talking
about constants.  The simplifications gained in pgss.c's normalization
are pretty cool.

+   bool        has_squashed_lists;
[...]
+   if (jstate->has_squashed_lists)
+       jstate->highest_extern_param_id = 0;

This new flag in JumbleState needs to be documented, explaining why
it needs to be here.  I have to admit that it is strange to see
highest_extern_param_id, one value in JumbleState be forced to zero in
the PGSS normalization code if has_squashed_lists is set to true.
This seems like a layer violation to me: JumbleState should only be
set while in the jumbling code, not forced to something else
afterwards while in the extension.
--
Michael

  60. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-05-30T18:27:27Z

    > The test additions done in v7-0002 look sensible here.
>
> --- In the following two queries the operator expressions (+) and (@) have
> --- different oppno, and will be given different query_id if squashed, even though
> --- the normalized query will be the same
>
> In v7-0002, this comment is removed, but it still applies, isn't it?

No, the comment is wrong/misleading even in HEAD. The output looks
like the below so, the normalized query will not be the same and the
queries will also not be squashed.

```
SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
                                               query
                             | calls
----------------------------------------------------------------------------------------------------+-------
 SELECT * FROM test_squash WHERE id IN
                            +|     2
         ($1 + $2, $3 + $4, $5 + $6, $7 + $8, $9 + $10, $11 + $12, $13
+ $14, $15 + $16, $17 + $18) |
 SELECT * FROM test_squash WHERE id IN
                            +|     2
         (@ $1, @ $2, @ $3, @ $4, @ $5, @ $6, @ $7, @ $8, @ $9)
                             |
 SELECT pg_stat_statements_reset() IS NOT NULL AS t
                             |     1
(3 rows)
```

so I felt a much simpler and more appropriate comment is

```
-- No constants squashing for OpExpr
```


> --- Bigint, explicit cast is not squashed
> +-- Bigint, explicit cast is squashed
>
> Seems incorrect with 0002 taken in isolation.  The last cast is still
> present in the normalization.  It's not after v7-0003.

in 0002, this is still a squashed string even if the "::bigint" still appears at
the end of the string. Squashing is replacing the elements with
 "$1 /*, ... */"
```
----------------------------------------------------+-------
 SELECT * FROM test_squash_bigint WHERE data IN    +|     2
         ($1 /*, ... */::bigint)
```
0003 improves/fixes this by truly squashing between the entire
boundary of the list.

> Already mentioned upthread, but applying only v7-0003 on top of
> v7-0002 (not v7-0004) leads to various regression failures in dml.sql
> and squashing.sql.  The failures persist with v7-0004 applied.  Please
> see these as per the attached, the IN lists do not get squashed, the
> array elements are.  Just to make sure that I am not missing
> something, I've rebuilt from scratch with no success.

I cannot reproduce this. I applied each patch (v7-0002, 0003)
in order and ran "make check" on pg_stat_statements for every apply,
and I could not reproduce. Not sure what you and I are doing different?

I also could not reproduce with the v8 patch series either.

> IsSquashableExpressionList() includes this comment, which is outdated,
> probably because squashing was originally optional behind a GUC and
> the parameter has been removed while the comment has not been
> refreshed:
>     /*
>      * If squashing is disabled, or the list is too short, we don't try to
>      * squash it.
>      */

Thanks for reminding me about this. I remember seeing it, but missed
fixing it. Corrected now.

> RecordExpressionLocation()'s top comment needs a refresh, talking
> about constants.  The simplifications gained in pgss.c's normalization
> are pretty cool.

Yes, missed this also. Done.

> +   bool        has_squashed_lists;
> [...]
> +   if (jstate->has_squashed_lists)
> +       jstate->highest_extern_param_id = 0;
>
> This new flag in JumbleState needs to be documented, explaining why
> it needs to be here.

Done. I felt that combining highest_extern_param_id and
has_squashed_lists in the
same comment made the most sense, as they are closely related.

-    /* highest Param id we've seen, in order to start normalization
correctly */
+    /*
+     * Highest Param id we've seen, in order to start normalization correctly.
+     * However, if the jumble contains at least one squashed list, we
+     * disregard the highest_extern_param_id value because parameters can
+     * exist within the squashed list and are no longer considered for
+     * normalization.
+     */
     int            highest_extern_param_id;
+    bool        has_squashed_lists;


> I have to admit that it is strange to see
> highest_extern_param_id, one value in JumbleState be forced to zero in
> the PGSS normalization code if has_squashed_lists is set to true.
> This seems like a layer violation to me

Yeah, that's silly of me. This should be done in DoJumble after
_jumbleNode. Fixed.

I also reorganized the tests in extended.out to make them more readable,
namely I wanted to show separate outputs for what is tested
for "-- Unique query IDs with parameter numbers switched." and what is tested
for "-- Two groups of two queries with the same query ID."

I also added a comment for
``
bool extern_param;
```

v8 addresses the above.


--
Sami Imseih
Amazon Web Services (AWS)

  61. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-06-05T01:44:12Z

    I realized this thread did not have a CF entry,
so here it is https://commitfest.postgresql.org/patch/5801/

--
Sami

  62. Re: queryId constant squashing does not support prepared statements

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-06-09T10:44:59Z

    Hello,

I've spent a bunch of time looking at this series and here's my take on
the second one.  (The testing patch is unchanged from Sami's).  The
third patch (for PARAM_EXTERNs) should be a mostly trivial rebase on top
of these two.

I realized that the whole in_expr production in gram.y is pointless, and
the whole private struct that was added was unnecessary.  A much simpler
solution is to remove in_expr, expand its use in a_expr to the two
possibilities, and with that we can remove the need for a new struct.

I also added a recursive call in IsSquashableExpression to itself.  The
check for stack depth can be done without throwing an error.  I tested
this by adding stack bloat in that function.  I also renamed it to
IsSquashableConstant.  This changes one of the tests, because a cast
sequence like 42::int::bigint::int is considered squashable.

Other than that, the changes are cosmetic.

Barring objections, I'll push this soon, then look at rebasing 0003 on
top, which I expect to be an easy job.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  63. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-06-09T15:09:00Z

    > I've spent a bunch of time looking at this series and here's my take on
> the second one.

Thanks!

> I realized that the whole in_expr production in gram.y is pointless, and
> the whole private struct that was added was unnecessary.  A much simpler
> solution is to remove in_expr, expand its use in a_expr to the two
> possibilities, and with that we can remove the need for a new struct.

Nice simplification.

> I also added a recursive call in IsSquashableExpression to itself.  The

I agree with this. I was thinking about a follow-up patch for this based on
the discussion above, but why not just add it now.

> Barring objections, I'll push this soon, then look at rebasing 0003 on
> top, which I expect to be an easy job.

LGTM.

--
Sami

  64. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-06-10T06:37:18Z

    On Mon, Jun 09, 2025 at 12:44:59PM +0200, Alvaro Herrera wrote:
> I also added a recursive call in IsSquashableExpression to itself.  The
> check for stack depth can be done without throwing an error.  I tested
> this by adding stack bloat in that function.  I also renamed it to
> IsSquashableConstant.  This changes one of the tests, because a cast
> sequence like 42::int::bigint::int is considered squashable.
> 
> Other than that, the changes are cosmetic.
> 
> Barring objections, I'll push this soon, then look at rebasing 0003 on
> top, which I expect to be an easy job.

v9-0002 is failing in the CI for the freebsd task:
https://github.com/michaelpq/postgres/runs/43784034162

Here is the link to the diffs, also attached to this message:
https://api.cirrus-ci.com/v1/artifact/task/5378459897167872/testrun/build/testrun/pg_stat_statements/regress/regression.diffs

I am also able to reproduce these failures locally, FWIW.  For
example, with a IN clause made of integer constants gets converted to
an ArrayExpr, but in _jumbleElements() we fail to call
RecordConstLocation() and the list is not squashed.

I think that this is can be reproduced by
-DWRITE_READ_PARSE_PLAN_TREES -DCOPY_PARSE_PLAN_TREES
-DRAW_EXPRESSION_COVERAGE_TEST that I always include in my builds.
The freebsd task uses the same with debug_copy_parse_plan_trees=on,
debug_write_read_parse_plan_trees=on and
debug_raw_expression_coverage_test=on.
--
Michael

  65. Re: queryId constant squashing does not support prepared statements

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-06-10T17:25:27Z

    On 2025-Jun-10, Michael Paquier wrote:

> I think that this is can be reproduced by
> -DWRITE_READ_PARSE_PLAN_TREES -DCOPY_PARSE_PLAN_TREES
> -DRAW_EXPRESSION_COVERAGE_TEST that I always include in my builds.
> The freebsd task uses the same with debug_copy_parse_plan_trees=on,
> debug_write_read_parse_plan_trees=on and
> debug_raw_expression_coverage_test=on.

Ah, of course, I forgot to add read/write support for A_Expr.  Pushed a
new one, running at
https://cirrus-ci.com/build/6249249819590656

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"We’ve narrowed the problem down to the customer’s pants being in a situation
 of vigorous combustion" (Robert Haas, Postgres expert extraordinaire)

  66. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-06-11T00:32:27Z

    On Tue, Jun 10, 2025 at 07:25:27PM +0200, Alvaro Herrera wrote:
> On 2025-Jun-10, Michael Paquier wrote:
>> I think that this is can be reproduced by
>> -DWRITE_READ_PARSE_PLAN_TREES -DCOPY_PARSE_PLAN_TREES
>> -DRAW_EXPRESSION_COVERAGE_TEST that I always include in my builds.
>> The freebsd task uses the same with debug_copy_parse_plan_trees=on,
>> debug_write_read_parse_plan_trees=on and
>> debug_raw_expression_coverage_test=on.
> 
> Ah, of course, I forgot to add read/write support for A_Expr.  Pushed a
> new one, running at
> https://cirrus-ci.com/build/6249249819590656

Ah, right.  I completely forgot that we have a custom read/write
function for this node.  Yes, things should be OK once the function is
updated.  Thanks.
--
Michael

  67. Re: queryId constant squashing does not support prepared statements

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-06-12T16:32:38Z

    Hello,

I have pushed that now, and here's a rebase of patch 0003 to add support
for PARAM_EXTERN.  I'm not really sure about this one yet ...

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La victoria es para quien se atreve a estar solo"

  68. Re: queryId constant squashing does not support prepared statements

    Sami Imseih <samimseih@gmail.com> — 2025-06-12T18:44:21Z

    On Thu, Jun 12, 2025 at 11:32 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> Hello,
>
> I have pushed that now,

thanks!

> and here's a rebase of patch 0003 to add support
> for PARAM_EXTERN.  I'm not really sure about this one yet ...

see v11. I added a missing test to show how external param
normalization behaves without a squashed list vs with.

Also, improved some of the code comments for additional
clarity.

--
Sami

  69. Re: queryId constant squashing does not support prepared statements

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-06-24T17:45:15Z

    Hello

I spent a much longer time staring at this patch than I wanted to, and
at a point I almost wanted to boot the whole thing to pg19, but because
upthread we already had an agreement that we should get it in for this
cycle, I decided that the best course of action was to just move forward
with it.

My reluctance mostly comes from this bit in generate_normalized_query:

+       /*
+        * If we have an external param at this location, but no lists are
+        * being squashed across the query, then we skip here; this will make
+        * us print print the characters found in the original query that
+        * represent the parameter in the next iteration (or after the loop is
+        * done), which is a bit odd but seems to work okay in most cases.
+        */
+       if (jstate->clocations[i].extern_param && !jstate->has_squashed_lists)
+           continue;

Sami's patch didn't have a comment here and it was not immediately
obvious what was going on; moreover I was quite surprised at what
happened if I removed it: for example, one query text (in test
level_tracking.sql) changes from

-     2 |    2 | SELECT (i + $2)::INTEGER LIMIT $3

into

+     2 |    2 | SELECT ($2 + $3)::INTEGER LIMIT $4

and if I understand this correctly, the reason is that the query is
being executed from an SQL function,

CREATE FUNCTION PLUS_ONE(i INTEGER) RETURNS INTEGER AS
$$ SELECT (i + 1.0)::INTEGER LIMIT 1 $$ LANGUAGE SQL;

so the 'i' is actually a parameter, so it makes sense that it gets
turned into a parameter in the normalized query.  With the 'if' test I
mentioned above, we print it as 'i' literally only because we
'continued' and the next time through the loop we print the text from
the original query.  This may be considered not entirely correct ...
note that the constants in the query are shown as parameters, and that
the numbers do not start from 1.  (Obviously, a few other queries also
change.)


I decided to move forward with it anyway because this weirdness seems
more contained and less damaging than the unhelpfulness of the unpatched
behavior.  We may want to revisit this in pg19 -- or, if we're really
unconfortable with this, we could even decide to revert this commit in
pg18 and try again with Param support in pg19.  But I'd like to move on
from this and my judgement was that the situation with patch is better
than without.


I added one more commit, which iterates to peel as many layers of
CoerceViaIO and RelabelType as there are around an expression.  I
noticed this lack while reading Sami's patch for that function, which
just simplified the coding therein.  For normal cases this would iterate
just once (because repeated layers of casts are likely very unusual), so
I think it's okay to do that.  There was some discussion about handling
this via recursion but it died inconclusively; I added recursive
handling of FuncExpr's arguments in 0f65f3eec478, which was different,
but I think handling this case by iterating is better than recursing.

With these commits, IMO the open item can now be closed.  Even if we
ultimately end up reverting any of this, we would probably punt support
of Params to pg19, so the open item would be gone anyway.


Lastly, I decided not to do a catversion bump.  As far as I can tell,
changes in the jumbling functions do not need them.  I tried an
'installcheck' run with a datadir initdb'd with the original code, and
it works fine.  I also tried an 'installcheck' with pg_stat_statements
installed, and was surprised to realize that the Query Id reports in
EXPLAIN make a large number of tests fail.  If I take those lines from
the original code into the expected output, and then run the tests with
the new code, I notice that a few queries have changed queryId.  I
suppose this was to be expected, and shouldn't harm anything.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"It takes less than 2 seconds to get to 78% complete; that's a good sign.
A few seconds later it's at 90%, but it seems to have stuck there.  Did
somebody make percentages logarithmic while I wasn't looking?"
                http://smylers.hates-software.com/2005/09/08/1995c749.html

  70. Re: queryId constant squashing does not support prepared statements

    Michael Paquier <michael@paquier.xyz> — 2025-06-25T04:18:49Z

    On Tue, Jun 24, 2025 at 07:45:15PM +0200, Alvaro Herrera wrote:
> +       /*
> +        * If we have an external param at this location, but no lists are
> +        * being squashed across the query, then we skip here; this will make
> +        * us print print the characters found in the original query that
> +        * represent the parameter in the next iteration (or after the loop is
> +        * done), which is a bit odd but seems to work okay in most cases.
> +        */
> +       if (jstate->clocations[i].extern_param && !jstate->has_squashed_lists)
> +           continue;

+        * us print print the characters found in the original query that

The final commit includes this comment, with a s/print print/print/
required.

> and if I understand this correctly, the reason is that the query is
> being executed from an SQL function,
> 
> CREATE FUNCTION PLUS_ONE(i INTEGER) RETURNS INTEGER AS
> $$ SELECT (i + 1.0)::INTEGER LIMIT 1 $$ LANGUAGE SQL;

Right, with two executions of PLUS_ONE() making it a single entry with
calls=2 .

> so the 'i' is actually a parameter, so it makes sense that it gets
> turned into a parameter in the normalized query.  With the 'if' test I
> mentioned above, we print it as 'i' literally only because we
> 'continued' and the next time through the loop we print the text from
> the original query.  This may be considered not entirely correct ...
> note that the constants in the query are shown as parameters, and that
> the numbers do not start from 1.  (Obviously, a few other queries also
> change.)

What you have committed is also consistent with the decision in v17
and older branches.  The current result looks OK to me for v18.

> I added one more commit, which iterates to peel as many layers of
> CoerceViaIO and RelabelType as there are around an expression.  I
> noticed this lack while reading Sami's patch for that function, which
> just simplified the coding therein.  For normal cases this would iterate
> just once (because repeated layers of casts are likely very unusual), so
> I think it's okay to do that.  There was some discussion about handling
> this via recursion but it died inconclusively; I added recursive
> handling of FuncExpr's arguments in 0f65f3eec478, which was different,
> but I think handling this case by iterating is better than recursing.

Agreed.  I was actually wondering about the logic of
IsSquashableConstant() when it came to RelabelType and CoerceViaIO.
The order of the scans was expected but just going back to the top of
IsSquashableConstant() for these two nodes makes the code easier to
follow.  So agreed that your change is an improvement.

> With these commits, IMO the open item can now be closed.  Even if we
> ultimately end up reverting any of this, we would probably punt support
> of Params to pg19, so the open item would be gone anyway.

Yes.

> Lastly, I decided not to do a catversion bump.  As far as I can tell,
> changes in the jumbling functions do not need them.  I tried an
> 'installcheck' run with a datadir initdb'd with the original code, and
> it works fine.

This reminds me of 4c7cd07aa62a and this thread:
https://www.postgresql.org/message-id/1364409.1727673407@sss.pgh.pa.us

Doesn't the change in the Param structure actually require one because
it can change the representation of some SQL functions?  I am not
completely sure.

> I also tried an 'installcheck' with pg_stat_statements
> installed, and was surprised to realize that the Query Id reports in
> EXPLAIN make a large number of tests fail.  If I take those lines from
> the original code into the expected output, and then run the tests with
> the new code, I notice that a few queries have changed queryId.  I
> suppose this was to be expected, and shouldn't harm anything.

I don't think we've put a lot of work in trying to make installcheck
work with PGSS (never tried it TBH), so I am not surprised to see some
failures when one tries this mode.
--
Michael

  71. Re: queryId constant squashing does not support prepared statements

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-06-26T16:42:46Z

    On 2025-Jun-25, Michael Paquier wrote:

> On Tue, Jun 24, 2025 at 07:45:15PM +0200, Alvaro Herrera wrote:
> > +       /*
> > +        * If we have an external param at this location, but no lists are
> > +        * being squashed across the query, then we skip here; this will make
> > +        * us print print the characters found in the original query that
> > +        * represent the parameter in the next iteration (or after the loop is
> > +        * done), which is a bit odd but seems to work okay in most cases.
> > +        */
> > +       if (jstate->clocations[i].extern_param && !jstate->has_squashed_lists)
> > +           continue;
> 
> +        * us print print the characters found in the original query that
> 
> The final commit includes this comment, with a s/print print/print/
> required.

Ugh.  Fixed, thanks for noticing that.

> > Lastly, I decided not to do a catversion bump.  As far as I can tell,
> > changes in the jumbling functions do not need them.  I tried an
> > 'installcheck' run with a datadir initdb'd with the original code, and
> > it works fine.
> 
> This reminds me of 4c7cd07aa62a and this thread:
> https://www.postgresql.org/message-id/1364409.1727673407@sss.pgh.pa.us
> 
> Doesn't the change in the Param structure actually require one because
> it can change the representation of some SQL functions?  I am not
> completely sure.

Hmm, but the Param structure didn't actually change; only its jumbling
function did (and others that rely on LocationLen).  So I think what
could happen here if there's no catversion is that somebody has a
pg_stat_statements populated with query Ids for some queries that have a
different queryIds when computed with the new code.  So they're going to
have duplicates in pg_stat_statements.  I think this is a pretty minor
problem, so I'm not inclined to do a catversion bump for it.

Anyway we have one due to 0cd69b3d7ef3, so it's moot now.  (But it's a
good discussion to have, for the future.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Niemand ist mehr Sklave, als der sich für frei hält, ohne es zu sein."
       Nadie está tan esclavizado como el que se cree libre no siéndolo
                                           (Johann Wolfgang von Goethe)