From e879139e9fcea6e9764fbe4af5553257949bcb65 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov Date: Fri, 18 Sep 2026 00:08:02 +0300 Subject: [PATCH v3] Jumble the WAIT option list by meaning, not by spelling The LSN target has been normalized since 447aae13b03 and the follow-up in this thread, but the WITH clause has not, so waits that differ only in how long they may take get an entry each, and writing the same options in a different order gets another one: WAIT FOR LSN $1 WITH (mode 'primary_flush', timeout '5ms') WAIT FOR LSN $1 WITH (mode 'primary_flush', timeout '7ms') WAIT FOR LSN $1 WITH (timeout '5ms', mode 'primary_flush') Give WaitStmt.options a custom jumble function that walks the recognized options in a fixed order rather than in the order written, so the last of those joins the first. Record TIMEOUT as a constant, since it only bounds how long the command may wait, which merges the first two and prints the value as a parameter. MODE and NO_THROW select what the command does, so their values are still jumbled and each mode keeps its own entry. Do not normalize option values generically. What a value means differs per option, and only the command knows: the parser has the same problem with SET and solves it with VariableSetStmt.jumble_args, set per grammar flavor. DefElem.arg_location is added for the benefit of the above, but nothing else reads it, so no other statement changes. Discussion: https://postgr.es/m/1f04f78c-2cfa-47a0-997f-f02572cd10eb%40eisentraut.org --- .../pg_stat_statements/expected/utility.out | 37 ++++++++++-- contrib/pg_stat_statements/sql/utility.sql | 7 +++ src/backend/nodes/makefuncs.c | 2 + src/backend/nodes/queryjumblefuncs.c | 60 +++++++++++++++++++ src/backend/parser/gram.y | 1 + src/include/nodes/parsenodes.h | 3 +- 6 files changed, 105 insertions(+), 5 deletions(-) diff --git a/contrib/pg_stat_statements/expected/utility.out b/contrib/pg_stat_statements/expected/utility.out index 0762adbe94a..4d09836ae66 100644 --- a/contrib/pg_stat_statements/expected/utility.out +++ b/contrib/pg_stat_statements/expected/utility.out @@ -744,15 +744,44 @@ WAIT FOR LSN '0/0' WITH (MODE 'primary_flush', TIMEOUT 1); success (1 row) +-- TIMEOUT is a value: these join the entry above. The order the options +-- are written in does not matter either. +WAIT FOR LSN '0/0' WITH (MODE 'primary_flush', TIMEOUT 2); + status +--------- + success +(1 row) + +WAIT FOR LSN '0/0' WITH (TIMEOUT 3, MODE 'primary_flush'); + status +--------- + success +(1 row) + +-- MODE says what to wait for, so each mode gets its own entry. +WAIT FOR LSN '0/0' WITH (MODE 'standby_replay', TIMEOUT 1, NO_THROW); + status +----------------- + not in recovery +(1 row) + +WAIT FOR LSN '0/0' WITH (MODE 'standby_write', TIMEOUT 1, NO_THROW); + status +----------------- + not in recovery +(1 row) + SELECT calls, rows, query FROM pg_stat_statements WHERE query LIKE 'WAIT FOR LSN%' ORDER BY query COLLATE "C"; - calls | rows | query --------+------+-------------------------------------------------------- + calls | rows | query +-------+------+-------------------------------------------------------------------- 2 | 0 | WAIT FOR LSN $1 WITH (MODE 'primary_flush') 1 | 0 | WAIT FOR LSN $1 WITH (MODE 'primary_flush', NO_THROW) - 1 | 0 | WAIT FOR LSN $1 WITH (MODE 'primary_flush', TIMEOUT 1) -(3 rows) + 3 | 0 | WAIT FOR LSN $1 WITH (MODE 'primary_flush', TIMEOUT $2) + 1 | 0 | WAIT FOR LSN $1 WITH (MODE 'standby_replay', TIMEOUT $2, NO_THROW) + 1 | 0 | WAIT FOR LSN $1 WITH (MODE 'standby_write', TIMEOUT $2, NO_THROW) +(5 rows) SELECT pg_stat_statements_reset() IS NOT NULL AS t; t diff --git a/contrib/pg_stat_statements/sql/utility.sql b/contrib/pg_stat_statements/sql/utility.sql index b159c1de1aa..abf64845e9f 100644 --- a/contrib/pg_stat_statements/sql/utility.sql +++ b/contrib/pg_stat_statements/sql/utility.sql @@ -370,6 +370,13 @@ WAIT FOR LSN '0/0' WITH (MODE 'primary_flush'); WAIT FOR LSN '0/1' WITH (MODE 'primary_flush'); WAIT FOR LSN '0/0' WITH (MODE 'primary_flush', NO_THROW); WAIT FOR LSN '0/0' WITH (MODE 'primary_flush', TIMEOUT 1); +-- TIMEOUT is a value: these join the entry above. The order the options +-- are written in does not matter either. +WAIT FOR LSN '0/0' WITH (MODE 'primary_flush', TIMEOUT 2); +WAIT FOR LSN '0/0' WITH (TIMEOUT 3, MODE 'primary_flush'); +-- MODE says what to wait for, so each mode gets its own entry. +WAIT FOR LSN '0/0' WITH (MODE 'standby_replay', TIMEOUT 1, NO_THROW); +WAIT FOR LSN '0/0' WITH (MODE 'standby_write', TIMEOUT 1, NO_THROW); SELECT calls, rows, query FROM pg_stat_statements WHERE query LIKE 'WAIT FOR LSN%' ORDER BY query COLLATE "C"; diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c index cdc02b274ff..628f4bb0a48 100644 --- a/src/backend/nodes/makefuncs.c +++ b/src/backend/nodes/makefuncs.c @@ -643,6 +643,7 @@ makeDefElem(char *name, Node *arg, int location) res->arg = arg; res->defaction = DEFELEM_UNSPEC; res->location = location; + res->arg_location = -1; return res; } @@ -662,6 +663,7 @@ makeDefElemExtended(char *nameSpace, char *name, Node *arg, res->arg = arg; res->defaction = defaction; res->location = location; + res->arg_location = -1; return res; } diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c index 094d2872644..f6e47e08e87 100644 --- a/src/backend/nodes/queryjumblefuncs.c +++ b/src/backend/nodes/queryjumblefuncs.c @@ -79,6 +79,9 @@ static void _jumbleVariableSetStmt(JumbleState *jstate, Node *node); static void _jumbleRangeTblEntry_eref(JumbleState *jstate, RangeTblEntry *rte, Alias *expr); +static void _jumbleWaitStmt_options(JumbleState *jstate, + WaitStmt *expr, + List *options); /* * Given a possibly multi-statement source string, confine our attention to the @@ -774,6 +777,63 @@ _jumbleRangeTblEntry_eref(JumbleState *jstate, JUMBLE_STRING(aliasname); } +/* + * Custom query jumble function for WaitStmt.options. + * + * The WITH clause accepts each option at most once, and the order the options + * are written in does not change what the command does. Jumble the options we + * know in a fixed order, so that a wait written as "WITH (mode 'm', timeout + * 't')" and one written as "WITH (timeout 't', mode 'm')" land in the same + * entry. + * + * TIMEOUT only bounds how long the command may wait, so it is recorded as a + * constant and normalized away; waits differing only in their timeout are + * counted together. MODE and NO_THROW choose what the command does, so their + * values are jumbled as given. An unrecognized option fails at execution + * time, but jumble it too, so that distinct failures do not collide. + */ +static void +_jumbleWaitStmt_options(JumbleState *jstate, WaitStmt *expr, List *options) +{ + static const char *const knownOptions[] = {"mode", "no_throw", "timeout"}; + ListCell *lc; + + for (int i = 0; i < lengthof(knownOptions); i++) + { + foreach(lc, options) + { + DefElem *defel = lfirst_node(DefElem, lc); + + if (strcmp(defel->defname, knownOptions[i]) != 0) + continue; + + AppendJumble(jstate, (const unsigned char *) defel->defname, + strlen(defel->defname) + 1); + + if (strcmp(defel->defname, "timeout") == 0) + RecordConstLocation(jstate, false, defel->arg_location, -1); + else + _jumbleNode(jstate, defel->arg); + } + } + + foreach(lc, options) + { + DefElem *defel = lfirst_node(DefElem, lc); + bool known = false; + + for (int i = 0; i < lengthof(knownOptions); i++) + known = known || strcmp(defel->defname, knownOptions[i]) == 0; + + if (!known) + { + AppendJumble(jstate, (const unsigned char *) defel->defname, + strlen(defel->defname) + 1); + _jumbleNode(jstate, defel->arg); + } + } +} + /* * CompLocation: comparator for qsorting LocationLen structs by location */ diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 0563453fe24..a8bfc6fa9f6 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -1187,6 +1187,7 @@ utility_option_elem: utility_option_name utility_option_arg { $$ = makeDefElem($1, $2, @1); + $$->arg_location = @2; } ; diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 0debcd193ab..43d2eff26fa 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -848,6 +848,7 @@ typedef struct DefElem * TypeName */ DefElemAction defaction; /* unspecified action, or SET/ADD/DROP */ ParseLoc location; /* token location, or -1 if unknown */ + ParseLoc arg_location; /* location of arg, or -1 if unknown */ } DefElem; /* @@ -4450,7 +4451,7 @@ typedef struct WaitStmt /* LSN string from grammar */ char *lsn_literal pg_node_attr(query_jumble_ignore); /* List of DefElem nodes */ - List *options; + List *options pg_node_attr(custom_query_jumble); /* token location, or -1 if unknown */ ParseLoc lsn_location pg_node_attr(query_jumble_location); } WaitStmt; -- 2.55.0