Thank you for the corrections. Here’s revision 3 (attached). As you pointed out, the numbered topics are below, with my comments.
1. A back-reference in gram.y It would be good to prevent this with a comment. I’ve slightly revised the sentence, but the intended meaning remains the same. 2. The first line of the comment in ruleutils.c I’ve rewritten this part of the text. Yes, it could be confusing. 3. About the tests Yes. I've removed the TEMP and added a test case and reproduced the issue as described. I was able to verify the same results on my end. Thanks again for your thoughtful review. Best regards... On Thu, Aug 20, 2026 at 11:35 AM Henson Choi <[email protected]> wrote: > > Hi Kwangwon, > > Thanks for v2. Narrowing the quoting to the four names the grammar > actually rejects is what I was hoping for, and I have checked that four > is the complete set: after opt_existing_window_name the grammar can only > see PARTITION, ORDER, RANGE, ROWS, GROUPS or ')', and ORDER is reserved, > so quote_identifier() already quotes it. > > I have three requests. With those applied I am happy with the patch. > > 1. A back-reference in gram.y > > Choosing (b) turns this into an invariant spanning two files, and only > one direction of it is documented. The new comment in ruleutils.c > points at gram.y, but that is the safe direction: someone reading the > list already wants to know why those four names are there. The > direction that bites is the other one. Someone adding a keyword to the > exclusion has no reason to open ruleutils.c at all -- which is exactly > how we got here: the commit that stopped quoting unreserved keywords and > the one that added window functions never knew about each other. > Nothing enforces the invariant at compile time or in the tests, so a > comment is the only guard we get. > > It is also about to be exercised: the Row Pattern Recognition patch > (CF 4460) adds PATTERN, AFTER, INITIAL and SEEK to this same exclusion. > > I would suggest appending this to the existing comment above > opt_existing_window_name: > > * ruleutils.c must quote these names when deparsing an existing_window_name; > * see appendWindowRefName() there. Keep that list in sync with this one, or > * a view with a window of that name will not survive dump and restore. > > 2. The first line of the comment in ruleutils.c > > The header line says: > > * Emit the name of the window definition. > > but the function emits wc->refname, which is the reference, not the > definition -- and the body of the same comment correctly says > existing_window_name. This matters more than a usual wording nit here, > because the expected output deliberately prints the same identifier two > ways: > > WINDOW rows AS (PARTITION BY v), w2 AS ("rows" ORDER BY v); > > A reader who trusts that first line will read the unquoted definition as > something this patch missed. Something like: > > * Emit the name of the existing window a window specification inherits from. > > 3. The test > > Two things, both of which come from the switch to (b). > > First, under (b) the list *is* the fix, and only "rows" is covered. I > removed each of the four entries in turn and rebuilt: with the current > test, dropping partition, range or groups fails nothing at all. With > the block below, each removal fails the window test. > > Second, the test compares the pg_get_viewdef() string but never reparses > it, and because the view is temporary nothing downstream reparses it > either -- temporary objects are not dumped. Making these regular views > puts them in the regression database, where the dump/restore round-trip > in src/bin/pg_upgrade/t/002_pg_upgrade.pl exercises them for real (that > block is opt-in via PG_TEST_EXTRA=regress_dump_restore). > > I applied only the test change to a tree without the patch. The > round-trip fails there with exactly the symptom you reported in your > first mail: > > pg_restore: error: could not execute query: ERROR: syntax error at or near > "ORDER" > pg_restore: error: could not execute query: ERROR: relation > "public.v_window_kw_groups" does not exist > > Proposed replacement for the block in window.sql: > > -- PARTITION, RANGE, ROWS and GROUPS are not accepted bare as an existing > -- window name, so deparse has to quote them there. The window's own name is > -- a ColId and stays unquoted, hence the asymmetry below. > CREATE VIEW v_window_kw_partition AS > SELECT count(*) OVER w2 FROM generate_series(1, 1) s(v) > WINDOW "partition" AS (PARTITION BY v), w2 AS ("partition" ORDER BY v); > > SELECT pg_get_viewdef('v_window_kw_partition'); > > CREATE VIEW v_window_kw_range AS > SELECT count(*) OVER w2 FROM generate_series(1, 1) s(v) > WINDOW "range" AS (PARTITION BY v), w2 AS ("range" ORDER BY v); > > SELECT pg_get_viewdef('v_window_kw_range'); > > CREATE VIEW v_window_kw_rows 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('v_window_kw_rows'); > > CREATE VIEW v_window_kw_groups AS > SELECT count(*) OVER w2 FROM generate_series(1, 1) s(v) > WINDOW "groups" AS (PARTITION BY v), w2 AS ("groups" ORDER BY v); > > SELECT pg_get_viewdef('v_window_kw_groups'); > > Dropping TEMP is the part that matters. This is a dump/restore bug, > and a temporary view is never dumped -- so for as long as the view is > temporary the test can only compare a string, and can never exercise the > failure it is about. > > I ran make check (245 tests) and the pg_upgrade TAP tests, including the > opt-in dump/restore round-trip, with all of the above applied and > everything passes. > > Best regards, > Henson Choi
From 36f9a04118c2ffc5fa75f2eba5463f335d0b0594 Mon Sep 17 00:00:00 2001 From: Kwangwon Seo <[email protected]> Date: Thu, 20 Aug 2026 18:36:11 +0900 Subject: [PATCH v3] 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/parser/gram.y | 4 +++ src/backend/utils/adt/ruleutils.c | 32 ++++++++++++++++++- src/test/regress/expected/window.out | 47 ++++++++++++++++++++++++++++ src/test/regress/sql/window.sql | 27 ++++++++++++++++ 4 files changed, 109 insertions(+), 1 deletion(-) diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 17035fb4d15..5b13f7dc1b7 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -17452,6 +17452,10 @@ window_specification: '(' opt_existing_window_name opt_partition_clause * that the shift/reduce conflict is resolved in favor of reducing the rule. * These keywords are thus precluded from being an existing_window_name but * are not reserved for any other purpose. + * + * To preserve quoting in views during dump and restore, ruleutils.c must quote + * those keywords when deparsing an existing_window_name. + * See appendWindowRefName(). */ opt_existing_window_name: ColId { $$ = $1; } | /*EMPTY*/ %prec Op { $$ = NULL; } diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index e95dd1b11eb..ef95e37b9f2 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 existing window a window specification inherits from. + * + * 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..32a89bf4c3e 100644 --- a/src/test/regress/expected/window.out +++ b/src/test/regress/expected/window.out @@ -1361,6 +1361,53 @@ SELECT pg_get_viewdef('v_window'); FROM generate_series(now(), (now() + '@ 100 days'::interval), '@ 1 hour'::interval) i(i); (1 row) +-- PARTITION, RANGE, ROWS and GROUPS are not accepted bare as an existing +-- window name, so deparse has to quote them there. The window's own name is +-- a ColId and stays unquoted, hence the asymmetry below. +CREATE VIEW v_window_kw_partition AS + SELECT count(*) OVER w2 FROM generate_series(1, 1) s(v) + WINDOW "partition" AS (PARTITION BY v), w2 AS ("partition" ORDER BY v); +SELECT pg_get_viewdef('v_window_kw_partition'); + pg_get_viewdef +------------------------------------------------------------------------- + SELECT count(*) OVER w2 AS count + + FROM generate_series(1, 1) s(v) + + WINDOW partition AS (PARTITION BY v), w2 AS ("partition" ORDER BY v); +(1 row) + +CREATE VIEW v_window_kw_range AS + SELECT count(*) OVER w2 FROM generate_series(1, 1) s(v) + WINDOW "range" AS (PARTITION BY v), w2 AS ("range" ORDER BY v); +SELECT pg_get_viewdef('v_window_kw_range'); + pg_get_viewdef +----------------------------------------------------------------- + SELECT count(*) OVER w2 AS count + + FROM generate_series(1, 1) s(v) + + WINDOW range AS (PARTITION BY v), w2 AS ("range" ORDER BY v); +(1 row) + +CREATE VIEW v_window_kw_rows 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('v_window_kw_rows'); + 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) + +CREATE VIEW v_window_kw_groups AS + SELECT count(*) OVER w2 FROM generate_series(1, 1) s(v) + WINDOW "groups" AS (PARTITION BY v), w2 AS ("groups" ORDER BY v); +SELECT pg_get_viewdef('v_window_kw_groups'); + pg_get_viewdef +------------------------------------------------------------------- + SELECT count(*) OVER w2 AS count + + FROM generate_series(1, 1) s(v) + + WINDOW groups AS (PARTITION BY v), w2 AS ("groups" 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..ea35b605545 100644 --- a/src/test/regress/sql/window.sql +++ b/src/test/regress/sql/window.sql @@ -330,6 +330,33 @@ CREATE TEMP VIEW v_window AS SELECT pg_get_viewdef('v_window'); +-- PARTITION, RANGE, ROWS and GROUPS are not accepted bare as an existing +-- window name, so deparse has to quote them there. The window's own name is +-- a ColId and stays unquoted, hence the asymmetry below. +CREATE VIEW v_window_kw_partition AS + SELECT count(*) OVER w2 FROM generate_series(1, 1) s(v) + WINDOW "partition" AS (PARTITION BY v), w2 AS ("partition" ORDER BY v); + +SELECT pg_get_viewdef('v_window_kw_partition'); + +CREATE VIEW v_window_kw_range AS + SELECT count(*) OVER w2 FROM generate_series(1, 1) s(v) + WINDOW "range" AS (PARTITION BY v), w2 AS ("range" ORDER BY v); + +SELECT pg_get_viewdef('v_window_kw_range'); + +CREATE VIEW v_window_kw_rows 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('v_window_kw_rows'); + +CREATE VIEW v_window_kw_groups AS + SELECT count(*) OVER w2 FROM generate_series(1, 1) s(v) + WINDOW "groups" AS (PARTITION BY v), w2 AS ("groups" ORDER BY v); + +SELECT pg_get_viewdef('v_window_kw_groups'); + -- test overflow frame specifications SELECT sum(unique1) over (rows between current row and 9223372036854775807 following exclude current row), unique1, four -- 2.52.0
