Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 2dc2e4e

Browse files
committed
Avoid improbable PANIC during heap_update, redux.
Commit 34f581c intended to ensure that RelationGetBufferForTuple would acquire a visibility-map page pin in case the otherBuffer's all-visible bit had become set since we last had lock on that page. But I missed a case: when we're extending the relation, VM concerns were dealt with only in the relatively-less-likely case that we fail to conditionally lock the otherBuffer. I think I'd believed that we couldn't need to worry about it if the conditional lock succeeds, which is true for the target buffer; but the otherBuffer was unlocked for awhile so its bit might be set anyway. So we need to do the GetVisibilityMapPins dance, and then also recheck the page's free space, in both cases. Per report from Jaime Casanova. Back-patch to v12 as the previous patch was (although there's still no evidence that the bug is reachable pre-v14). Discussion: https://postgr.es/m/E1lWLjP-00006Y-Ml@gemulon.postgresql.org
1 parent 0e497ea commit 2dc2e4e

File tree

1 file changed

+23
-18
lines changed
  • src/backend/access/heap

1 file changed

+23
-18
lines changed

src/backend/access/heap/hio.c

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -678,29 +678,34 @@ RelationGetBufferForTuple(Relation relation, Size len,
678678
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
679679
LockBuffer(otherBuffer, BUFFER_LOCK_EXCLUSIVE);
680680
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
681+
}
681682

682-
/*
683-
* Because the buffers were unlocked for a while, it's possible,
684-
* although unlikely, that an all-visible flag became set or that
685-
* somebody used up the available space in the new page. We can
686-
* use GetVisibilityMapPins to deal with the first case. In the
687-
* second case, just retry from start.
688-
*/
689-
GetVisibilityMapPins(relation, otherBuffer, buffer,
690-
otherBlock, targetBlock, vmbuffer_other,
691-
vmbuffer);
683+
/*
684+
* Because the buffers were unlocked for a while, it's possible,
685+
* although unlikely, that an all-visible flag became set or that
686+
* somebody used up the available space in the new page. We can use
687+
* GetVisibilityMapPins to deal with the first case. In the second
688+
* case, just retry from start.
689+
*/
690+
GetVisibilityMapPins(relation, otherBuffer, buffer,
691+
otherBlock, targetBlock, vmbuffer_other,
692+
vmbuffer);
692693

693-
if (len > PageGetHeapFreeSpace(page))
694-
{
695-
LockBuffer(otherBuffer, BUFFER_LOCK_UNLOCK);
696-
UnlockReleaseBuffer(buffer);
694+
/*
695+
* Note that we have to check the available space even if our
696+
* conditional lock succeeded, because GetVisibilityMapPins might've
697+
* transiently released lock on the target buffer to acquire a VM pin
698+
* for the otherBuffer.
699+
*/
700+
if (len > PageGetHeapFreeSpace(page))
701+
{
702+
LockBuffer(otherBuffer, BUFFER_LOCK_UNLOCK);
703+
UnlockReleaseBuffer(buffer);
697704

698-
goto loop;
699-
}
705+
goto loop;
700706
}
701707
}
702-
703-
if (len > PageGetHeapFreeSpace(page))
708+
else if (len > PageGetHeapFreeSpace(page))
704709
{
705710
/* We should not get here given the test at the top */
706711
elog(PANIC, "tuple is too big: size %zu", len);

0 commit comments

Comments
 (0)