Re: Normalization of utility queries in pg_stat_statements

Hi,

On 2/17/23 3:35 AM, Michael Paquier wrote:
> 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.
>

Thanks! LGTM.

>> 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.
>

Thanks!

>> +-- 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..
>

Right.

002 LGTM.

>> 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.

Agree.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com