Re: pg_stat_statements and "IN" conditions

> > > Since the merging is a yes/no option (I think there used to be some discussions
> > > about having a threshold or some other fancy modes), maybe you could instead
> > > differentiate the merged version by have 2 constants rather than this "..." or
> > > something like that?
> > >
> > Maybe the representation can be "($1 /*, ... */)" so that it's obvious
> > that the array extends beyond the first element but is still
> > syntactically valid.

> Yeah that works too and it's probably way easier to implement.

+1

Just to throw out an alternate idea using comments. What about
adding a comment at the start to the query "/* query_id_squash_values */"
and keep the parameter symbols as-is. The comment at the start will
indicate that this feature was used.

> > On Thu, Feb 13, 2025 at 05:08:45PM GMT, Sami Imseih wrote:
> > Constants passed as parameters to a prepared statement will not be
> > handled as expected. I did not not test explicit PREPARE/EXECUTE statement,
> > but I assume it will have the same issue.

> This is the same question of supporting various cases. The original
> patch implementation handled Param expressions as well, this part was
> explicitly rejected during review. I think as a first step it's
> important to find a balance between applying this optimization in as
> many cases as possible, and at the same time keep the implementation
> simple to give the patch a chance. So far I'm inclined to leave Param
> for the future work, although of course I'm open to discussion.

I do see the discussion here [1], sorry for not noticing it.

I am not sure about this though. At minimum this needs to be documented,
However, I think the  prepared statement case is too common of a case to
skip for the first release of tis feature, and users that will likely
benefit from this feature are using prepared statements ( i.e. JDBC, etc ).

But, others may disagree.

> > pg_node_attr of query_jumble_merge is doing something
> > very specific to the elements list of an ArrayExpr. The
> > merge code likely cannot be used for other node types.

> It can be, take a look at pg_node_attr commentary. Any node can have a
> field marked with query_jumble_merge attribut and benefit from merging.

I can't think of other cases beyond ArrayExpr where this will be needed.
The node that could use this will need to carry constants, but ArrayExpr
is the only case I can think of in which this will be useful for jumbling.
There should be a really good reason IMO to do something other than the
existing pattern of using custom_query_jumble.

I scanned through the thread and could not find a discussion on this,
but maybe others have an opinion.

> > This case with an array passed to aa function seems to cause a regression
> > in pg_stat_statements query text. As you can see the text is incomplete.

> I've already mentioned that in the previous email. To reiterate, it's
> not a functionality regression, but an incomplete representation of a
> normalized query which turned out to be hard to change. While I'm
> working on that, there is a suggestion that it's not a blocker.

It's not a functionality regression as far as query execution
or pg_stat_statements counters go, but it is a regression as far as
displaying query text in pg_stat_statements. pg_stat_statements, unlike
pg_stat_acitivty, makes a guaranteee not to trim text as stated in the docs [2]
"The representative query texts are kept in an external disk file,
and do not consume shared memory. Therefore,
even very lengthy query texts can be stored successfully."

I don't think any feature that trims text in pg_stat_statements is acceptable,
IMO. Others may disagree.

Regards,

Sami


[1] https://www.postgresql.org/message-id/20230211104707.grsicemegr7d3mgh@erthalion.local
[2] https://www.postgresql.org/docs/current/pgstatstatements.html