Re: pg_stat_statements and "IN" conditions

> On Fri, Feb 14, 2025 at 05:26:19AM GMT, Sami Imseih wrote:
> > > 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 ).

Right, prepared statements are quite common case. This would be the
first thing I'll take on in the case if this patch will find it's way
into the release. As you can see it's not at all obvious that that will
happen, I estimate chances for that to be higher if moving in smaller
steps.

> > > 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.

Well, there are plenty expression nodes that have lists in them, maybe
more will be added in the future. And as before, the idea of using
pg_node_attr was a resonable suggestion from Michael Paquier on top of
the original design (which indeed used custom jumble function for
ArrayExpr).

> 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."

Just to clarify, the part you reference doesn't say anything about
trimming, doesn't it? In fact, the query text stored in
pg_stat_statements might be as well somewhat different from one that was
executed, due to similar queries having the same query_id and differ
only in e.g. parenthesis.

But in any case, you're right that the original thing was a bug. I
didn't realize you're talking about missing chunk of the normalized
query. The issue could be triggered when having multiple merged
intervals withing the same query.

Btw, there was another mistake in the last version introducing
"$1 /*, ... */" format, the constant position has to be of course
calculated as usual.