Date: Mon, 26 Mar 2018 11:20:16 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Pavan Deolasee <pavan.deolasee@gmail.com>,
	Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>,
	Andres Freund <andres@anarazel.de>,
	Pg Hackers <pgsql-hackers@postgresql.org>,
	Peter Geoghegan <pg@bowt.ie>
Subject: Re: ON CONFLICT DO UPDATE for partitioned tables
Message-ID: <20180326142016.m4st5e34chrzrknk@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <c8c23eab-4c86-d3ff-f37e-1bc7e0b2a9d8@lab.ntt.co.jp>
User-Agent: NeoMutt/20170306-137-4415bd-dirty (1.8.0)
Precedence: bulk

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