Обсуждение: pg_stat_statements: add missing tests for nesting_level

Hi,

While looking at pg_stat_statements nesting_level, I realized that there
are missing nesting_level tests for pgss_planner and pgss_ExecutorFinish.
That is, if you remove nesting_level++ and nesting_level-- in those 2 hooks,
the tests will still succeed.

For pgss_planner the nesting_level updates missing tests are the ones
when track_planning is enabled.

Attached is a quick patch to add coverage.


--
Sami Imseih
Amazon Web Services (AWS)

On Tue, Jan 20, 2026 at 06:08:14PM -0600, Sami Imseih wrote:
> While looking at pg_stat_statements nesting_level, I realized that there
> are missing nesting_level tests for pgss_planner and pgss_ExecutorFinish.
> That is, if you remove nesting_level++ and nesting_level-- in those 2 hooks,
> the tests will still succeed.
>
> For pgss_planner the nesting_level updates missing tests are the ones
> when track_planning is enabled.
>
> Attached is a quick patch to add coverage.

Confirmed these two deficiencies, nice catch.  If one does the same
removal of the nesting level calculation in other code paths like
pgss_ExecutorRun(), one get complaints.  Will see to get this addition
done.
--
Michael

On Tue, Jan 20, 2026 at 06:08:14PM -0600, Sami Imseih wrote:

While looking at pg_stat_statements nesting_level, I realized that there
are missing nesting_level tests for pgss_planner and pgss_ExecutorFinish.
That is, if you remove nesting_level++ and nesting_level-- in those 2 hooks,
the tests will still succeed.

For pgss_planner the nesting_level updates missing tests are the ones
when track_planning is enabled.

Attached is a quick patch to add coverage.

Confirmed these two deficiencies, nice catch.  If one does the same 
removal of the nesting level calculation in other code paths like
pgss_ExecutorRun(), one get complaints.  Will see to get this addition
done.

Two buildfarm animals [1], [2] say that that addition is incompatible with
the CLOBBER_CACHE_ALWAYS mode:
not ok 5     - level_tracking                          52571 ms

diff -U3 /home/buildfarm/avocet/buildroot/HEAD/pgsql.build/contrib/pg_stat_statements/expected/level_tracking.out /home/buildfarm/avocet/buildroot/HEAD/pgsql.build/contrib/pg_stat_statements/results/level_tracking.out
--- /home/buildfarm/avocet/buildroot/HEAD/pgsql.build/contrib/pg_stat_statements/expected/level_tracking.out    2026-01-22 01:59:12.213054121 +0100
+++ /home/buildfarm/avocet/buildroot/HEAD/pgsql.build/contrib/pg_stat_statements/results/level_tracking.out    2026-01-22 05:24:17.363666155 +0100
@@ -1560,7 +1560,7 @@
  toplevel | calls | rows | plans |                               query                                
 ----------+-------+------+-------+--------------------------------------------------------------------
  t        |     2 |    2 |     2 | SELECT PLUS_THREE($1)
- f        |     2 |    2 |     2 | SELECT i + 3 LIMIT 1
+ f        |     2 |    2 |     2 | SELECT i + $2 LIMIT $3
  t        |     1 |    1 |     0 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
  t        |     0 |    0 |     1 | SELECT toplevel, calls, rows, plans, query FROM pg_stat_statements+
           |       |      |       |   ORDER BY query COLLATE "C"

I can reproduce this locally with no extra tricks. Could you please adjust
the test for this mode?

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=avocet&dt=2026-01-22%2000%3A58%3A36
[2] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=trilobite&dt=2026-01-24%2023%3A10%3A13

Best regards,
Alexander

On Sun, Jan 25, 2026 at 08:00:00AM +0200, Alexander Lakhin wrote:
> I can reproduce this locally with no extra tricks. Could you please adjust
> the test for this mode?

Reproduced here.  That was trickier than it looks.  A trick with
debug_discard_caches cannot help, because with a CLOBBER_CACHE_ALWAYS
build we would still do a GetCachedPlan() -> RevalidateCachedQuery()
that goes through the post-parse analyze hook where the query would
still be normalized, showing up in the output anyway.

So I have come up with a plan B.  If we do a DISCARD PLANS before the
*first* function call, we can force the test to revalidate the cached
query without caring about CLOBBER_CACHE_ALWAYS, meaning that we would
always store a normalized version of the inner query.  The point of
the test is to check after the nesting level calculation in the
planner hook, and the test is still able to check that correctly.  If
I remove the nesting_level bits from the code while the DISCARD is
around, the entry is stored as a top level entry incorrectly, but it
should be stored as toplevel=false.  I'll go apply the attached
shortly, after some more checks..
--
Michael

On Sun, Jan 25, 2026 at 04:41:22PM +0900, Michael Paquier wrote:
> Reproduced here.  That was trickier than it looks.  A trick with
> debug_discard_caches cannot help, because with a CLOBBER_CACHE_ALWAYS
> build we would still do a GetCachedPlan() -> RevalidateCachedQuery()
> that goes through the post-parse analyze hook where the query would
> still be normalized, showing up in the output anyway.

It took a couple of days, but avocet has reported back green today:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=avocet&dt=2026-01-28%2001%3A10%3A18

It means that we should be good now.
--
Michael