Feedback-ID: ia2694551:Fastmail
Date: Wed, 12 Feb 2025 13:57:47 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Sami Imseih <samimseih@gmail.com>, 
	Michael Paquier <michael@paquier.xyz>, Lukas Fittl <lukas@fittl.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Marko M <marko@pganalyze.com>
Subject: Re: [PATCH] Optionally record Plan IDs to track plan changes for a
 query
Message-ID: <202502121257.ev3s2uy4h4ym@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <Z6wal4gWT4NW6sJv@jrouhaud>
Archived-At: <https://www.postgresql.org/message-id/202502121257.ev3s2uy4h4ym%40alvherre.pgsql>
Precedence: bulk

On 2025-Feb-12, Julien Rouhaud wrote:

> > FWIW, I think options to tweak queryId computation is something that
> > should be in core. It was discussed earlier in the context of IN
> > list merging; the patch for this currently has the guc for the
> > feature in pg_stat_statements, but there was a discussion about
> > actually moving this to core [1] Giving the user a way to control
> > certain behavior about the queryId computation is a good thing to do
> > in core; especially queryId is no longer just consumed in
> > pg_stat_statements. Maybe the right answer is an enum GUC, not sure
> > yet.

> Well, the ability for extensions to override the actual queryid
> calculation was the result of more than half a decade of strong
> disagreements about it.   And I'm definitely not volunteering to
> reopen that topic :)

Sorry, Michael already did.

Anyway, I think that's different.  We do support compute_query_id=off as
a way for a custom module to compute completely different query IDs
using their own algorithm, which I think is what you're referring to.
However, the ability to affect the way the in-core algorithm works is a
different thing: you still want in-core code to compute the query ID.

Right now, the proposal in the other thread is that if you want to
affect that algorithm in order to merge arrays to be considered a single
query element regardless of its length, you set the GUC for that.
Initially the GUC was in the core code.  Then, based on review, the GUC
was moved to the extension, _BUT_ the implementation was still in the
core code: in order to activate it, the extension calls a function that
modifies core code behavior.  So there are more moving parts than
before, and if you for whatever reason want that behavior but not the
extension, then you need to write a C function.  To me this is absurd.
So what I suggest we do is return to having the GUC in the core code.

Now I admit I'm not sure what the solution would be for the problem
discussed in this subthread.  Apparently the problem is related to temp
tables and their changing OIDs.  I'm not sure what exactly the proposal
for a GUC is.  I mean, what would the behavior change be?  Maybe what
you want is something like "if this table reference here is to a temp
table, then instead of jumbling the OID then jumble the string
'pg_temp.tablename' instead", which would make the query ID be the same
for all occurrences of that query in whatever backend return the same
number, regardless both of what OID the temp schema for that backend is,
and the table OID itself.  Is there more to it than that?  (The only
difficulty I see here is how to get the table name when the only thing
you have is the RangeTblEntry, which doesn't have the name but just the
OID.  I see in [1] that you simply do a syscache lookup, but it would be
good to avoid that.)

Maybe that sounds pretty obscure if you try to describe it too
precisely, but if you don't think too hard about it it probably natural
-- at least to me.  So my next question is, do we really need this
behavior to be configurable?  Wouldn't it be better to make the default
way to deal with temp tables in all cases?  The current behavior seems
rather unhelpful.  I do note that what you do in pg_queryid, which is
simply to ignore the table altogether, is probably not a great idea.


Anyway, assuming we make a GUC of it (a big if!), let me talk a bit
about GUC names.  In the other thread, the list of GUC names in the
submitted patch plus the ones I suggested are:

query_id_const_merge
query_id_merge_values
query_id_merge_value_lists
query_id_squash_constant_lists

so maybe here I would consider something like

query_id_merge_temp_tables
query_id_squash_temporary_tables

[1] https://github.com/rjuju/pg_queryid/blob/master/pg_queryid.c#L941

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The important things in the world are problems with society that we don't
understand at all. The machines will become more complicated but they won't
be more complicated than the societies that run them."    (Freeman Dyson)