Re: Inconsistent update in the MERGE command

On Thu, 4 Sep 2025 13:50:41 +0100
Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

> On Thu, 28 Aug 2025 at 15:10, Yugo Nagata <nagata@sraoss.co.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@sraoss.co.jp>