On Wed, Sep 11, 2024 at 09:41:58PM -0500, Sami Imseih wrote: >> 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 > > I played around with BackgrounsPsql. It works and gives us more flexibility > in testing, but I think the pg_stat_statements test are good enough for this > purpose. > > My only concern is this approach tests core functionality ( reporting of > queryId ) > in the tests of a contrib module ( pg_stat_statements ). Is that a valid > concern?
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 blow up on that with compute_query_id enabled. We could do something like that in ExecutorRun() at least as that may be helpful for extensions? An assertion would be like: Assert(!IsQueryIdEnabled() || pgstat_get_my_query_id() != 0); ExecutorFinish() and ExecutorEnd() are not that mandatory, so there's a risk that this causes the backend to complain because a planner or post-analyze hook decides to force the hand of the backend entry in an extension. With such checks, we're telling them to just not do that. 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. The execute/fetch has been missing for years without us being able to detect it automatically. 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. -- Michael
signature.asc
Description: PGP signature
