+# Concurrency error from GetTupleForTrigger +# Concurrency error from ExecLockRows
I think you don't need to mention above sentences in spec files. Apart from that, your patch looks good to me. I have marked it as Ready For Committer.
I too have tested this feature with isolation framework and this look good to me.
It looks to me that we are trying to fix only one issue here with concurrent updates. What happens if a non-partition key is first updated and then a second session updates the partition key?
For example, with your patches applied:
CREATE TABLE pa_target (key integer, val text)
PARTITION BY LIST (key);
CREATE TABLE part1 PARTITION OF pa_target FOR VALUES IN (1);
CREATE TABLE part2 PARTITION OF pa_target FOR VALUES IN (2);
INSERT INTO pa_target VALUES (1, 'initial1');
session1:
BEGIN;
UPDATE pa_target SET val = val || ' updated by update1' WHERE key = 1;
UPDATE 1
postgres=# SELECT * FROM pa_target ;
key | val
-----+-----------------------------
1 | initial1 updated by update1
(1 row)
session2:
UPDATE pa_target SET val = val || ' updated by update2', key = key + 1 WHERE key = 1
<blocks>
session1:
postgres=# COMMIT;
COMMIT
<session1 unblocks and completes its UPDATE>
postgres=# SELECT * FROM pa_target ;
key | val
-----+-----------------------------
2 | initial1 updated by update2
(1 row)
Ouch. The committed updates by session1 are overwritten by session2. This clearly violates the rules that rest of the system obeys and is not acceptable IMHO.
Clearly, ExecUpdate() while moving rows between partitions is missing out on re-constructing the to-be-updated tuple, based on the latest tuple in the update chain. Instead, it's simply deleting the latest tuple and inserting a new tuple in the new partition based on the old tuple. That's simply wrong.
I haven't really thought carefully to see if this should be a separate patch, but it warrants attention. We should at least think through all different concurrency aspects of partition key updates and think about a holistic solution, instead of fixing one problem at a time. This probably also shows that isolation tests for partition key updates are either missing (I haven't checked) or they need more work.
