|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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  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 https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
|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|