[PATCH] Fix quotation logic for unreserved keywords in window specifications

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.

Best regards,
Henson

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

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 <ishii@postgresql.org>님이 작성:

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

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...

Hi Kwangwon,

Thanks for v3. All three points are addressed, and I have checked the
list once more against the grammar: of the nine tokens sharing IDENT's
precedence, only the four that can begin a clause in that position --
PARTITION, RANGE, ROWS and GROUPS -- are rejected bare; the rest
round-trip unquoted, so the four in appendWindowRefName() are exactly
the set that needs quoting. The new window.sql cases pass here.

Best regards,
Henson

Thank you for the corrections. Here’s revision 3 (attached).

I applied v3 patch to current master and ran dump/restore test:

$ cd src/bin/pg_upgrade
$ PG_TEST_EXTRA="regress_dump_restore" make check

Without the patch to ruleutils.c the test failed. With the patch, the
test passed!

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp