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

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