Thanks for the kind review, Mr. Henson Choi and Mr. Tatsuo Ishii. I also checked commit 4c310eca2ea. Yes, (a) breaks that.
I also think that breaking changes are not desirable for a small patch like this one. So, as Mr. Tatsuo pointed out, this patch (v2) would limit the quoted range. I also added some commit text that I missed in the first patch file. That was my mistake, so it includes the "Reviewed-by" tag and a link to the "Discussion" thread. I hope this commit will be helpful for the work you are currently doing. 2026년 8월 14일 (금) 오전 11:50, Tatsuo Ishii <[email protected]>님이 작성: > Hi Henson, > > > Hi Kwangwon, Tatsuo, > > > > Thanks for the patch -- this is a real dump/restore hazard, and the fix > > is in the right place (get_rule_windowspec, refname only). > > > >> The attached patch quotes the window name when it is a keyword > > > > One thing worth settling: as written this quotes the refname for any > > keyword, but only four names actually break on reparse -- PARTITION, > > RANGE, ROWS, GROUPS, the ones the grammar won't accept unquoted as an > > existing window name (exactly the comment you found). Every other > > keyword still round-trips fine unquoted, so quoting just those four is > > enough; quoting all keywords works too, it is just broader than needed. > > > > For context, this is an old interaction between two commits that never > > knew about each other: 4c310eca2ea (2007) made quote_identifier() stop > > quoting unreserved keywords -- the point being that unreserved keywords > > are usable bare by definition, so quoting them was just unnecessary > > noise -- and 95b07bc7f50 (2008, window functions) later had the grammar > > reject four of those keywords as a bare existing-window-name. So when > > such a keyword lands in that slot as a refname, deparse prints it > > unquoted (quote_identifier skips it as unreserved) while the grammar > > will not take it unquoted -- and reparsing breaks in exactly that gap. > > > > So the choice is: > > > > (a) quote any keyword (what the patch does now) -- simple, but > > over-quotes. > > (b) quote only those four -- minimal, but it needs an explicit list in > > ruleutils kept in sync with the grammar, since no keyword category > > isolates them. > > > > Tatsuo, I would like your read as committer, because this has a > > cross-patch consequence. The Row Pattern Recognition patch (CF 4460) > > adds four more names -- PATTERN, AFTER, INITIAL, SEEK -- to the same > > grammar exclusion, so it hits the same bug. Under (b), RPR would have to > > extend the list itself (plus a round-trip test per name) and depend on > > this patch landing first; under (a), RPR needs nothing here. > > > > That history is also why I lean (b): quoting every keyword on the > > refname (option (a)) re-introduces exactly the broad quoting > > 4c310eca2ea set out to remove, just confined to one slot. (b) quotes > > only the four names the grammar actually rejects, which keeps faith with > > that original intent. If we take (b), then Kwangwon, since you would be > > building that list here anyway, would you be up for helping with the > > matching change on the RPR side (adding those four names to the same > > list in CF 4460)? It is the same mechanism, so doing both keeps the list > > in one place and consistent. > > > > Either way, I am happy to help review it through. > > Thank you for the explanation. I prefer (b) too since (a) breaks > 4c310eca2ea as you said. > > Regards, > -- > Tatsuo Ishii > SRA OSS K.K. > English: http://www.sraoss.co.jp/index_en/ > Japanese:http://www.sraoss.co.jp >
From 45a6893c1161791974c7b00e5f92ac60758c947d Mon Sep 17 00:00:00 2001 From: Kwangwon Seo <[email protected]> Date: Fri, 14 Aug 2026 17:15:19 +0900 Subject: [PATCH v2] Fix quotation logic for unreserved keywords in window specifications When ruleutils.c deparsed a window specification that referenced an existing window, it used quote_identifier() for the referenced window name. The same is not done for unreserved keywords, especially PARTITION, RANGE, ROWS, and GROUPS. Because the grammar treats them as clause starters rather than existing_window_name when they appear at the start of a window specification, pg_get_viewdef() could emit SQL that failed to reparse when the referenced window was named "rows", "range", "groups", or "partition". This commit quotes those names properly when emitting a referenced window name. Author: Kwangwon Seo <[email protected]> Reviewed-by: Henson Choi <[email protected]> Reviewed-by: Tatsuo Ishii <[email protected]> Discussion: https://www.postgresql.org/message-id/flat/CAHJxwBWx_v%3DaWp7ZrGRFw2r_7MJdxYX2um3ZOmxTg4c_tL1qLA%40mail.gmail.com --- src/backend/utils/adt/ruleutils.c | 32 +++++++++++++++++++++++++++- src/test/regress/expected/window.out | 12 +++++++++++ src/test/regress/sql/window.sql | 7 ++++++ 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index e95dd1b11eb..c605095694a 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -450,6 +450,7 @@ static void get_rule_orderby(List *orderList, List *targetList, static void get_rule_windowclause(Query *query, deparse_context *context); static void get_rule_windowspec(WindowClause *wc, List *targetList, deparse_context *context); +static void appendWindowRefName(StringInfo buf, const char *refname); static void get_window_frame_options(int frameOptions, Node *startOffset, Node *endOffset, deparse_context *context); @@ -7158,7 +7159,7 @@ get_rule_windowspec(WindowClause *wc, List *targetList, appendStringInfoChar(buf, '('); if (wc->refname) { - appendStringInfoString(buf, quote_identifier(wc->refname)); + appendWindowRefName(buf, wc->refname); needspace = true; } /* partition clauses are always inherited, so only print if no refname */ @@ -7200,6 +7201,35 @@ get_rule_windowspec(WindowClause *wc, List *targetList, appendStringInfoChar(buf, ')'); } +/* + * Emit the name of the window definition. + * + * PARTITION, RANGE, ROWS, and GROUPS have the same precedence as IDENT + * at the start of a window specification, preventing them from being + * recognized as an existing_window_name (see opt_existing_window_name + * in gram.y). Since these are unreserved keywords, quote_identifier() + * does not quote them, causing the generated SQL to fail when reparsed. + * Therefore, quote these keywords here. + */ +static void +appendWindowRefName(StringInfo buf, const char *refname) +{ + const char *quoted = quote_identifier(refname); + + if (quoted == refname && + (strcmp(refname, "partition") == 0 || + strcmp(refname, "range") == 0 || + strcmp(refname, "rows") == 0 || + strcmp(refname, "groups") == 0)) + { + appendStringInfoChar(buf, '"'); + appendStringInfoString(buf, refname); + appendStringInfoChar(buf, '"'); + } + else + appendStringInfoString(buf, quoted); +} + /* * Append the description of a window's framing options to context->buf */ diff --git a/src/test/regress/expected/window.out b/src/test/regress/expected/window.out index c0bde1c5eec..5080b415e0b 100644 --- a/src/test/regress/expected/window.out +++ b/src/test/regress/expected/window.out @@ -1361,6 +1361,18 @@ SELECT pg_get_viewdef('v_window'); FROM generate_series(now(), (now() + '@ 100 days'::interval), '@ 1 hour'::interval) i(i); (1 row) +-- A window name that is an unreserved keyword cannot be an existing_window_name +CREATE TEMP VIEW v2_window_unreserved_kw AS + SELECT count(*) OVER w2 FROM generate_series(1, 1) s(v) + WINDOW "rows" AS (PARTITION BY v), w2 AS ("rows" ORDER BY v); +SELECT pg_get_viewdef('v2_window_unreserved_kw'); + pg_get_viewdef +--------------------------------------------------------------- + SELECT count(*) OVER w2 AS count + + FROM generate_series(1, 1) s(v) + + WINDOW rows AS (PARTITION BY v), w2 AS ("rows" ORDER BY v); +(1 row) + -- test overflow frame specifications SELECT sum(unique1) over (rows between current row and 9223372036854775807 following exclude current row), unique1, four diff --git a/src/test/regress/sql/window.sql b/src/test/regress/sql/window.sql index 8e6f92d94c7..396b95d0d39 100644 --- a/src/test/regress/sql/window.sql +++ b/src/test/regress/sql/window.sql @@ -330,6 +330,13 @@ CREATE TEMP VIEW v_window AS SELECT pg_get_viewdef('v_window'); +-- A window name that is an unreserved keyword cannot be an existing_window_name +CREATE TEMP VIEW v2_window_unreserved_kw AS + SELECT count(*) OVER w2 FROM generate_series(1, 1) s(v) + WINDOW "rows" AS (PARTITION BY v), w2 AS ("rows" ORDER BY v); + +SELECT pg_get_viewdef('v2_window_unreserved_kw'); + -- test overflow frame specifications SELECT sum(unique1) over (rows between current row and 9223372036854775807 following exclude current row), unique1, four -- 2.52.0
