Horton

Re: queryId constant squashing does not support prepared statements

Michael Paquier <michael@paquier.xyz>

View thread
From: Michael Paquier <michael@paquier.xyz>
To: Sami Imseih <samimseih@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2025-05-01T00:29:13Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix typo in comment

  2. Make query jumbling also squash PARAM_EXTERN params

  3. Fix squashing algorithm for query texts

  4. pg_stat_statements: Fix parameter number gaps in normalized queries

On Wed, Apr 30, 2025 at 04:52:06PM -0500, Sami Imseih wrote:
> 62d712ec added the ability to squash constants from an IN LIST
> for queryId computation purposes. This means that a similar
> queryId will be generated for the same queries that only
> different on the number of values in the IN-LIST.
> 
> The patch lacks the ability to apply this optimization to values
> passed in as parameters ( i.e. parameter kind = PARAM_EXTERN )
> which will be the case for SQL prepared statements and protocol level
> prepared statements, i.e.
> 
> I think this is a pretty big gap as many of the common drivers such as JDBC,
> which use extended query protocol, will not be able to take advantage of
> the optimization in 18, which will be very disappointing.
> 
> Thoughts?

Yes.  Long IN/ANY clauses are as far as a more common pattern caused
by ORMs, and I'd like to think that application developers would not
hardcode such clauses in their right minds (well, okay, I'm likely
wrong about this assumption, feel free to counter-argue).  These also 
like relying on the extended query protocol.  Not taking into account
JDBC in that is a bummer, and it is very popular.

I agree that the current solution we have in the tree feels incomplete
because we are not taking into account the most common cases that
users would care about.  Now, allowing PARAM_EXTERN means that we
allow any expression to be detected as a squashable thing, and this
kinds of breaks the assumption of IsSquashableConst() where we want
only constants to be allowed because EXECUTE parameters can be any
kind of Expr nodes.  At least that's the intention of the code on
HEAD.

Now, I am not actually objecting about PARAM_EXTERN included or not if
there's a consensus behind it and my arguments are considered as not
relevant.  The patch is written so as it claims that a PARAM_EXTERN
implies the expression to be a Const, but it may not be so depending
on what the execution path is given for the parameter.  Or at least
the patch could be clearer and rename the parts about the "Const"
squashable APIs around queryjumblefuncs.c.

To be honest, the situation of HEAD makes me question whether we are
using the right approach for this facility.  I did mention a couple of
months ago about an alternative, but it comes down to accept that any
expressions would be normalized, unfortunately I never got down to
study it in details as this touches around expr_list in the parser: we
could detect in the parser the start and end locations of an
expression list in a query string, then group all of them together
based on their location in the string.  This would be also cheaper
than going through all the elements in the list, tweaking things when
dealing with a subquery..

The PARAM_EXTERN part has been mentioned a couple of weeks ago here,
btw:
https://www.postgresql.org/message-id/CAA5RZ0tu6_KRiYJCFptf4_--wjFSu9cZMj1XNmOCqTNxu=VpEA@mail.gmail.com
--
Michael