Re: queryId constant squashing does not support prepared statements

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