Re: pg_stat_statements: Fix nested tracking for implicitly closed cursors - Mailing list pgsql-hackers

Hi,

It was brought to my attention that there is pg_stat_statements
behavior where an implicitly closed cursor, via COMMIT or END,
is stored as toplevel for both the utility DECLARE CURSOR
statement and the underlying query.

```
BEGIN;
DECLARE FOOCUR CURSOR FOR SELECT * from stats_track_tab;
FETCH FORWARD 1 FROM foocur;
 x
---
(0 rows)

COMMIT;
SELECT toplevel, calls, query FROM pg_stat_statements
  ORDER BY query COLLATE "C";
 toplevel | calls |                          query
----------+-------+----------------------------------------------------------
 t        |     1 | BEGIN
 t        |     1 | COMMIT
 t        |     1 | DECLARE FOOCUR CURSOR FOR SELECT * from stats_track_tab
 t        |     1 | DECLARE FOOCUR CURSOR FOR SELECT * from stats_track_tab;
 t        |     1 | FETCH FORWARD $1 FROM foocur
 t        |     1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
(6 rows)
```

Also, with track_planning enabled, the underlying query is
stored with toplevel = false for the plans counter and with
toplevel = true for the calls counter, resulting in an
additional entry.

```
SELECT toplevel, calls, plans, query FROM pg_stat_statements
  ORDER BY query COLLATE "C";
 toplevel | calls | plans |                            query
----------+-------+-------+--------------------------------------------------------------
 t        |     1 |     0 | BEGIN
 t        |     1 |     0 | COMMIT
 t        |     1 |     0 | DECLARE FOOCUR CURSOR FOR SELECT * from
stats_track_tab
 t        |     1 |     0 | DECLARE FOOCUR CURSOR FOR SELECT * from
stats_track_tab;
 f        |     0 |     1 | DECLARE FOOCUR CURSOR FOR SELECT * from
stats_track_tab;
 t        |     1 |     0 | FETCH FORWARD $1 FROM foocur
 t        |     1 |     0 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
 t        |     0 |     1 | SELECT toplevel, calls, plans, query FROM
pg_stat_statements+
          |       |       |   ORDER BY query COLLATE "C"
 ```

The reason this occurs is because PortalCleanup, which triggers
ExecutorEnd, runs after the ProcessUtility hook. At that point,
nesting_level has already been reset back to 0.

I am not sure how common this pattern is, but it is probably
worth fixing. At a minimum, we need tests to show this behavior,
but we can do better by checking whether we just processed a
COMMIT statement and setting a flag to let ExecutorEnd increment
nesting_level. There should be no other way to reach ExecutorEnd
after a COMMIT besides PortalCleanup, AFAICT. I could be proven
wrong.

The attached patch fixes this as described above.

Note that, due to f85f6ab051b7, there is a separate issue that
should be improved. Tracking the underlying SQL of a utility
statement using the utility statement itself is confusing
and should be fixed. That is a separate point, but I am
mentioning it here for clarity.


--
Sami Imseih
Amazon Web Services