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