Re: Fwd: Problem with a "complex" upsert

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Mario De Frutos Dieguez <mariodefrutos(at)gmail(dot)com>, pgsql-bugs(at)lists(dot)postgresql(dot)org
Subject: Re: Fwd: Problem with a "complex" upsert
Date: 2018-08-03 10:17:05
Message-ID: 8e5d96df-95d0-ffdd-46cb-784da36b0e14@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-admin pgsql-bugs

Thanks Dean for taking a look.

On 2018/08/03 18:40, Dean Rasheed wrote:
> On 3 August 2018 at 07:52, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> This doesn't look right to me. It breaks the following case ...

Hmm yeah, matching view and base relation names like that was silly.

> Here's an updated patch that fixes this.
>
>> I also don't see why it should reject columns from the view that
>> aren't in the base relation.
>
> This patch also allows access to view columns that aren't in the
> underlying base relation. The rationale for the result in the new test
> case where it attempts to insert (1,'y') into columns (aa,bb) of the
> view is that the new view row that would have resulted if the insert
> had succeeded is ('y',1,(1,'y')), hence that's what excluded.* should
> be for the view in the "on conflict" action, and there should be no
> problem referring to any part of that excluded view tuple.

Ah, I see what you did there with converting EXCLUDED column references.
I had tried to do the coversion from view attnos to base rel attnos using
tupconvert.c. When fiddling with that, I had to install that restriction
of not allowing accessing view's own columns via EXCLUDED, *because of*
trying to convert using tupconvert.c. Somehow, I had also became
convinced that restricting it like that made sense semantically, which as
you've shown, it doesn't.

After seeing your first email, I had started replacing the tupconvert.c
based conversion (which wouldn't even be readily backpatchable to 9.5) to
ReplaceVarsFromTargetList one, but you beat me to it.

Your updated version looks good and also nice that it has more tests. One
thing that stood out to me was how column references via EXCLUDED now
refer to base rel column names, but I guess that's fine.

create table foo (a int unique, b int);
create view foo_view as select b as bb, a + 1 as cc, a as aa from foo;

explain insert into foo_view (aa, bb) select 1, 1 on conflict (aa) do
update set bb = excluded.bb where excluded.cc > 1;
QUERY PLAN
─────────────────────────────────────────────────
Insert on foo (cost=0.00..0.01 rows=1 width=8)
Conflict Resolution: UPDATE
Conflict Arbiter Indexes: foo_a_key
Conflict Filter: ((excluded.a + 1) > 1)
-> Result (cost=0.00..0.01 rows=1 width=8)
(5 rows)

explain insert into foo_view (aa, bb) select 1, 1 on conflict (aa) do
update set bb = excluded.bb where excluded.aa > 1;
QUERY PLAN
─────────────────────────────────────────────────
Insert on foo (cost=0.00..0.01 rows=1 width=8)
Conflict Resolution: UPDATE
Conflict Arbiter Indexes: foo_a_key
Conflict Filter: (excluded.a > 1)
-> Result (cost=0.00..0.01 rows=1 width=8)
(5 rows)

Thanks again.

Regards,
Amit

In response to

Browse pgsql-admin by date

  From Date Subject
Next Message Achilleas Mantzios 2018-08-03 11:48:35 logical replication problems (10.4)
Previous Message Dean Rasheed 2018-08-03 09:40:50 Re: Fwd: Problem with a "complex" upsert

Browse pgsql-bugs by date

  From Date Subject
Next Message PG Bug reporting form 2018-08-03 11:45:16 BUG #15309: ERROR: catalog is missing 1 attribute(s) for relid 760676 when max_parallel_maintenance_workers > 0
Previous Message Dean Rasheed 2018-08-03 09:40:50 Re: Fwd: Problem with a "complex" upsert