> Do you think that we'd better replace the calls reporting the query ID > in execMain.c by some assertions on HEAD? This won't work for > ExecutorStart() because PREPARE statements (or actually EXECUTE, > e.g. I bumped on that yesterday but I don't recall which one) would
Yes, adding the asserts in execMain.c is better, but there is complications there due to the issue you mention. I think the issue you are bumping into is when pg_stat_statements.track_utility = on ( default ), the assert in ExecutorStart will fail on EXECUTE. I believe it's because ( need to verify ) pg_stat_statements.c sets the queryId = 0 in the ProcessUtility hook [1]. > So your point would be to force this rule within the core executor on > HEAD? I would not object to that in case we're missing more spots > with the extended query protocol, actually. That would help us detect > cases where we're still missing the query ID to be set and the > executor should know about that. Yes, but looking at how pg_stat_statements works with PREPARE/EXECUTE, I am now thinking it's better to Just keep the tests in pg_stat_statements. Having test coverage in pg_stat_statements is better than nothing, and check-world ( or similar ) will be able to cacth such failures. > Note that I'm not much worried about the dependency with > pg_stat_statements. We already rely on it for query jumbling > normalization for some parse node patterns like DISCARD, and query > jumbling requires query IDs to be around. So that's not new. Good point. [1] https://github.com/postgres/postgres/blob/master/contrib/pg_stat_statements/pg_stat_statements.c#L1127-L1128 Regards, Sami
