Re: query_id, pg_stat_activity, extended query protocol

On Wed, Sep 11, 2024 at 09:41:58PM -0500, Sami Imseih wrote:
>> The tests in pg_stat_statements are one part I'm pretty sure is one
>> good way forward. It is not perfect, but with the psql meta-commands
>
> I played around with BackgrounsPsql. It works and gives us more flexibility
> in testing, but I think the pg_stat_statements test are good enough for this
> purpose.
>
> My only concern is this approach tests core functionality ( reporting of queryId )
> in the tests of a contrib module ( pg_stat_statements ). Is that a valid
> concern?

Do you think that we'd better replace the calls reporting the query ID
in execMain.c by some assertions on HEAD? This won't work for
ExecutorStart() because PREPARE statements (or actually EXECUTE,
e.g. I bumped on that yesterday but I don't recall which one) would
blow up on that with compute_query_id enabled. We could do something
like that in ExecutorRun() at least as that may be helpful for
extensions? An assertion would be like:
Assert(!IsQueryIdEnabled() || pgstat_get_my_query_id() != 0);

ExecutorFinish() and ExecutorEnd() are not that mandatory, so there's
a risk that this causes the backend to complain because a planner or
post-analyze hook decides to force the hand of the backend entry in an
extension. With such checks, we're telling them to just not do that.
So your point would be to force this rule within the core executor on
HEAD? I would not object to that in case we're missing more spots
with the extended query protocol, actually. That would help us detect
cases where we're still missing the query ID to be set and the
executor should know about that. The execute/fetch has been missing
for years without us being able to detect it automatically.

Note that I'm not much worried about the dependency with
pg_stat_statements. We already rely on it for query jumbling
normalization for some parse node patterns like DISCARD, and query
jumbling requires query IDs to be around. So that's not new.
--
Michael