Thread: Possible SSI bug in heap_update

While re-reading heap_update() in connection with that PANIC we're
chasing, my attention was drawn to this comment:

    /*
     * Note: beyond this point, use oldtup not otid to refer to old tuple.
     * otid may very well point at newtup->t_self, which we will overwrite
     * with the new tuple's location, so there's great risk of confusion if we
     * use otid anymore.
     */

This seemingly sage advice is being ignored in one place:

    CheckForSerializableConflictIn(relation, otid, BufferGetBlockNumber(buffer));

I wonder whether that's a mistake.  There'd be only a low probability
of our detecting it through testing, I fear.

            regards, tom lane

On Mon, Apr 12, 2021 at 4:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> While re-reading heap_update() in connection with that PANIC we're
> chasing, my attention was drawn to this comment:
>
>     /*
>      * Note: beyond this point, use oldtup not otid to refer to old tuple.
>      * otid may very well point at newtup->t_self, which we will overwrite
>      * with the new tuple's location, so there's great risk of confusion if we
>      * use otid anymore.
>      */
>
> This seemingly sage advice is being ignored in one place:
>
>         CheckForSerializableConflictIn(relation, otid, BufferGetBlockNumber(buffer));
>
> I wonder whether that's a mistake.  There'd be only a low probability
> of our detecting it through testing, I fear.

Yeah.  Patch attached.

I did a bit of printf debugging, and while it's common that otid ==
&newtup->t_self, neither our regression tests nor our isolation tests
reach a case where ItemPointerEquals(otid, &oldtup.t_self) is false at
the place where that check runs.  Obviously those tests don't exercise
all the branches and concurrency scenarios where we goto l2, so I'm
not at all sure about this, but hmm... at first glance, perhaps there
is no live bug here because the use of *otid comes before
RelationPutHeapTuple() which is where newtup->t_self is really
updated?

On Mon, Apr 12, 2021 at 10:36 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> Yeah.  Patch attached.

Pushed.