[PATCH] Fix quotation logic for unreserved keywords in window specifications
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253043psql -h localhost -U postgresBuilt from patchset v4 (message #4), August 18, 2026 at 10:25 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253043_4 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253043_4 && git checkout t253043_4Patchset v4 (message #4) is on t253043_4
Hi hackers,
I was testing views with pg_dump/pg_restore and found a case where an
unreserved keyword is not quoted correctly.
example:
-- Just a base table
CREATE TABLE t (v int);
INSERT INTO t VALUES (1), (2);
-- Create a view
CREATE VIEW v AS
SELECT count(*) OVER w2 FROM t
WINDOW "rows" AS (PARTITION BY v), w2 AS ("rows" ORDER BY v);
But the problem is, here, that quotation is gone.
-- Problem: Quotation is disappared
SELECT pg_get_viewdef('v');
pg_get_viewdef
-------------------------------------------------------------
SELECT count(*) OVER w2 AS count +
FROM t +
WINDOW rows AS (PARTITION BY v), w2 AS (rows ORDER BY v);
(1 row)
-- If you use this for recovering the view above, You'll face this error.
SELECT count(*) OVER w2 AS count
FROM t
WINDOW rows AS (PARTITION BY v), w2 AS (rows ORDER BY v);
ERROR: syntax error at or near "ORDER"
LINE 3: WINDOW rows AS (PARTITION BY v), w2 AS (rows ORDER BY v);
This breaks pg_dump/restore: the view is lost.
As a result, pg_dump/pg_restore cannot restore the view successfully.
Fortunately, I found this comment in the gram.y:
It says:
* If we see PARTITION, RANGE, ROWS or GROUPS as the first token after the
'('
* of a window_specification, we want the assumption to be that there is
* no existing_window_name; but those keywords are unreserved and so could
* be ColIds.
I think that assumption can be broken when inheritance is used
across multiple window definitions.
I mean this: w2 AS ("rows" ORDER BY v)
Some comments for patch file:
The attached patch quotes the window name when it is a keyword
in get_rule_windowspec().
Output for ordinary window names is unchanged. The window definition itself
is a ColId so no change is needed there; only the refname is quoted.
Maybe this is a known thing on the team's side, but I think, at least,
should not break
compatibility with current tools. That is the motivation of this patch...
I'd appreciate any feedback or suggestions...!!
Best regards...
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>님이 작성:
Show quoted text
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