Re: Bug in pg_stat_statements

On 2025-Oct-28, Michael Paquier wrote:

> FWIW, the line defining the separation between pg_stat_statements.c
> and queryjumblefuncs.c should rely on a pretty simple concept:
> JumbleState can be written only by queryjumblefuncs.c and
> src/backend/nodes/, and should remain in an untouched constant state
> when looked at by any external module loaded, included PGSS, because
> it could be shared across more than one paths.

I can get behind this as a principle.

> That's just to say that I can get behind the argument you are
> presenting in patch 0002, to move the updates of
> JumbleState.clocations[N].length to happen inside the query jumbling
> code.  This relies on a query string anyway, which would be the same
> for all the modules interested in interacting in a planner, analyze or
> execution hook.

Hmm, yeah, now that you mention this, it seems rather odd that
pgss_store() is messing with (modifying) the jstate it is given.  If we
had multiple modules using a planner_hook to interact with the query
representation, what each module sees would be different depending on
which hook is called first, which sounds wrong.  Maybe, as you say, we
do need to consider this a design flaw that should be fixed in a more
principled fashion ... and it's pretty hard to see that we could
backpatch any such fixes.  It's a pretty old issue though (you probably
notice I hesitate to call it a bug.)  So I agree with you: we should fix
the specific bug first across branches, and the reworking of the
jumbling framework should be done afterwards in master only.

I'm going to get the first patch pushed, after looking at it more
carefully.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Update: super-fast reaction on the Postgres bugs mailing list. The report
was acknowledged [...], and a fix is under discussion.
The wonders of open-source !"
             https://twitter.com/gunnarmorling/status/1596080409259003906