Re: query_id, pg_stat_activity, extended query protocol

On Mon, Sep 02, 2024 at 10:11:43AM +0900, Michael Paquier wrote:
> I need to spend a bit more time with my head down for this thread, but
> couldn't we use these commands with various query patterns in
> pg_stat_statements and look at the shmem counters reported through its
> view?

My apologies for the time it took, but here you go with a patch set.

I have looked at this thread overall, and there are two problems at
hand regarding the lack of reporting of the query ID in backend
entries for the extended query protocol:
1) ExecutorRun() misses the reports, which happens when a query
does an ExecutorStart(), then a series of ExecutorRun() through a
portal with bind messages. Robert has mentioned that separately a few
days ago at [1]. But that's not everything.
2) A query executed through a portal with tuples to return in a
tuplestore also miss the query ID report. For example, a DML
RETURNING with the extended protocol would use an execute (with
ExecutorStart and ExecutorRun) followed by a series of execute fetch.
pg_stat_activity would report the query ID for the execute, not for
the fetches, while pg_stat_activity has the query string. That's
confusing.

The patch series attached address these two in 0001 and 0003. 0001
should be backpatched (still need to wordsmith the comments), where
I've come down to the approach of using a report in ExecutorRun()
because it is simpler and it does the job. Perhaps also 0003, but
nobody has complained about that, either.

I have also looked at the tests proposed (isolation, TAP, custom
module); all of them are a bit disappointing because they duplicate
some patterns that are already tested in pg_stat_statements, while
willing to check the contents of pg_stat_statements. I am afraid that
it is not going to age well because we'd need to have the same query
patterns in more than one place. We should have tests, definitely,
but we can do an equivalent of pg_stat_activity lookups by calling
pgstat_get_my_query_id() in strategic places, making sure that all
dedicated paths always have the query ID reported:
- Check pgstat_get_my_query_id() in the run, finish and end executor
hooks.
- In the parse-analyze hook, before the query ID is reported (except
for a PREPARE), check that the ID in a Query is set.

The test proposed by Robert on the other thread was fancy enough that
I've added it. All that is in 0002, and that's enough to cause 0001
to fail, planning only these on HEAD. Tests in 0003 require fetch
messages, and I don't have a trick in my sleeves except if we invent a
new meta-command in psql.

There are other problems mentioned on this thread, with plan caching
for example. Let's deal with that separately, in separate threads.

[1]: https://www.postgresql.org/message-id/CA+TgmoZxtnf_jZ=VqBSyaU8hfUkkwoJCJ6ufy4LGpXaunKrjrg@mail.gmail.com
--
Michael