> 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
