Re: ON CONFLICT DO UPDATE for partitioned tables

On 2018/03/26 23:20, Alvaro Herrera wrote:
> Pushed now.

Thank you!

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

Yes, I suppose.

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

Hmm, actually we can't use that map, assuming you're talking about the
following map:

TupleConversionMap *map = proute->parent_child_tupconv_maps[partidx];

We can only use that to tell if we need converting expressions (as we
currently do), but it cannot be used to actually convert the expressions.
The map in question is for use by do_convert_tuple(), not to map varattnos
in Vars using map_variable_attnos().

But it's definitely a bit undesirable to have various
map_partition_varattnos() calls within ExecInitPartitionInfo() to
initialize the same information (the map) multiple times.

> 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 agree. That means we simply remove the Assert after the
map_partition_varattnos call.

> I'm going to close a few other things first, then come back to this.

Attached find a patch to fix the whole-row expression issue. I added your
test to insert_conflict.sql.

Thanks,
Amit