Date: Fri, 2 Mar 2018 12:35:58 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: ON CONFLICT DO UPDATE for partitioned tables
Message-ID: <20180302153558.czbufv6ojwdfk4lp@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3c3bbb2c-c28f-5b09-c124-b99e2231fe9b@lab.ntt.co.jp>
User-Agent: NeoMutt/20170306-137-4415bd-dirty (1.8.0)
Precedence: bulk

Amit Langote wrote:

> Actually, after your comment on my original patch [1], I did make it work
> for multiple levels by teaching the partition initialization code to find
> a given partition's indexes that are inherited from the root table (that
> is the table mentioned in command).  So, after a tuple is routed to a
> partition, we switch from the original arbiterIndexes list to the one we
> created for the partition, which must contain OIDs corresponding to those
> in the original list.  After all, for each of the parent's indexes that
> the planner put into the original arbiterIndexes list, there must exist an
> index in each of the leaf partitions.

Oh, your solution for this seems simple enough.  Silly me, I was trying
to implement it in a quite roundabout way.  Thanks.  (I do wonder if we
should save the "root" reloid in the relcache).

> I had also observed when working on the patch that various TupleTableSlots
> used by the ON CONFLICT DO UPDATE code must be based on TupleDesc of the
> inheritance-translated target list (DO UPDATE SET target list).  In fact,
> that has to take into account also the dropped columns; we may have
> dropped columns either in parent or in a partition or in both at same or
> different attnum positions.  That means, simple map_partition_varattnos()
> translation doesn't help in this case.

Yeah, I was aware these corner cases could become a problem though I
hadn't gotten around to testing them yet.  Thanks for all your work on
this.

The usage of the few optimizer/prep/ functions that are currently static
doesn't fill me with joy.  These functions have weird APIs because
they're static so we don't rally care, but once we export them we should
strive to be more careful.  I'd rather stay away from just exporting
them all, so I chose to encapsulate those calls in a single function and
export only expand_targetlist from preptlist.c, keeping the others
static in prepunion.c.  In the attached patch set, I put an API change
(work on tupdescs rather than full-blown relations) for a couple of
those functions as 0001, then your patch as 0002, then a few fixups of
my own.  (0002 is not bit-by-bit identical to yours; I think I had to
fix some merge conflict with 0001, but should be pretty much the same).

But looking further, I think there is much cruft that has accumulated in
those functions (because they've gotten simplified over time), and we
could do some additional cleanup surgery.  For example, there is no
reason to pass a list pointer to make_inh_translation_list(); we could
just return it.  And then we don't have to cons up a fake AppendRelInfo
with all dummy values that adjust_inherited_tlist doesn't even care
about.  I think there was a point for all these contortions back at some
point (visible by checking git history of this code), but it all seems
useless now.

Re. the "ugly hack" comments in adjust_inherited_tlist(), I'm confused:
expand_targetlist() runs *after*, not before, so how could it have
affected the result?  I'm obviously confused about what
expand_targetlist call this comment is talking about.  Anyway I wanted
to make it use resjunk entries instead, but that broke some other case
that I didn't have time to research yesterday.  I'll get back to this
soon, but in the meantime, here's what I have.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services