On Tue, Jul 23, 2024 at 04:00:25PM -0500, Sami Imseih wrote: > Correct, I also don´t think ExecutorRun is enough. Another reason is we > should also > be setting the queryId during bind, right before planning starts. > Planning could have significant impact on the server and I think we better > track the responsible queryId. > > I have not tested the holdStore case. IIUC the holdStore deals with fetching > a > WITH HOLD CURSOR. Why would this matter for this conversation?
Not only, see portal.h. This matters for holdable cursors, PORTAL_ONE_RETURNING and PORTAL_UTIL_SELECT. > Doing the work in exec_execute_message makes sense, although maybe > setting the queryId after pgstat_report_activity is better because it occurs > earlier. > Also, we should do the same for exec_bind_message and set the queryId > right after pgstat_report_activity in this function as well. Sounds fine by me (still need to check all three patterns). + if (list_length(psrc->query_list) > 0) + pgstat_report_query_id(linitial_node(Query, psrc->query_list)->queryId, false); Something that slightly worries me is to assume that the first Query in the query_list is fetched. Using a foreach() for all three paths may be better, jumping out at the loop when finding a valid query ID. I have not looked at that entirely in details, and I'd need to check if it is possible to use what's here for more predictible tests: https://www.postgresql.org/message-id/ZqCMCS4HUshUYjGc%40paquier.xyz > We do have to account for the queryId changing after cache revalidation, so > we should still set the queryId inside GetCachedPlan in the case the query > underwent re-analysis. This means there is a chance that a queryId set at > the start of the exec_bind_message may be different by the time we complete > the function, in the case the query revalidation results in a different > queryId. Makes sense to me. I'd rather make that a separate patch, with, if possible, its own tests (the case of Andrei with a DROP/CREATE TABLE) . -- Michael
signature.asc
Description: PGP signature
