Message-ID: <14965f63-6d78-45ff-d029-daafe8eddb40@gmail.com>
Date: Fri, 17 Feb 2023 09:36:27 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.8.0
Subject: Re: Normalization of utility queries in pg_stat_statements
To: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
References: <Y+MRdEq9W9XVa2AB@paquier.xyz> <Y+16K+q9L0yjV3zA@paquier.xyz>
 <0e067034-e92c-302b-24ed-492bca228e26@gmail.com>
 <Y+7n+uz6XkzJpB7z@paquier.xyz>
Content-Language: en-US
From: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
In-Reply-To: <Y+7n+uz6XkzJpB7z@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/14965f63-6d78-45ff-d029-daafe8eddb40%40gmail.com>
Precedence: bulk

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