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

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