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

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 <assam258(at)gmail(dot)com> 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