Re: ON CONFLICT DO UPDATE for partitioned tables - Mailing list pgsql-hackers

Fujita-san,

On 2018/03/19 21:59, Etsuro Fujita wrote:
> (2018/03/18 13:17), Alvaro Herrera wrote:
>> Alvaro Herrera wrote:
>> The only thing that I remain unhappy about this patch is the whole
>> adjust_and_expand_partition_tlist() thing.  I fear we may be doing
>> redundant and/or misplaced work.  I'll look into it next week.
> 
> I'm still reviewing the patches, but I really agree on that point.  As
> Pavan mentioned upthread, the onConflictSet tlist for the root parent,
> from which we create a translated onConflictSet tlist for a partition,
> would have already been processed by expand_targetlist() to contain all
> missing columns as well, so I think we could create the tlist for the
> partition by simply re-ordering the expression-converted tlist (ie,
> conv_setproj) based on the conversion map for the partition.  The Attached
> defines a function for that, which could be called, instead of calling
> adjust_and_expand_partition_tlist().  This would allow us to get rid of
> planner changes from the patches.  Maybe I'm missing something, though.

Thanks for the patch.  I can confirm your proposed
adjust_onconflictset_tlist() is enough to replace adjust_inherited_tlist()
+ expand_targetlist() combination (aka
adjust_and_expand_partition_tlist()), thus rendering the planner changes
in this patch unnecessary.  I tested it with a partition tree involving
partitions of varying attribute numbers (dropped columns included) and it
seems to work as expected (as also exercised in regression tests) as shown
below.

Partitioned table p has partitions p1, p2, p3, p4, and p5 whose attributes
look like this; shown as (colname: attnum, ...).

p:  (a: 1, b: 4)
p1: (a: 1, b: 4)
p2: (a: 2, b: 4)
p3: (a: 1, b: 3)
p4: (a: 3, b: 8)
p5: (a: 1, b: 5)

You may notice that all partitions but p1 will have a tuple conversion map
and hence will undergo adjust_onconflictset_tlist() treatment.

insert into p values (1, 'a') on conflict (a) do update set b = excluded.b
where excluded.b = 'b';
INSERT 0 1

insert into p values (1, 'b') on conflict (a) do update set b = excluded.b
where excluded.b = 'b';
INSERT 0 1

insert into p values (1, 'c') on conflict (a) do update set b = excluded.b
where excluded.b = 'b';
INSERT 0 0

insert into p values (1) on conflict (a) do update set b = excluded.b
where excluded.b = 'b';
INSERT 0 0

insert into p values (2, 'a') on conflict (a) do update set b = excluded.b
where excluded.b = 'b';
INSERT 0 1

insert into p values (2, 'b') on conflict (a) do update set b = excluded.b
where excluded.b = 'b';
INSERT 0 1

insert into p values (2, 'c') on conflict (a) do update set b = excluded.b
where excluded.b = 'b';
INSERT 0 0

insert into p values (5, 'a') on conflict (a) do update set b = excluded.b
where excluded.b = 'b';
INSERT 0 1

insert into p values (5, 'b') on conflict (a) do update set b = excluded.b
where excluded.b = 'b';
INSERT 0 1

insert into p values (5, 'c') on conflict (a) do update set b = excluded.b
where excluded.b = 'b';
INSERT 0 0

insert into p values (5) on conflict (a) do update set b = excluded.b
where excluded.b = 'b';
INSERT 0 0

select tableoid::regclass, * from p;
 tableoid | a | b
----------+---+---
 p1       | 1 | b
 p2       | 2 | b
 p5       | 5 | b
(3 rows)

I have incorporated your patch in the main patch after updating the
comments a bit.  Also, now that 6666ee49f49 is in [1], the transition
table related tests I proposed yesterday pass nicely.  Instead of posting
as a separate patch, I have merged it with the main patch.  So now that
planner refactoring is unnecessary, attached is just one patch.

Thanks,
Amit

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=6666ee49f49