Re: BUG #17552: pg_stat_statements tracks internal FK check queries when COPY used to load data
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Sergei Kornilov <sk@zsrv.org>, Julien Rouhaud <rjuju123@gmail.com>,
maxim.boguk@gmail.com, pgsql-bugs@lists.postgresql.org
Date: 2023-11-01T21:20:21Z
Lists: pgsql-bugs
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Fix some issues with tracking nesting level in pg_stat_statements.
- 76db9cb6368e 17.0 landed
Attachments
- v3-0001-increment-exec-nest-level-more-often.patch (text/x-diff) patch v3-0001
- v3-0002-handle-plan-nesting-better.patch (text/x-diff) patch v3-0002
I wrote: > However, I then started to wonder whether it's really a good idea > that the code treats PREPARE and EXECUTE alike for this purpose. > In the case of EXECUTE, the idea is that we'll still be at top > level when we reach the executor hooks, and they will do the right > thing and then increment exec_nested_level before any lower-level > statement can be reached. But how does that concept apply to > PREPARE, which won't reach the executor? ISTM the net result > is that if any subsidiary statements are reached during PREPARE > (perhaps via const-simplification of some SQL or PL function), > we will erroneously treat them as top-level. Poking at that, I found that (a) PREPARE doesn't run the planner, so probably nested statements are impossible, and (b) if we remove the check so that it bumps the nesting level, then some existing regression test outputs change. PREPARE does run parse analysis of the contained statement, and it seems that that's sensitive to the is-top-level state. So we do need to keep the exclusion for PREPARE. However, I'm still fairly down on PGSS_HANDLED_UTILITY, because I think the rationale for treating PREPARE and EXECUTE specially is different in each place where that's being used, so tying them together is more likely to cause future bugs than prevent bugs. So v3-0001 attached revises the patch per those ideas. It's only cosmetically different from before, but I think the explanatory comments are better. Meanwhile, I realized that we have a second set of bugs. As I said above, it's possible to reach nested statements during planning, as a consequence of const-simplification of an immutable or stable function. pg_stat_statements generally does the wrong thing here, incorrectly treating such statements as top-level if we weren't already nested. This needs to be fixed by including plan_nest_level as a reason to consider execution to be nested too. What's more, testing this showed that pgss_planner has the same bug as pgss_ProcessUtility: it needs to bump the nesting level even if it chooses not to track planning time. v3-0002 fixes this in a minimally invasive way by replacing each check of exec_nested_level by plan_nested_level + exec_nested_level. That's just for review/testing though. What I think we actually ought to do is fold plan_nested_level and exec_nested_level into a single variable nested_level (or nesting_level would be better English). There's no reason to keep them separate; it just adds cycles, complexity, and more risk of mistakes. Comments? regards, tom lane