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