On Tue, Sep 24, 2024 at 04:57:28PM +0900, Michael Paquier wrote: > 0001 is straight-forward and that was I think a mistake to not include > that from the start when I've expanded these tests in the v16 cycle > (well, time..). 0002 also is quite conservative at the end, and this > design can be used to tune easily the jumbling patterns from gram.y > depending on the feedback we'd get.
Applied 0001 for now to expand the tests, with one tweak: the removal of SET NAMES. It was proving tricky to use something else than UTF-8, the CI complaining on Windows. True that this could use like unaccent and an alternate output in a different file, but I'm not inclined to take the cost just for this specific query pattern. The remaining 0002 is attached for now. I am planning to wrap that next week after a second lookup, except if there are any comments, of course. -- Michael
From 40983157423d93334f30c70def5bf311fcff80f6 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 25 Sep 2024 12:08:04 +0900
Subject: [PATCH v3] Normalize queries starting with SET
---
src/include/nodes/parsenodes.h | 20 +++++++++++++---
src/backend/nodes/queryjumblefuncs.c | 19 +++++++++++++++
src/backend/parser/gram.y | 24 +++++++++++++++++++
contrib/pg_stat_statements/expected/dml.out | 2 +-
.../expected/level_tracking.out | 2 +-
.../pg_stat_statements/expected/utility.out | 20 ++++++----------
contrib/pg_stat_statements/expected/wal.out | 2 +-
7 files changed, 70 insertions(+), 19 deletions(-)
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index e62ce1b753..9e76713f2d 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2616,11 +2616,25 @@ typedef enum VariableSetKind
typedef struct VariableSetStmt
{
+ pg_node_attr(custom_query_jumble)
+
NodeTag type;
VariableSetKind kind;
- char *name; /* variable to be set */
- List *args; /* List of A_Const nodes */
- bool is_local; /* SET LOCAL? */
+ /* variable to be set */
+ char *name;
+ /* List of A_Const nodes */
+ List *args;
+
+ /*
+ * True if arguments should be accounted for in query jumbling. We use a
+ * separate flag rather than query_jumble_ignore on "args" as several
+ * grammar flavors of SET rely on a list of values that are parsed
+ * directly from the grammar's keywords.
+ */
+ bool jumble_args;
+ /* SET LOCAL? */
+ bool is_local;
+ ParseLoc location pg_node_attr(query_jumble_location);
} VariableSetStmt;
/* ----------------------
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index 129fb44709..5e43fd9229 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -57,6 +57,7 @@ static void RecordConstLocation(JumbleState *jstate, int location);
static void _jumbleNode(JumbleState *jstate, Node *node);
static void _jumbleA_Const(JumbleState *jstate, Node *node);
static void _jumbleList(JumbleState *jstate, Node *node);
+static void _jumbleVariableSetStmt(JumbleState *jstate, Node *node);
/*
* Given a possibly multi-statement source string, confine our attention to the
@@ -352,3 +353,21 @@ _jumbleA_Const(JumbleState *jstate, Node *node)
}
}
}
+
+static void
+_jumbleVariableSetStmt(JumbleState *jstate, Node *node)
+{
+ VariableSetStmt *expr = (VariableSetStmt *) node;
+
+ JUMBLE_FIELD(kind);
+ JUMBLE_STRING(name);
+
+ /*
+ * Account for the list of arguments in query jumbling only if told by the
+ * parser.
+ */
+ if (expr->jumble_args)
+ JUMBLE_NODE(args);
+ JUMBLE_FIELD(is_local);
+ JUMBLE_LOCATION(location);
+}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index b1d4642c59..4aa8646af7 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -1647,6 +1647,8 @@ set_rest:
n->kind = VAR_SET_MULTI;
n->name = "TRANSACTION";
n->args = $2;
+ n->jumble_args = true;
+ n->location = -1;
$$ = n;
}
| SESSION CHARACTERISTICS AS TRANSACTION transaction_mode_list
@@ -1656,6 +1658,8 @@ set_rest:
n->kind = VAR_SET_MULTI;
n->name = "SESSION CHARACTERISTICS";
n->args = $5;
+ n->jumble_args = true;
+ n->location = -1;
$$ = n;
}
| set_rest_more
@@ -1669,6 +1673,7 @@ generic_set:
n->kind = VAR_SET_VALUE;
n->name = $1;
n->args = $3;
+ n->location = @3;
$$ = n;
}
| var_name '=' var_list
@@ -1678,6 +1683,7 @@ generic_set:
n->kind = VAR_SET_VALUE;
n->name = $1;
n->args = $3;
+ n->location = @3;
$$ = n;
}
| var_name TO DEFAULT
@@ -1686,6 +1692,7 @@ generic_set:
n->kind = VAR_SET_DEFAULT;
n->name = $1;
+ n->location = -1;
$$ = n;
}
| var_name '=' DEFAULT
@@ -1694,6 +1701,7 @@ generic_set:
n->kind = VAR_SET_DEFAULT;
n->name = $1;
+ n->location = -1;
$$ = n;
}
;
@@ -1706,6 +1714,7 @@ set_rest_more: /* Generic SET syntaxes: */
n->kind = VAR_SET_CURRENT;
n->name = $1;
+ n->location = -1;
$$ = n;
}
/* Special syntaxes mandated by SQL standard: */
@@ -1715,6 +1724,8 @@ set_rest_more: /* Generic SET syntaxes: */
n->kind = VAR_SET_VALUE;
n->name = "timezone";
+ n->location = -1;
+ n->jumble_args = true;
if ($3 != NULL)
n->args = list_make1($3);
else
@@ -1736,6 +1747,7 @@ set_rest_more: /* Generic SET syntaxes: */
n->kind = VAR_SET_VALUE;
n->name = "search_path";
n->args = list_make1(makeStringConst($2, @2));
+ n->location = @2;
$$ = n;
}
| NAMES opt_encoding
@@ -1744,6 +1756,7 @@ set_rest_more: /* Generic SET syntaxes: */
n->kind = VAR_SET_VALUE;
n->name = "client_encoding";
+ n->location = @2;
if ($2 != NULL)
n->args = list_make1(makeStringConst($2, @2));
else
@@ -1757,6 +1770,7 @@ set_rest_more: /* Generic SET syntaxes: */
n->kind = VAR_SET_VALUE;
n->name = "role";
n->args = list_make1(makeStringConst($2, @2));
+ n->location = @2;
$$ = n;
}
| SESSION AUTHORIZATION NonReservedWord_or_Sconst
@@ -1766,6 +1780,7 @@ set_rest_more: /* Generic SET syntaxes: */
n->kind = VAR_SET_VALUE;
n->name = "session_authorization";
n->args = list_make1(makeStringConst($3, @3));
+ n->location = @3;
$$ = n;
}
| SESSION AUTHORIZATION DEFAULT
@@ -1774,6 +1789,7 @@ set_rest_more: /* Generic SET syntaxes: */
n->kind = VAR_SET_DEFAULT;
n->name = "session_authorization";
+ n->location = -1;
$$ = n;
}
| XML_P OPTION document_or_content
@@ -1783,6 +1799,8 @@ set_rest_more: /* Generic SET syntaxes: */
n->kind = VAR_SET_VALUE;
n->name = "xmloption";
n->args = list_make1(makeStringConst($3 == XMLOPTION_DOCUMENT ? "DOCUMENT" : "CONTENT", @3));
+ n->jumble_args = true;
+ n->location = -1;
$$ = n;
}
/* Special syntaxes invented by PostgreSQL: */
@@ -1793,6 +1811,7 @@ set_rest_more: /* Generic SET syntaxes: */
n->kind = VAR_SET_MULTI;
n->name = "TRANSACTION SNAPSHOT";
n->args = list_make1(makeStringConst($3, @3));
+ n->location = @3;
$$ = n;
}
;
@@ -1900,6 +1919,7 @@ reset_rest:
n->kind = VAR_RESET;
n->name = "timezone";
+ n->location = -1;
$$ = n;
}
| TRANSACTION ISOLATION LEVEL
@@ -1908,6 +1928,7 @@ reset_rest:
n->kind = VAR_RESET;
n->name = "transaction_isolation";
+ n->location = -1;
$$ = n;
}
| SESSION AUTHORIZATION
@@ -1916,6 +1937,7 @@ reset_rest:
n->kind = VAR_RESET;
n->name = "session_authorization";
+ n->location = -1;
$$ = n;
}
;
@@ -1927,6 +1949,7 @@ generic_reset:
n->kind = VAR_RESET;
n->name = $1;
+ n->location = -1;
$$ = n;
}
| ALL
@@ -1934,6 +1957,7 @@ generic_reset:
VariableSetStmt *n = makeNode(VariableSetStmt);
n->kind = VAR_RESET_ALL;
+ n->location = -1;
$$ = n;
}
;
diff --git a/contrib/pg_stat_statements/expected/dml.out b/contrib/pg_stat_statements/expected/dml.out
index f6ac8da5ca..acc2c5e524 100644
--- a/contrib/pg_stat_statements/expected/dml.out
+++ b/contrib/pg_stat_statements/expected/dml.out
@@ -82,7 +82,7 @@ SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
2 | 4 | SELECT * FROM pgss_dml_tab WHERE a > $1 ORDER BY a
1 | 8 | SELECT * FROM pgss_dml_tab WHERE a IN ($1, $2, $3, $4, $5)
1 | 1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
- 1 | 0 | SET pg_stat_statements.track_utility = FALSE
+ 1 | 0 | SET pg_stat_statements.track_utility = $1
6 | 6 | UPDATE pgss_dml_tab SET b = $1 WHERE a = $2
1 | 3 | UPDATE pgss_dml_tab SET b = $1 WHERE a > $2
(10 rows)
diff --git a/contrib/pg_stat_statements/expected/level_tracking.out b/contrib/pg_stat_statements/expected/level_tracking.out
index d8dd8a2dee..bb65e98ce0 100644
--- a/contrib/pg_stat_statements/expected/level_tracking.out
+++ b/contrib/pg_stat_statements/expected/level_tracking.out
@@ -64,7 +64,7 @@ SELECT toplevel, calls, query FROM pg_stat_statements
| | END; $$
f | 1 | SELECT $1::TEXT
t | 1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
- t | 1 | SET pg_stat_statements.track = 'all'
+ t | 1 | SET pg_stat_statements.track = $1
(7 rows)
-- Procedure with multiple utility statements.
diff --git a/contrib/pg_stat_statements/expected/utility.out b/contrib/pg_stat_statements/expected/utility.out
index 8d3fd77201..060d4416dd 100644
--- a/contrib/pg_stat_statements/expected/utility.out
+++ b/contrib/pg_stat_statements/expected/utility.out
@@ -108,7 +108,7 @@ SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
1 | 0 | CREATE FOREIGN TABLE foreign_stats (a int) SERVER server_stats
1 | 0 | CREATE FUNCTION func_stats(a text DEFAULT 'a_data', b text DEFAULT lower('b_data'))+
| | RETURNS text AS $$ SELECT $1::text || '_' || $2::text; $$ LANGUAGE SQL +
- | | SET work_mem = '256kB'
+ | | SET work_mem = $1
1 | 0 | CREATE FUNCTION trigger_func_stats () RETURNS trigger LANGUAGE plpgsql +
| | AS $$ BEGIN return OLD; end; $$
1 | 0 | CREATE INDEX pt_stats2_index ON ONLY pt_stats2 (a)
@@ -620,8 +620,7 @@ SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
1 | 0 | SET LOCAL SESSION AUTHORIZATION 'regress_stat_set_1'
1 | 0 | SET LOCAL SESSION AUTHORIZATION 'regress_stat_set_2'
1 | 0 | SET LOCAL SESSION AUTHORIZATION DEFAULT
- 1 | 0 | SET LOCAL work_mem = '128kB'
- 1 | 0 | SET LOCAL work_mem = '256kB'
+ 2 | 0 | SET LOCAL work_mem = $1
2 | 0 | SET LOCAL work_mem = DEFAULT
1 | 0 | SET LOCAL work_mem FROM CURRENT
1 | 0 | SET SESSION AUTHORIZATION 'regress_stat_set_1'
@@ -630,8 +629,6 @@ SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
1 | 0 | SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY, READ ONLY
1 | 0 | SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY, READ WRITE
1 | 0 | SET SESSION SESSION AUTHORIZATION DEFAULT
- 1 | 0 | SET SESSION work_mem = '300kB'
- 1 | 0 | SET SESSION work_mem = '400kB'
1 | 0 | SET TIME ZONE 'America/New_York'
1 | 0 | SET TIME ZONE 'Asia/Tokyo'
1 | 0 | SET TIME ZONE 'CST7CDT,M4.1.0,M10.5.0'
@@ -641,13 +638,11 @@ SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
1 | 0 | SET TRANSACTION ISOLATION LEVEL SERIALIZABLE
1 | 0 | SET XML OPTION CONTENT
1 | 0 | SET XML OPTION DOCUMENT
- 1 | 0 | SET enable_seqscan = off
- 1 | 0 | SET enable_seqscan = on
- 2 | 0 | SET work_mem = '1MB'
- 1 | 0 | SET work_mem = '2MB'
+ 2 | 0 | SET enable_seqscan = $1
+ 5 | 0 | SET work_mem = $1
2 | 0 | SET work_mem = DEFAULT
1 | 0 | SET work_mem FROM CURRENT
-(39 rows)
+(34 rows)
DROP ROLE regress_stat_set_1;
DROP ROLE regress_stat_set_2;
@@ -733,9 +728,8 @@ SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
-------+------+----------------------------------------------------
1 | 0 | RESET ALL
1 | 1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
- 1 | 0 | SET SCHEMA 'foo'
- 1 | 0 | SET SCHEMA 'public'
-(4 rows)
+ 2 | 0 | SET SCHEMA $1
+(3 rows)
SELECT pg_stat_statements_reset() IS NOT NULL AS t;
t
diff --git a/contrib/pg_stat_statements/expected/wal.out b/contrib/pg_stat_statements/expected/wal.out
index 34a2bf5b03..977e382d84 100644
--- a/contrib/pg_stat_statements/expected/wal.out
+++ b/contrib/pg_stat_statements/expected/wal.out
@@ -18,7 +18,7 @@ FROM pg_stat_statements ORDER BY query COLLATE "C";
DELETE FROM pgss_wal_tab WHERE a > $1 | 1 | 1 | t | t | t
INSERT INTO pgss_wal_tab VALUES(generate_series($1, $2), $3) | 1 | 10 | t | t | t
SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1 | 1 | f | f | f
- SET pg_stat_statements.track_utility = FALSE | 1 | 0 | f | f | t
+ SET pg_stat_statements.track_utility = $1 | 1 | 0 | f | f | t
UPDATE pgss_wal_tab SET b = $1 WHERE a > $2 | 1 | 3 | t | t | t
(5 rows)
--
2.45.2
signature.asc
Description: PGP signature
