Re: queryId constant squashing does not support prepared statements

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