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