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=vqbsyau8hfukkwojcj6ufy4lgpxaunkr...@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
v5-0001-Add-missing-queryId-reporting-in-extended-query.patch
Description: Binary data
v5-0002-Report-new-queryId-after-plancache-re-validation.patch
Description: Binary data
