Re: Inconsistent update in the MERGE command

On Thu, 4 Sep 2025 13:50:41 +0100
Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:

> On Thu, 28 Aug 2025 at 15:10, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> >
> > Thank you for your suggestion and the test patch. The test looks good
> > to me, so I’ve attached an updated patch including it.
>
> Thanks, that mostly looks good to me.

Thank you for updating the patch.

> There's one other place in ExecMergeMatched() that's using
> TM_FailureData.ctid when it shouldn't, and that's this check:
>
> if (ItemPointerIndicatesMovedPartitions(&context->tmfd.ctid))
> ereport(ERROR,
> (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
> errmsg("tuple to be merged was already moved to
> another partition due to concurrent update")));

I overlooked this check.

> I've updated that to use tupleid in the attached v3 patch, and added a
> couple more isolation tests. In practice, however, I don't think that
> error can ever happen because this check follows table_tuple_lock()
> which has a similar test which will always error out first. I was
> tempted to simply remove this check from ExecMergeMatched(), but I
> think perhaps it's worth keeping just in case.

ItemPointerIndicatesMovedPartitions() is checked in heapam_tuple_lock(),
but other table access methods might not perform this check (though I'm
not sure if this is acceptable), so it may still make sense to keep it.

> Also, I thought that it's worth updating the comments for
> table_tuple_lock() and TM_FailureData to make it clearer that it
> updates its tid argument, and which fields of TM_FailureData can be
> relied upon in the different cases.

+1
This should help prevent misuse of the argument in the future.

Regards,
Yugo Nagata

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>