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

From: Henson Choi <assam258(at)gmail(dot)com>
To: Kwangwon Seo <anchovyseo(at)gmail(dot)com>, Tatsuo Ishii <ishii(at)postgresql(dot)org>, jian he <jian(dot)universality(at)gmail(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: [PATCH] Fix quotation logic for unreserved keywords in window specifications
Date: 2026-07-24 05:11:07
Message-ID: CAAAe_zDSX8ewrwss=C_Z3qo=JrtW2zjKQVG+H63TkWYDk2zv7Q@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Hayato Kuroda (Fujitsu) 2026-07-24 05:19:10 RE: sequencesync worker race with REFRESH SEQUENCES
Previous Message solai v 2026-07-24 04:50:22 Re: convert various variables to atomics