Re: Normalization of utility queries in pg_stat_statements

On Thu, Feb 16, 2023 at 10:55:32AM +0100, Drouvot, Bertrand wrote:
> In the new pg_stat_statements.sql? That way pg_stat_statements.sql would always behave
> with default values for those (currently we are setting both of them as non default).
>
> Then, with the default values in place, if we feel that some tests
> are missing we could add them in > utility.sql or planning.sql
> accordingly.

I am not sure about this part, TBH, so I have left these as they are.

Anyway, while having a second look at that, I have noticed that it is
possible to extract as an independent piece all the tests related to
level tracking. Things are worse than I thought initially, actually,
because we had test scenarios mixing planning and level tracking, but
the tests don't care about measuring plans at all, see around FETCH
FORWARD, meaning that queries on the table pg_stat_statements have
just been copy-pasted around for the last few years. There were more
tests that used "test" for a table name ;)

I have been pondering about this part, and the tracking matters for DO
blocks and PL functions, so I have moved all these cases into a new,
separate file. There is a bit more that can be done, for WAL tracking
and roles near the end of pg_stat_statements.sql, but I have left that
out for now. I have checked the output of the tests before and after
the refactoring for quite a bit of time, and the outputs match so
there is no loss of coverage.

0001 looks quite committable at this stage, and that's independent on
the rest. At the end this patch creates four new test files that are
extended in the next patches: utility, planning, track and cleanup.

> 0002:
>
> Produces:
> v2-0002-Add-more-test-for-utility-queries-in-pg_stat_stat.patch:834: trailing whitespace.
> CREATE VIEW view_stats_1 AS
> v2-0002-Add-more-test-for-utility-queries-in-pg_stat_stat.patch:838: trailing whitespace.
> CREATE VIEW view_stats_1 AS
> warning: 2 lines add whitespace errors.

Thanks, fixed.

> +-- SET TRANSACTION ISOLATION
> +BEGIN;
> +SET TRANSACTION ISOLATION LEVEL READ COMMITTED;
> +SET TRANSACTION ISOLATION LEVEL REPEATABLE READ;
> +SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
>
> What about adding things like "SET SESSION CHARACTERISTICS AS
> TRANSACTION..." too?

That's a good idea. It is again one of these fancy cases, better to
keep a track of them in the long-term..

> 0003 and 0004:
> Thanks for keeping 0003 that's useful to see the impact of A_Const normalization.
>
> Looking at the diff they produce, I also do think that 0004 is what
> could be done for PG16.

I am wondering if others have an opinion to share about that, but,
yes, 0004 seems enough to begin with. We could always study more
normalization areas in future releases, taking it slowly.
--
Michael