Re: query_id, pg_stat_activity, extended query protocol

On Tue, Oct 01, 2024 at 10:00:00PM +0300, Alexander Lakhin wrote:
> Hello Michael,
>
> 01.10.2024 05:07, Michael Paquier wrote:
> > On Mon, Sep 30, 2024 at 10:07:55AM +0900, Michael Paquier wrote:
> > ...
> > And done this part.
>
> If I'm not missing something, all the patches discussed here are committed
> now, so maybe I've encountered a new anomaly.
>
> Please try the following script:
> BEGIN;
> PREPARE s AS SELECT 1;
> SELECT $1 \bind 1 \g
> EXECUTE s;
>
> It produces for me:
> TRAP: failed Assert("!IsQueryIdEnabled() || !pgstat_track_activities ||
> !debug_query_string || pgstat_get_my_query_id() != 0"), File: "execMain.c",
> Line: 423, PID: 1296466

The failure would appear only with pg_stat_statements loaded, not with
compute_query_id enabled while pgss is not loaded. The difference is
explained by pgss_post_parse_analyze() where the query ID is reset for
an ExecuteStmt.

The \bind followed by the EXECUTE is at fault here. I thought first
that this was some manipulation related to unnamed portals, because
PREPARE followed by EXECUTE would assign the ID from the PREPARE in
the EXECUTE command when it reaches ExecutorFinish(). However,
\bind_named is able to show the same problem, like:
SELECT 2 \parse stmt1
begin;
PREPARE s AS SELECT 1;
\bind_named stmt1 \g
EXECUTE s; -- query ID 0

And that's when I noticed that this is only caused by the fact that we
would go through ExecuteEnd() and ExecuteFinish() while doing a
cleanup of the portal created for \bind[_named]. The query ID is
cleaned up first, then the executor end/finish paths are called. This
requires also pg_stat_statements.track_utility to be enabled and a
transaction block.

This proves that the two assertions within ExecutorFinish() and
ExecutorEnd() are a bad idea, as they depend on the code paths where
an active portal is going to be removed. That leaves the one in
ExecutorRun(), lowering the whole value of the check quite a bit. So
perhaps it is just better to let that go entirely and finish this
experiment?

Alexander, I've thought about a couple of fancy cases for
ExecutorRun() but I could not break it. Perhaps you have something in
your sleeve that would also break this case?
--
Michael