On Thu, 4 Sep 2025 13:50:41 +0100 Dean Rasheed <[email protected]> wrote:
> On Thu, 28 Aug 2025 at 15:10, Yugo Nagata <[email protected]> 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 <[email protected]>
