On Fri, 5 Sep 2025 08:47:19 +0100
Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On Thu, 4 Sept 2025 at 16:03, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> >
> > > 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.
>
> Good point.
>
> > > 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.
>
> OK, thanks. Pushed.
>
> In v15 and v16 the code was a little different, and it was actually
> much more obvious that it thought that table_tuple_lock() didn't
> update the tupleid passed to it, because it was explicitly updating it
> from tmfd->ctid.
>
> It was also slightly harder to trigger the error in v15 and v16, so I
> had to tweak the tests a little. In doing so, I realised that it's not
> actually necessary to have 3 sessions to reproduce this error -- it
> only requires 2 sessions, as long as the "other" session does 2
> updates. So I did it that way in all branches, which simplified them a
> bit.
Thank you for committing it, and for your additional work on improving
the test and the backpatch.
Regards,
Yugo Nagata
--
Yugo Nagata <nagata@sraoss.co.jp>
