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
