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

Hi,

On 2/16/23 1:34 AM, Michael Paquier wrote:
> While wondering about this stuff about the last few days and
> discussing with bertrand, I have changed my mind on the point that
> there is no need to be that aggressive yet with the normalization of
> the A_Const nodes, because the query string normalization of
> pg_stat_statements is not prepared yet to handle cases where a A_Const
> value uses a non-quoted value with whitespaces.  The two cases where I
> saw an impact is on the commands that can define an isolation level:
> SET TRANSACTION and BEGIN.
> 
> For example, applying normalization to A_Const nodes does the
> following as of HEAD:
> 1) BEGIN TRANSACTION READ ONLY, READ WRITE, DEFERRABLE, NOT DEFERRABLE;
> BEGIN TRANSACTION $1 ONLY, $2 WRITE, $3, $4 DEFERRABLE
> 2) SET TRANSACTION ISOLATION LEVEL READ COMMITTED;
> SET TRANSACTION ISOLATION LEVEL $1 COMMITTED
> 
> On top of that, specifying a different isolation level may cause these
> commands to be grouped, which is not really cool.  All that could be
> done incrementally later on, in 17~ or later depending on the
> adjustments that make sense.
> 

Thanks for those patches!

Yeah, agree about the proposed approach.


> 0002 has been
> completed with a couple of commands to track all the commands with
> A_Const, so as we never lose sight of what happens.  0004 is what I
> think could be done for PG16, where normalization affects only Const.
> At the end of the day, this reflects the following commands that use
> Const nodes because they use directly queries, so the same rules as
> SELECT and DMLs apply to them:
> - DECLARE
> - EXPLAIN
> - CREATE MATERIALIZED VIEW
> - CTAS, SELECT INTO
> 

0001:

I like the idea of splitting the existing tests in dedicated files.

What do you think about removing:

"
SET pg_stat_statements.track_utility = FALSE;
SET pg_stat_statements.track_planning = TRUE;
"

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.

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.

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

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.

Regards,

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