On Wed, Sep 11, 2024 at 05:02:07PM -0500, Sami Imseih wrote: > 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].
There are a few ways to do things: - Add an extra report in ExecutorRun(), because we know that it is going to be what we are going to cross when using a portal with multiple execution calls. This won't work for the case of multiple fetch messages where there is only one initial ExecutorRun() call followed by the tuple fetches, as you say. - Add these higher in the stack, when processing the messages. In which case, we can also argue about removing the calls in ExecutorRun() and ExecutorStart(), entirely, because these are unnecessary duplicates as long as the query ID is set close to where it is reset when we are processing the kind and execute messages. ExecutorStart() as report location is ill-thought from the start. - Keep all of them, relying on the first one set as the follow-up ones are harmless. Perhaps also just reduce the number of calls on HEAD. After sleeping on it, I'd tend to slightly favor the last option in the back-branches and the second option on HEAD where we reduce the number of report calls. This way, we are a bit more careful in released branches by being more aggressive in reporting the query ID. That's also why I have ordered the previous patch set this way but that was badly presented, even if it does not take care of the processing of the execute_is_fetch case for execute messages. 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 we have a good deal of coverage on top of the other queries already in the test suite. That's also the only place in core where we force the query ID across all these hooks, and this does not impact switching the way stats are stored if we were to switch to pgstats in shmem with the custom stats APIs. > I am not sure how we can add tests for RevalidateCachedQuery though using > pg_stat_statements. We could skip testing this scenario, maybe?? Perhaps. I'd need to think through this one. Let's do things in order and see about the reports for the bind/execute messages, first, please? -- Michael
signature.asc
Description: PGP signature
