On Thu, 9 Mar 2023 at 13:11, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> So I wrote a patch doing that, and added some isolation tests
> confirming that it fixes the reported issue.
>
> Unfortunately, I think this exposes a different issue -- if the target
> table has a BEFORE ROW trigger, and the target tuple is concurrently
> updated, the trigger code will lock the updated tuple, and the merge
> code won't see it as a concurrent update, and so it won't re-check the
> WHEN condition. That leads to some quite surprising results, as shown
> in the new isolation test cases.
>
I have been thinking about this some more, and I added a bunch of
additional isolation tests to test MERGE UPDATE/DELETE against
concurrent UPDATE/DELETE for normal tables, tables with BEFORE ROW
triggers, and partitioned tables where the MERGE does a
cross-partition update. Various of those tests currently fail, either
with the crash reported in this thread, or because they execute the
wrong merge action, or no action at all.
To fix this, it's necessary to pass the TM_Result and TM_FailureData
results from the trigger code's attempt to lock the old tuple, and
from the cross-partition-update code's attempt to delete the old
tuple, up to the merge code. This allows the merge code to handle
those cases in the same way as it does for non-partitioned tables
without triggers.
Overall, this leads to a few simplifications in nodeModifyTable.c:
1). ModifyTableContext->GetUpdateNewTuple() is now never called for
MERGE, so it and mergeGetUpdateNewTuple() can be deleted, and
ExecGetUpdateNewTuple() can be restored to how it was in v14.
2). ModifyTableContext->cpUpdateRetrySlot is no longer used by MERGE,
so it can also be deleted, and ExecCrossPartitionUpdate() can just
return retry_slot directly to ExecUpdateAct(), as it used to do.
3). ExecMergeMatched() becomes a bit simpler, since it no longer needs
special-case code for cross-partition updates.
Overall, this removes around 65 lines from nodeModifyTable.c, but more
importantly, it seems to fix the concurrent update issues.
Attached is an updated patch that I'm more happy with now, and a
slightly modified one for v15, keeping the trigger API
backwards-compatible for extensions.
Regards,
Dean