Re: Row pattern recognition

Hi Jian,

> To make the preceding ALTER TABLE ADD COLUMN fail, we must recursively
> find all directly and indirectly associated views that contain an RPR
> query,
> and that view's RPR DEFINE clause already references this new column name.
> Making this bulletproof doesn't seem easy. We also need to consider
> ALTER TABLE RENAME COLUMN.

I agree that approach is hard, and I don't think we need it.

Where a view depends on a column outright, PostgreSQL does block the
DDL: DROP COLUMN and ALTER COLUMN TYPE both fail on the dependency.
But it has not taken that route for name drift. ADD COLUMN and RENAME
COLUMN are allowed even when they change which column a bare name
inside a stored view would resolve to. Those are let through, and the
names are pinned at deparse time instead.

- SELECT * is expanded at parse time into a fixed column list
- NATURAL JOIN is not printed as NATURAL; the common columns are
resolved at parse time and printed as an explicit USING list
- a column reference is printed with its table qualifier whenever the
query has more than one range-table entry
- where a name itself has to be pinned, a column alias list is
emitted as soon as that name stops matching the catalog, and if the
freed-up name has been taken by another column, _1, _2, ... are
appended to push the collision aside

DEFINE cannot use the third one. A qualified reference is not legal
there (the qualifier slot names a pattern variable), and
get_rule_define() in ruleutils.c deparses with varprefix off. So in
DEFINE it is the fourth one that has to do the work.

> The implication of the above is that pg_dump | pg_restore will fail to
> restore view v1.
> This seems unsolvable; also see standard 6.5: Row pattern variables
> and other range variables
> So I added a warning in doc/src/sgml/ref/select.sgml.

The fourth one already handles a situation of the same shape. Copy and
run:

CREATE TABLE t1 (id int, val int);
CREATE TABLE t2 (id int, val int);
CREATE VIEW v AS SELECT * FROM t1 JOIN t2 USING (id, val);

-- the column the view's USING refers to moves out of the way
ALTER TABLE t2 RENAME COLUMN val TO valx;
-- and a different val shows up
ALTER TABLE t2 ADD COLUMN val int;

SELECT pg_get_viewdef('v'::regclass, true);

gives

SELECT t1.id,
t1.val
FROM t1
JOIN t2 t2(id, val, val_1) USING (id, val);

The newcomer is pushed to val_1, and the name val in the view text
still resolves to t2's original column, now called valx -- the join
still executes as t1.val = t2.valx. Running that text back gives an
identical view. No DDL was blocked, and the renaming happens purely at
deparse time.

This is not something I invented; it is already covered by the
regression tests. It is the tt3 case in
src/test/regress/sql/create_view.sql, whose expected output in
create_view.out is

JOIN tt3 tt3(ax, b, c, c_1, e) USING (b, c);

after tt3's c is renamed to d and a fresh c and e are then added, so
tt3's real columns are ax, b, d, c, e. The third alias is the renamed
d, held under the name the view's USING still needs, and the newly
added c is pushed aside to c_1. The renaming is done by
make_colname_unique() in ruleutils.c, driven by
set_relation_column_names().

Your case looks like the same shape. It currently deparses to

SELECT
FROM t1,
t2
WHERE t1.id = t2.id
WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
AFTER MATCH SKIP PAST LAST ROW
INITIAL
PATTERN (a)
DEFINE
a AS val > 0);

and that text does not reparse, exactly as you say
(ERROR: column reference "val" is ambiguous). But with one column
alias list on t2,

t2 t2(id, val_1)

the same text creates the view, a second deparse gives back the same
text, and DEFINE resolves to t1.val. So what the deparser is missing
here is a single token.

One thing that helps is that two DEFINE clauses cannot each want a bare
val from a different relation: that is already ambiguous at creation
time, so it cannot be written in the first place. There is at most one
name to protect per DEFINE reference, which bounds the problem.

Getting there does look awkward, though. The path a USING merged
column takes looks like the one to borrow, but that path is only
switched on for certain join shapes today, and a DEFINE reference would
have to be registered ahead of it in a step that does not exist yet.
Non-relation RTEs, and collisions with a USING name, would each need a
decision of their own. So the machinery is there, but a fair amount
would have to be built around it, which does not feel like something to
bolt on right now. I have also not worked through standard 6.5 in
detail, so I may be missing a requirement it imposes here.

What I would suggest is to keep the select.sgml warning you attached as
a stopgap and clear the simple things first. We have a few problems
piling up that do not have an easy answer; it seems better to get the
easy ones out of the way and then take the hard pile together, with
time to spend on it. If this one lands, the warning can come out then.

> Because
> ERROR: cannot use CURRENT ROW as frame end with row pattern recognition
>
> { RANGE | ROWS | GROUPS } frame_start [ frame_exclusion ] [
> row_pattern_common_syntax ]
> is not supported. we can remove row_pattern_common_syntax from here.

Agreed. Every spelling of a bare frame_start is rejected: either the
start is not CURRENT ROW, or it is and frame_end then defaults to
CURRENT ROW, which is rejected in turn.

One thing to add, for later: BETWEEN CURRENT ROW AND CURRENT ROW is not
a problem for the machinery itself. What rules it out is that the
standard limits the frame end to UNBOUNDED FOLLOWING or an offset
FOLLOWING of at least one, and for the same reason we reject 0
FOLLOWING at run time. A single-row match is not meaningless, so I
wonder whether it would be worth proposing to the standard that BETWEEN
CURRENT ROW AND CURRENT ROW, and 0 FOLLOWING, be allowed.

Best regards,
Henson