Re: ON CONFLICT DO UPDATE for partitioned tables

On 2018/03/03 0:36, Alvaro Herrera wrote:
> 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).

Do you mean store the root reloid for "any" partition (table or index)?

>> 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.

Yeah, I think it might as well work to fix up these interfaces the way
you've done.

> 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.

Hmm. I can imagine how the newly added comments in
adjust_inherited_tlist() may be confusing. With this patch, we're now
calling adjust_inherited_tlist() from the executor, whereas it was
designed only to be run in the planner prep phase. What we're passing to
it from the executor is the DO UPDATE SET's targetlist that has undergone
the expand_targetlist() treatment by the planner. Maybe, we need to
update the adjust_inherited_tlist() comments to reflect the expansion of
its scope due to this patch.

Some comments on 0003:

+ * Fist, fix the target entries' resnos, by using inheritance
translation.

First

+ appinfo.parent_reltype = InvalidOid; // parentRel->rd_rel->reltype;

I guess you won't retain that comment. :)

+ result_tl = expand_targetlist(result_tl, CMD_UPDATE,

Should we add a CmdType argument to adjust_and_expand_partition_tlist()
and use it instead of hard-coding CMD_UPDATE here?

Thanks,
Amit