On Tue, Jan 31, 2023 at 03:40:56PM +0900, Michael Paquier wrote: > With all that in mind, I have spent my day polishing that and doing a > close lookup, and the patch has been applied. Thanks a lot!
While working on a different patch, I have noticed a small issue in the way the jumbling happens for A_Const, where ValUnion was not getting jumbled correctly. This caused a few statements that rely on this node to compile the same query IDs when using different values. The full contents of pg_stat_statements for a regression database point to: - SET. - COPY with queries. - CREATE TABLE with partition bounds and default expressions. This was causing some confusion in pg_stat_statements where some utility queries would be incorrectly reported, and at this point the intention is to keep this area compatible with the string-based method when it comes to the values. Like read, write and copy nodes, we need to compile the query ID based on the type of the value, which cannot be automated. Attached is a patch to fix this issue with some regression tests, that I'd like to get fixed before moving on with more business in pg_stat_statements (aka properly show Const nodes for utilities with normalized queries). Comments or objections are welcome, of course. (FWIW, I'd like to think that there is an argument to normalize the A_Const nodes for a portion of the DDL queries, by ignoring their values in the query jumbling and mark a location, which would be really useful for some workloads, but that's a separate discussion I am keeping for later.) -- Michael
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 3d67787e7a..855da99ec0 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -355,7 +355,7 @@ union ValUnion
typedef struct A_Const
{
- pg_node_attr(custom_copy_equal, custom_read_write)
+ pg_node_attr(custom_copy_equal, custom_read_write, custom_query_jumble)
NodeTag type;
union ValUnion val;
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index 223d1bc826..72ce15e43d 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -49,6 +49,7 @@ static void AppendJumble(JumbleState *jstate,
const unsigned char *item, Size size);
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 _jumbleRangeTblEntry(JumbleState *jstate, Node *node);
@@ -313,6 +314,40 @@ _jumbleList(JumbleState *jstate, Node *node)
}
}
+static void
+_jumbleA_Const(JumbleState *jstate, Node *node)
+{
+ A_Const *expr = (A_Const *) node;
+
+ JUMBLE_FIELD(isnull);
+ if (!expr->isnull)
+ {
+ JUMBLE_FIELD(val.node.type);
+ switch (nodeTag(&expr->val))
+ {
+ case T_Integer:
+ JUMBLE_FIELD(val.ival.ival);
+ break;
+ case T_Float:
+ JUMBLE_STRING(val.fval.fval);
+ break;
+ case T_Boolean:
+ JUMBLE_FIELD(val.boolval.boolval);
+ break;
+ case T_String:
+ JUMBLE_STRING(val.sval.sval);
+ break;
+ case T_BitString:
+ JUMBLE_STRING(val.bsval.bsval);
+ break;
+ default:
+ elog(ERROR, "unrecognized node type: %d",
+ (int) nodeTag(&expr->val));
+ break;
+ }
+ }
+}
+
static void
_jumbleRangeTblEntry(JumbleState *jstate, Node *node)
{
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index fb9ccd920f..753062a9d7 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -579,6 +579,14 @@ NOTICE: table "test" does not exist, skipping
NOTICE: table "test" does not exist, skipping
NOTICE: function plus_one(pg_catalog.int4) does not exist, skipping
DROP FUNCTION PLUS_TWO(INTEGER);
+-- This SET query uses two different strings, still thet count as one entry.
+SET work_mem = '1MB';
+Set work_mem = '1MB';
+SET work_mem = '2MB';
+RESET work_mem;
+SET enable_seqscan = off;
+SET enable_seqscan = on;
+RESET enable_seqscan;
SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
query | calls | rows
------------------------------------------------------------------------------+-------+------
@@ -588,10 +596,16 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
DROP FUNCTION PLUS_TWO(INTEGER) | 1 | 0
DROP TABLE IF EXISTS test | 3 | 0
DROP TABLE test | 1 | 0
+ RESET enable_seqscan | 1 | 0
+ RESET work_mem | 1 | 0
SELECT $1 | 1 | 1
SELECT pg_stat_statements_reset() | 1 | 1
SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" | 0 | 0
-(9 rows)
+ SET enable_seqscan = off | 1 | 0
+ SET enable_seqscan = on | 1 | 0
+ SET work_mem = '1MB' | 2 | 0
+ SET work_mem = '2MB' | 1 | 0
+(15 rows)
--
-- Track the total number of rows retrieved or affected by the utility
diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
index b82cddf16f..8ab8eb41ed 100644
--- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql
+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
@@ -270,6 +270,14 @@ DROP TABLE IF EXISTS test \;
Drop Table If Exists test \;
DROP FUNCTION IF EXISTS PLUS_ONE(INTEGER);
DROP FUNCTION PLUS_TWO(INTEGER);
+-- This SET query uses two different strings, still thet count as one entry.
+SET work_mem = '1MB';
+Set work_mem = '1MB';
+SET work_mem = '2MB';
+RESET work_mem;
+SET enable_seqscan = off;
+SET enable_seqscan = on;
+RESET enable_seqscan;
SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
signature.asc
Description: PGP signature
