pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedFrom: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Pavan Deolasee <pavan.deolasee@gmail.com>
Cc: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
Cc: Andres Freund <andres@anarazel.de>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Cc: Peter Geoghegan <pg@bowt.ie>
Subject: Re: ON CONFLICT DO UPDATE for partitioned tables
Date: Mon, 26 Mar 2018 11:20:16 -0300
Message-ID: <20180326142016.m4st5e34chrzrknk@alvherre.pgsql> (raw)
In-Reply-To: <c8c23eab-4c86-d3ff-f37e-1bc7e0b2a9d8@lab.ntt.co.jp>
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 https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
view thread (53+ messages) latest in thread
Message-ID: <20180326142016.m4st5e34chrzrknk@alvherre.pgsql>
Permalink: ../20180326142016.m4st5e34chrzrknk@alvherre.pgsql/
Also on: postgresql.org/message-id/20180326142016.m4st5e34chrzrknk@alvherre.pgsql
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, Langote_Amit_f8@lab.ntt.co.jp, pavan.deolasee@gmail.com, fujita.etsuro@lab.ntt.co.jp, andres@anarazel.de, pg@bowt.ie
Subject: Re: ON CONFLICT DO UPDATE for partitioned tables
In-Reply-To: <20180326142016.m4st5e34chrzrknk@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox