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
