Re: BUG #15623: Inconsistent use of default for updatable view

On Tue, 12 Feb 2019 at 10:33, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> Here's an updated patch ...

So I pushed that. However, ...

Playing around with it some more, I realised that whilst this appeared
to fix the reported problem, it exposes another issue which is down to
the interaction between rewriteTargetListIU() and rewriteValuesRTE()
--- for an INSERT with a VALUES RTE, rewriteTargetListIU() computes a
list of target attribute numbers (attrno_list) from the targetlist in
its original order, which rewriteValuesRTE() then relies on being the
same length (and in the same order) as each of the lists in the VALUES
RTE. That's OK for the initial invocation of those functions, but it
breaks down when they're recursively invoked for auto-updatable views.

For example, the following test-case, based on a slight variation of
the new regression tests:

create table foo (a int default 1, b int);
create view foo_v as select * from foo;
alter view foo_v alter column b set default 2;
insert into foo_v values (default), (default);

triggers the following Assert in rewriteValuesRTE():

/* Check list lengths (we can assume all the VALUES sublists are alike) */
Assert(list_length(attrnos) == list_length(linitial(rte->values_lists)));

What's happening is that the initial targetlist, which contains just 1
entry for the column a, gains another entry to set the default for
column b from the view. Then, when it recurses into
rewriteTargetListIU()/rewriteValuesRTE() for the base relation, the
size of the targetlist (now 2) no longer matches the sizes of the
VALUES RTE lists (1).

I think that that failed Assert was unreachable prior to 41531e42d3,
because the old version of rewriteValuesRTE() would always replace all
unresolved DEFAULT items with NULLs, so when it recursed into
rewriteValuesRTE() for the base relation, it would always bail out
early because there would be no DEFAULT items left, and so it would
fail to notice the list size mismatch.

My first thought was that this could be fixed by having
rewriteTargetListIU() compute attrno_list using only those targetlist
entries that refer to the VALUES RTE, and thus omit any new entries
added due to view defaults. That doesn't work though, because that's
not the only way that a list size mismatch can be triggered --- it's
also possible that rewriteTargetListIU() will merge together
targetlist entries, for example if they're array element assignments
referring to the same column, in which case the rewritten targetlist
could be shorter than the VALUES RTE lists, and attempting to compute
attrno_list from it correctly would be trickier.

So actually, I think the right way to fix this is to give up trying to
compute attrno_list in rewriteTargetListIU(), and have
rewriteValuesRTE() work out the attribute assignments itself for each
column of the VALUES RTE by examining the rewritten targetlist. That
looks to be quite straightforward, and results in a cleaner separation
of logic between the 2 functions, per the attached patch.

I think that rewriteValuesRTE() should only ever see DEFAULT items for
columns that are simple assignments to columns of the target relation,
so it only needs to work out the target attribute numbers for TLEs
that contain simple Vars referring to the VALUES RTE. Any DEFAULT seen
in a column not matching that would be an error, but actually I think
such a thing ought to be a "can't happen" error because of the prior
checks during parse analysis, so I've used elog() to report if this
does happen.

Incidentally, it looks like the part of rewriteValuesRTE()'s comment
about subscripted and field assignment has been wrong since
a3c7a993d5, so I've attempted to clarify that. That will need to look
different pre-9.6, I think.

Thoughts?

Regards,
Dean