Re: ON CONFLICT DO UPDATE for partitioned tables

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com>, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>, Andres Freund <andres(at)anarazel(dot)de>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Peter Geoghegan <pg(at)bowt(dot)ie>
Subject: Re: ON CONFLICT DO UPDATE for partitioned tables
Date: 2018-03-26 14:20:16
Message-ID: 20180326142016.m4st5e34chrzrknk@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Pushed now.

Amit Langote wrote:
> On 2018/03/24 9:23, Alvaro Herrera wrote:

> > To fix this, I had to completely rework the "get partition parent root"
> > stuff into "get list of ancestors of this partition".
> I wondered if a is_partition_ancestor(partrelid, ancestorid) isn't enough
> instead of creating a list of ancestors and then looping over it as you've
> done, but maybe what you have here is fine.

Yeah, I wondered about doing it that way too (since you can stop looking
early), but decided that I didn't like repeatedly opening pg_inherits
for each level. Anyway the most common case is a single level, and in
rare cases two levels ... I don't think we're going to see much more
than that. So it doesn't matter too much. We can refine later anyway,
if this becomes a hot spot (I doubt it TBH).

> > * General code style improvements, comment rewording, etc.
> There was one comment in Fujita-san's review he posted on Friday [1] that
> doesn't seem to be addressed in v10, which I think we probably should. It
> was this comment:
> "ExecBuildProjectionInfo is called without setting the tuple descriptor of
> mtstate->mt_conflproj to tupDesc. That might work at least for now, but I
> think it's a good thing to set it appropriately to make that future proof."
> All of his other comments seem to have been taken care of in v10. I have
> fixed the above one in the attached updated version.

I was of two minds about this item myself; we don't use the tupdesc for
anything at that point and I expect more things would break if we
required that. But I don't think it hurts, so I kept it.

The one thing I wasn't terribly in love with is the four calls to
map_partition_varattnos(), creating the attribute map four times ... but
we already have it in the TupleConversionMap, no? Looks like we could
save a bunch of work there.

And a final item is: can we have a whole-row expression in the clauses?
We currently don't handle those either, not even to throw an error.
[figures a test case] ... and now that I test it, it does crash!

create table part (a int primary key, b text) partition by range (a);
create table part1 (b text, a int not null);
alter table part attach partition part1 for values from (1) to (1000);
insert into part values (1, 'two') on conflict (a)
do update set b = format('%s (was %s)', excluded.b, part.b)
where part.* *<> (1, text 'two');

I think this means we should definitely handle found_whole_row. (If you
create part1 in the normal way, it works as you'd expect.)

I'm going to close a few other things first, then come back to this.

Álvaro Herrera
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Claudio Freire 2018-03-26 14:26:51 Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently
Previous Message David Steele 2018-03-26 14:19:57 Re: Re: [PATCH] Add missing type conversion functions for PL/Python