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>
