Re: BUG #17552: pg_stat_statements tracks internal FK check queries when COPY used to load data

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>
Cc: Sergei Kornilov <sk(at)zsrv(dot)org>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, maxim(dot)boguk(at)gmail(dot)com, pgsql-bugs(at)lists(dot)postgresql(dot)org
Subject: Re: BUG #17552: pg_stat_statements tracks internal FK check queries when COPY used to load data
Date: 2023-11-01 21:20:21
Message-ID: 2869953.1698873621@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs

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

Attachment Content-Type Size
v3-0001-increment-exec-nest-level-more-often.patch text/x-diff 7.8 KB
v3-0002-handle-plan-nesting-better.patch text/x-diff 7.8 KB

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Alexander Korotkov 2023-11-01 22:29:41 Re: BUG #18170: Unexpected error: no relation entry for relid 3
Previous Message Tom Lane 2023-11-01 17:15:27 Re: BUG #17552: pg_stat_statements tracks internal FK check queries when COPY used to load data