Re: query_id, pg_stat_activity, extended query protocol

I took a look at your patches and here are my comments

> 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.

1/
In your 0003-Report-query-ID-for-execute-fetch-in-extended-query-.patch
patch, you are still setting the queryId inside exec_execute_message
if (execute_is_fetch). This condition could be removed and don't need to set
the queryId inside ExecutorRun. This is exactly what v5-0001 does.

V5-0001 also sets the queryId inside the exec_bind_message.
We must do that otherwise we will have a NULL queryId during bind.

also tested it against this for the case that was raised by Robert [1].

I also think we need to handle RevalidateCachedQuery. This is the case where we
have a new queryId after a cached query revalidation.

I addressed the comments by Andrei [3] in v5-0002. For RevalidateCachedQuery,
we can simple call pgstat_report_query_id with "force" = "false" so it will take care
of updating a queryId only if it's a top level query.

2/
As far as 0002-Add-sanity-checks-related-to-query-ID-reporting-in-p.patch,
I do like the pg_stat_statements extended tests to perform these tests.

What about adding the Assert(pgstat_get_my_query_id() != 0) inside
exec_parse_message, exec_bind_message and exec_execute_message as well?

I think having the Asserts inside the hooks in pg_stat_statements are good
as well.

I am not sure how we can add tests for RevalidateCachedQuery though using
pg_stat_statements. We could skip testing this scenario, maybe??

Let me know what you think.

[1] https://www.postgresql.org/message-id/CA+TgmoZxtnf_jZ=VqBSyaU8hfUkkwoJCJ6ufy4LGpXaunKrjrg@mail.gmail.com
[2] https://www.postgresql.org/message-id/flat/2beb1a00-3060-453a-90a6-7990d6940d62%40gmail.com#fffec59b563dbf49910e8b6d9f855e5a
[3] https://www.postgresql.org/message-id/F001F959-400F-41C6-9886-C9665A4DE0A3%40gmail.com

Regards,

Sami