Re: Fwd: Problem with a "complex" upsert

Thanks for looking at the patch and sorry I couldn't reply sooner.

On 2018/07/11 5:59, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> writes:
>> Having worked a little bit on the ON CONFLICT code recently, I was able to
>> guess at the triggering detail. At least, I was able to reproduce the
>> error and crash seen in the OP's report. Here's a minimal example:
>
>> create table foo (a text unique, b float);
>> insert into foo values ('xyxyxy', 1);
>
>> -- note the different order of columns in the view
>> create view foo_view as select b, a from foo;
>
> Ah-hah.
>
>> I tried debugging why that happens and concluded that rewriteTargetView
>> fails to *completely* account for the fact that the view's column's may
>> have different attribute numbers than the underlying table that the DO
>> UPDATE action will be applied to. Especially, even if the view's Vars are
>> replaced with those corresponding underlying base table's columns, the
>> TargetEntry's into which those Vars are contained are not refreshed, that
>> is, their resnos don't match varattnos.
>
>> I created a patch that seems to fix the issue, which also adds a
>> representative test in updatable_view.sql.
>
> Hm. I looked at this patch a bit. While the onConflictSet change looks
> reasonable, I find the exclRelTlist change fishy. Shouldn't those resnos
> correspond to the exclRelTlist's *own* vars, independently of what is or
> isn't in the view_targetlist? And why is it OK to ignore failure to find
> a match?
>
> The provided test case doesn't seem to me to prove that that code is OK.
> AFAICS, exclRelTlist only gets used by EXPLAIN, and there's no EXPLAIN
> output in the test case.

On further study, I have concluded that EXCLUDED pseudo-relation and any
references to it in the sub-expressions of OnConflictExpr need to be
revised after the rewriter has substituted target view relation with its
underlying base relation.

As things stand today, transformOnConflictClause creates the EXCLUDED
pseudo-relation based on the original target relation of the query, which
in this case is the view relation. rewriteTargetView replaces the view
relation with its underlying base relation, subject to various
restrictions on what the query can then do, such as not being able to
update columns that are not present in the underlying base relation.

On the same lines, I think OnConflictExpr shouldn't be allowed to contain
references to view's own columns via the EXCLUDED pseudo-relation. That's
because ON CONFLICT's execution machinery would be able to access only
those columns of the EXCLUDED tuple that are present in the underlying
physical relation. Hence, to account for the view relation's substitution
with its underlying physical relation, we should discard the original
EXCLUDED range table entry and target list (exclRelTlist) that parser
created based on the view relation and recreate both based on the
substituted base rel. Furthermore, any Vars contained in OnConflictExpr's
sub-expressions that reference original EXCLUDED rte will need to be
substituted with Vars based on the revised rte.

Attaching the updated patch, which is quite heavily revised from the
earlier patch, given the above findings.

Thanks,
Amit