On Mon, Apr 12, 2021 at 4:54 AM Tom Lane <t...@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?
From 32d5c87f38d383501d10dbbda1f93ab8eb4d0ab6 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.munro@gmail.com> Date: Mon, 12 Apr 2021 09:53:54 +1200 Subject: [PATCH] Fix SSI bug in heap_update(). Commit 6f38d4dac38 failed to heed a warning about the stability of the value pointed to by "otid". The caller is allowed to pass in a pointer to newtup->t_self, which will be updated during the execution of the function. Instead, use the value we copy into oldtup.t_self at the top of the function. Back-patch to 13. Reported-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/2689164.1618160085%40sss.pgh.pa.us --- src/backend/access/heap/heapam.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 03d4abc938..548720021e 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -3900,7 +3900,8 @@ l2: * will include checking the relation level, there is no benefit to a * separate check for the new tuple. */ - CheckForSerializableConflictIn(relation, otid, BufferGetBlockNumber(buffer)); + CheckForSerializableConflictIn(relation, &oldtup.t_self, + BufferGetBlockNumber(buffer)); /* * At this point newbuf and buffer are both pinned and locked, and newbuf -- 2.30.2
