public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v6 04/17] hio: Don't pin the VM while holding buffer lock while extending
7+ messages / 4 participants
[nested] [flat]
* [PATCH v6 04/17] hio: Don't pin the VM while holding buffer lock while extending
@ 2023-03-29 01:17 Andres Freund <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Andres Freund @ 2023-03-29 01:17 UTC (permalink / raw)
Discussion: http://postgr.es/m/[email protected]
---
src/backend/access/heap/hio.c | 120 +++++++++++++++++++++++-----------
1 file changed, 81 insertions(+), 39 deletions(-)
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 7479212d4e0..8b3dfa0ccae 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -134,14 +134,17 @@ ReadBufferBI(Relation relation, BlockNumber targetBlock,
* buffer2 may be InvalidBuffer, if only one buffer is involved. buffer1
* must not be InvalidBuffer. If both buffers are specified, block1 must
* be less than block2.
+ *
+ * Returns whether buffer locks were temporarily released.
*/
-static void
+static bool
GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
BlockNumber block1, BlockNumber block2,
Buffer *vmbuffer1, Buffer *vmbuffer2)
{
bool need_to_pin_buffer1;
bool need_to_pin_buffer2;
+ bool released_locks = false;
Assert(BufferIsValid(buffer1));
Assert(buffer2 == InvalidBuffer || block1 <= block2);
@@ -155,9 +158,10 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
&& PageIsAllVisible(BufferGetPage(buffer2))
&& !visibilitymap_pin_ok(block2, *vmbuffer2);
if (!need_to_pin_buffer1 && !need_to_pin_buffer2)
- return;
+ break;
/* We must unlock both buffers before doing any I/O. */
+ released_locks = true;
LockBuffer(buffer1, BUFFER_LOCK_UNLOCK);
if (buffer2 != InvalidBuffer && buffer2 != buffer1)
LockBuffer(buffer2, BUFFER_LOCK_UNLOCK);
@@ -183,6 +187,8 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
|| (need_to_pin_buffer1 && need_to_pin_buffer2))
break;
}
+
+ return released_locks;
}
/*
@@ -345,6 +351,7 @@ RelationGetBufferForTuple(Relation relation, Size len,
BlockNumber targetBlock,
otherBlock;
bool needLock;
+ bool unlockedTargetBuffer;
len = MAXALIGN(len); /* be conservative */
@@ -630,6 +637,9 @@ loop:
if (needLock)
UnlockRelationForExtension(relation, ExclusiveLock);
+ unlockedTargetBuffer = false;
+ targetBlock = BufferGetBlockNumber(buffer);
+
/*
* We need to initialize the empty new page. Double-check that it really
* is empty (this should never happen, but if it does we don't want to
@@ -639,75 +649,107 @@ loop:
if (!PageIsNew(page))
elog(ERROR, "page %u of relation \"%s\" should be empty but is not",
- BufferGetBlockNumber(buffer),
+ targetBlock,
RelationGetRelationName(relation));
PageInit(page, BufferGetPageSize(buffer), 0);
MarkBufferDirty(buffer);
/*
- * The page is empty, pin vmbuffer to set all_frozen bit.
+ * The page is empty, pin vmbuffer to set all_frozen bit. We don't want to
+ * do IO while the buffer is locked, so we unlock the page first if IO is
+ * needed (necessitating checks below).
*/
if (options & HEAP_INSERT_FROZEN)
{
- Assert(PageGetMaxOffsetNumber(BufferGetPage(buffer)) == 0);
- visibilitymap_pin(relation, BufferGetBlockNumber(buffer), vmbuffer);
+ Assert(PageGetMaxOffsetNumber(page) == 0);
+
+ if (!visibilitymap_pin_ok(targetBlock, *vmbuffer))
+ {
+ unlockedTargetBuffer = true;
+ LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
+ visibilitymap_pin(relation, targetBlock, vmbuffer);
+ }
}
/*
- * Lock the other buffer. It's guaranteed to be of a lower page number
- * than the new page. To conform with the deadlock prevent rules, we ought
- * to lock otherBuffer first, but that would give other backends a chance
- * to put tuples on our page. To reduce the likelihood of that, attempt to
- * lock the other buffer conditionally, that's very likely to work.
- * Otherwise we need to lock buffers in the correct order, and retry if
- * the space has been used in the mean time.
+ * If we unlocked the target buffer above, it's unlikely, but possible,
+ * that another backend used space on this page.
+ *
+ * If we didn't, and otherBuffer is valid, we need to lock the other
+ * buffer. It's guaranteed to be of a lower page number than the new page.
+ * To conform with the deadlock prevent rules, we ought to lock
+ * otherBuffer first, but that would give other backends a chance to put
+ * tuples on our page. To reduce the likelihood of that, attempt to lock
+ * the other buffer conditionally, that's very likely to work. Otherwise
+ * we need to lock buffers in the correct order, and retry if the space
+ * has been used in the mean time.
*
* Alternatively, we could acquire the lock on otherBuffer before
* extending the relation, but that'd require holding the lock while
* performing IO, which seems worse than an unlikely retry.
*/
- if (otherBuffer != InvalidBuffer)
+ if (unlockedTargetBuffer)
+ {
+ if (otherBuffer != InvalidBuffer)
+ LockBuffer(otherBuffer, BUFFER_LOCK_EXCLUSIVE);
+ LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
+ }
+ else if (otherBuffer != InvalidBuffer)
{
Assert(otherBuffer != buffer);
- targetBlock = BufferGetBlockNumber(buffer);
Assert(targetBlock > otherBlock);
if (unlikely(!ConditionalLockBuffer(otherBuffer)))
{
+ unlockedTargetBuffer = true;
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
LockBuffer(otherBuffer, BUFFER_LOCK_EXCLUSIVE);
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
}
+ }
- /*
- * Because the buffers were unlocked for a while, it's possible,
- * although unlikely, that an all-visible flag became set or that
- * somebody used up the available space in the new page. We can use
- * GetVisibilityMapPins to deal with the first case. In the second
- * case, just retry from start.
- */
- GetVisibilityMapPins(relation, otherBuffer, buffer,
- otherBlock, targetBlock, vmbuffer_other,
- vmbuffer);
-
- /*
- * Note that we have to check the available space even if our
- * conditional lock succeeded, because GetVisibilityMapPins might've
- * transiently released lock on the target buffer to acquire a VM pin
- * for the otherBuffer.
- */
- if (len > PageGetHeapFreeSpace(page))
+ /*
+ * If one of the buffers was unlocked, it's possible, although unlikely,
+ * that an all-visible flag became set. We can use GetVisibilityMapPins
+ * to deal with that.
+ */
+ if (unlockedTargetBuffer || otherBuffer != InvalidBuffer)
+ {
+ if (otherBuffer != InvalidBuffer)
{
- LockBuffer(otherBuffer, BUFFER_LOCK_UNLOCK);
+ if (GetVisibilityMapPins(relation, otherBuffer, buffer,
+ otherBlock, targetBlock, vmbuffer_other,
+ vmbuffer))
+ unlockedTargetBuffer = true;
+ }
+ else
+ {
+ if (GetVisibilityMapPins(relation, buffer, InvalidBuffer,
+ targetBlock, InvalidBlockNumber,
+ vmbuffer, InvalidBuffer))
+ unlockedTargetBuffer = true;
+ }
+ }
+
+ /*
+ * If the target buffer was temporarily unlocked since the relation
+ * extension, it's possible, although unlikely, that all the space on the
+ * page was already used. If so, we just retry from the start. If we
+ * didn't unlock, something has gone wrong if there's not enough space -
+ * the test at the top should have prevented reaching this case.
+ */
+ pageFreeSpace = PageGetHeapFreeSpace(page);
+ if (len > pageFreeSpace)
+ {
+ if (unlockedTargetBuffer)
+ {
+ if (otherBuffer != InvalidBuffer)
+ LockBuffer(otherBuffer, BUFFER_LOCK_UNLOCK);
UnlockReleaseBuffer(buffer);
goto loop;
}
- }
- else if (len > PageGetHeapFreeSpace(page))
- {
- /* We should not get here given the test at the top */
elog(PANIC, "tuple is too big: size %zu", len);
}
@@ -720,7 +762,7 @@ loop:
* current backend to make more insertions or not, which is probably a
* good bet most of the time. So for now, don't add it to FSM yet.
*/
- RelationSetTargetBlock(relation, BufferGetBlockNumber(buffer));
+ RelationSetTargetBlock(relation, targetBlock);
return buffer;
}
--
2.38.0
--cfn72vqnlbycypta
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0005-bufmgr-Add-some-error-checking-around-pinning.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v3 2/2] hio: Don't pin the VM while holding buffer lock while extending
@ 2023-04-03 18:26 Andres Freund <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Andres Freund @ 2023-04-03 18:26 UTC (permalink / raw)
Discussion: http://postgr.es/m/[email protected]
---
src/backend/access/heap/hio.c | 126 +++++++++++++++++++++++-----------
1 file changed, 85 insertions(+), 41 deletions(-)
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index e8aea42f8a9..15912f6415f 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -134,14 +134,17 @@ ReadBufferBI(Relation relation, BlockNumber targetBlock,
* To avoid complexity in the callers, either buffer1 or buffer2 may be
* InvalidBuffer if only one buffer is involved. For the same reason, block2
* may be smaller than block1.
+ *
+ * Returns whether buffer locks were temporarily released.
*/
-static void
+static bool
GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
BlockNumber block1, BlockNumber block2,
Buffer *vmbuffer1, Buffer *vmbuffer2)
{
bool need_to_pin_buffer1;
bool need_to_pin_buffer2;
+ bool released_locks = false;
/*
* Swap buffers around to handle case of a single block/buffer, and to
@@ -175,9 +178,10 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
&& PageIsAllVisible(BufferGetPage(buffer2))
&& !visibilitymap_pin_ok(block2, *vmbuffer2);
if (!need_to_pin_buffer1 && !need_to_pin_buffer2)
- return;
+ break;
/* We must unlock both buffers before doing any I/O. */
+ released_locks = true;
LockBuffer(buffer1, BUFFER_LOCK_UNLOCK);
if (buffer2 != InvalidBuffer && buffer2 != buffer1)
LockBuffer(buffer2, BUFFER_LOCK_UNLOCK);
@@ -203,6 +207,8 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
|| (need_to_pin_buffer1 && need_to_pin_buffer2))
break;
}
+
+ return released_locks;
}
/*
@@ -365,6 +371,8 @@ RelationGetBufferForTuple(Relation relation, Size len,
BlockNumber targetBlock,
otherBlock;
bool needLock;
+ bool unlockedTargetBuffer;
+ bool recheckVmPins;
len = MAXALIGN(len); /* be conservative */
@@ -645,6 +653,9 @@ loop:
if (needLock)
UnlockRelationForExtension(relation, ExclusiveLock);
+ unlockedTargetBuffer = false;
+ targetBlock = BufferGetBlockNumber(buffer);
+
/*
* We need to initialize the empty new page. Double-check that it really
* is empty (this should never happen, but if it does we don't want to
@@ -654,75 +665,108 @@ loop:
if (!PageIsNew(page))
elog(ERROR, "page %u of relation \"%s\" should be empty but is not",
- BufferGetBlockNumber(buffer),
+ targetBlock,
RelationGetRelationName(relation));
PageInit(page, BufferGetPageSize(buffer), 0);
MarkBufferDirty(buffer);
/*
- * The page is empty, pin vmbuffer to set all_frozen bit.
+ * The page is empty, pin vmbuffer to set all_frozen bit. We don't want to
+ * do IO while the buffer is locked, so we unlock the page first if IO is
+ * needed (necessitating checks below).
*/
if (options & HEAP_INSERT_FROZEN)
{
- Assert(PageGetMaxOffsetNumber(BufferGetPage(buffer)) == 0);
- visibilitymap_pin(relation, BufferGetBlockNumber(buffer), vmbuffer);
+ Assert(PageGetMaxOffsetNumber(page) == 0);
+
+ if (!visibilitymap_pin_ok(targetBlock, *vmbuffer))
+ {
+ unlockedTargetBuffer = true;
+ LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
+ visibilitymap_pin(relation, targetBlock, vmbuffer);
+ }
}
/*
- * Lock the other buffer. It's guaranteed to be of a lower page number
- * than the new page. To conform with the deadlock prevent rules, we ought
- * to lock otherBuffer first, but that would give other backends a chance
- * to put tuples on our page. To reduce the likelihood of that, attempt to
- * lock the other buffer conditionally, that's very likely to work.
- * Otherwise we need to lock buffers in the correct order, and retry if
- * the space has been used in the mean time.
+ * Reacquire locks if necessary.
*
- * Alternatively, we could acquire the lock on otherBuffer before
- * extending the relation, but that'd require holding the lock while
- * performing IO, which seems worse than an unlikely retry.
+ * If the target buffer was unlocked above, or is unlocked while
+ * reacquiring the lock on otherBuffer below, it's unlikely, but possible,
+ * that another backend used space on this page. We check for that below,
+ * and retry if necessary.
*/
- if (otherBuffer != InvalidBuffer)
+ recheckVmPins = false;
+ if (unlockedTargetBuffer)
{
+ /* released lock on target buffer above */
+ if (otherBuffer != InvalidBuffer)
+ LockBuffer(otherBuffer, BUFFER_LOCK_EXCLUSIVE);
+ LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
+ recheckVmPins = true;
+ }
+ else if (otherBuffer != InvalidBuffer)
+ {
+ /*
+ * We did not release the target buffer, and otherBuffer is valid,
+ * need to lock the other buffer. It's guaranteed to be of a lower
+ * page number than the new page. To conform with the deadlock
+ * prevent rules, we ought to lock otherBuffer first, but that would
+ * give other backends a chance to put tuples on our page. To reduce
+ * the likelihood of that, attempt to lock the other buffer
+ * conditionally, that's very likely to work.
+ *
+ * Alternatively, we could acquire the lock on otherBuffer before
+ * extending the relation, but that'd require holding the lock while
+ * performing IO, which seems worse than an unlikely retry.
+ */
Assert(otherBuffer != buffer);
- targetBlock = BufferGetBlockNumber(buffer);
Assert(targetBlock > otherBlock);
if (unlikely(!ConditionalLockBuffer(otherBuffer)))
{
+ unlockedTargetBuffer = true;
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
LockBuffer(otherBuffer, BUFFER_LOCK_EXCLUSIVE);
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
}
+ recheckVmPins = true;
+ }
- /*
- * Because the buffers were unlocked for a while, it's possible,
- * although unlikely, that an all-visible flag became set or that
- * somebody used up the available space in the new page. We can use
- * GetVisibilityMapPins to deal with the first case. In the second
- * case, just retry from start.
- */
- GetVisibilityMapPins(relation, otherBuffer, buffer,
- otherBlock, targetBlock, vmbuffer_other,
- vmbuffer);
+ /*
+ * If one of the buffers was unlocked (always the case if otherBuffer is
+ * valid), it's possible, although unlikely, that an all-visible flag
+ * became set. We can use GetVisibilityMapPins to deal with that. It's
+ * possible that GetVisibilityMapPins() might need to temporarily release
+ * buffer locks, in which case we'll need to check if there's still enough
+ * space on the page below.
+ */
+ if (recheckVmPins)
+ {
+ if (GetVisibilityMapPins(relation, otherBuffer, buffer,
+ otherBlock, targetBlock, vmbuffer_other,
+ vmbuffer))
+ unlockedTargetBuffer = true;
+ }
- /*
- * Note that we have to check the available space even if our
- * conditional lock succeeded, because GetVisibilityMapPins might've
- * transiently released lock on the target buffer to acquire a VM pin
- * for the otherBuffer.
- */
- if (len > PageGetHeapFreeSpace(page))
+ /*
+ * If the target buffer was temporarily unlocked since the relation
+ * extension, it's possible, although unlikely, that all the space on the
+ * page was already used. If so, we just retry from the start. If we
+ * didn't unlock, something has gone wrong if there's not enough space -
+ * the test at the top should have prevented reaching this case.
+ */
+ pageFreeSpace = PageGetHeapFreeSpace(page);
+ if (len > pageFreeSpace)
+ {
+ if (unlockedTargetBuffer)
{
- LockBuffer(otherBuffer, BUFFER_LOCK_UNLOCK);
+ if (otherBuffer != InvalidBuffer)
+ LockBuffer(otherBuffer, BUFFER_LOCK_UNLOCK);
UnlockReleaseBuffer(buffer);
goto loop;
}
- }
- else if (len > PageGetHeapFreeSpace(page))
- {
- /* We should not get here given the test at the top */
elog(PANIC, "tuple is too big: size %zu", len);
}
@@ -735,7 +779,7 @@ loop:
* current backend to make more insertions or not, which is probably a
* good bet most of the time. So for now, don't add it to FSM yet.
*/
- RelationSetTargetBlock(relation, BufferGetBlockNumber(buffer));
+ RelationSetTargetBlock(relation, targetBlock);
return buffer;
}
--
2.38.0
--rlua5hhgdcrdaj2n--
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v7 11/14] hio: Don't pin the VM while holding buffer lock while extending
@ 2023-04-03 18:26 Andres Freund <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Andres Freund @ 2023-04-03 18:26 UTC (permalink / raw)
Discussion: http://postgr.es/m/[email protected]
---
src/backend/access/heap/hio.c | 126 +++++++++++++++++++++++-----------
1 file changed, 85 insertions(+), 41 deletions(-)
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index e77437fcac1..561d7329058 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -134,14 +134,17 @@ ReadBufferBI(Relation relation, BlockNumber targetBlock,
* To avoid complexity in the callers, either buffer1 or buffer2 may be
* InvalidBuffer if only one buffer is involved. For the same reason, block2
* may be smaller than block1.
+ *
+ * Returns whether buffer locks were temporarily released.
*/
-static void
+static bool
GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
BlockNumber block1, BlockNumber block2,
Buffer *vmbuffer1, Buffer *vmbuffer2)
{
bool need_to_pin_buffer1;
bool need_to_pin_buffer2;
+ bool released_locks = false;
/*
* Swap buffers around to handle case of a single block/buffer, and to
@@ -175,9 +178,10 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
&& PageIsAllVisible(BufferGetPage(buffer2))
&& !visibilitymap_pin_ok(block2, *vmbuffer2);
if (!need_to_pin_buffer1 && !need_to_pin_buffer2)
- return;
+ break;
/* We must unlock both buffers before doing any I/O. */
+ released_locks = true;
LockBuffer(buffer1, BUFFER_LOCK_UNLOCK);
if (buffer2 != InvalidBuffer && buffer2 != buffer1)
LockBuffer(buffer2, BUFFER_LOCK_UNLOCK);
@@ -203,6 +207,8 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
|| (need_to_pin_buffer1 && need_to_pin_buffer2))
break;
}
+
+ return released_locks;
}
/*
@@ -371,6 +377,8 @@ RelationGetBufferForTuple(Relation relation, Size len,
BlockNumber targetBlock,
otherBlock;
bool needLock;
+ bool unlockedTargetBuffer;
+ bool recheckVmPins;
len = MAXALIGN(len); /* be conservative */
@@ -651,6 +659,9 @@ loop:
if (needLock)
UnlockRelationForExtension(relation, ExclusiveLock);
+ unlockedTargetBuffer = false;
+ targetBlock = BufferGetBlockNumber(buffer);
+
/*
* We need to initialize the empty new page. Double-check that it really
* is empty (this should never happen, but if it does we don't want to
@@ -660,75 +671,108 @@ loop:
if (!PageIsNew(page))
elog(ERROR, "page %u of relation \"%s\" should be empty but is not",
- BufferGetBlockNumber(buffer),
+ targetBlock,
RelationGetRelationName(relation));
PageInit(page, BufferGetPageSize(buffer), 0);
MarkBufferDirty(buffer);
/*
- * The page is empty, pin vmbuffer to set all_frozen bit.
+ * The page is empty, pin vmbuffer to set all_frozen bit. We don't want to
+ * do IO while the buffer is locked, so we unlock the page first if IO is
+ * needed (necessitating checks below).
*/
if (options & HEAP_INSERT_FROZEN)
{
- Assert(PageGetMaxOffsetNumber(BufferGetPage(buffer)) == 0);
- visibilitymap_pin(relation, BufferGetBlockNumber(buffer), vmbuffer);
+ Assert(PageGetMaxOffsetNumber(page) == 0);
+
+ if (!visibilitymap_pin_ok(targetBlock, *vmbuffer))
+ {
+ unlockedTargetBuffer = true;
+ LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
+ visibilitymap_pin(relation, targetBlock, vmbuffer);
+ }
}
/*
- * Lock the other buffer. It's guaranteed to be of a lower page number
- * than the new page. To conform with the deadlock prevent rules, we ought
- * to lock otherBuffer first, but that would give other backends a chance
- * to put tuples on our page. To reduce the likelihood of that, attempt to
- * lock the other buffer conditionally, that's very likely to work.
- * Otherwise we need to lock buffers in the correct order, and retry if
- * the space has been used in the mean time.
+ * Reacquire locks if necessary.
*
- * Alternatively, we could acquire the lock on otherBuffer before
- * extending the relation, but that'd require holding the lock while
- * performing IO, which seems worse than an unlikely retry.
+ * If the target buffer was unlocked above, or is unlocked while
+ * reacquiring the lock on otherBuffer below, it's unlikely, but possible,
+ * that another backend used space on this page. We check for that below,
+ * and retry if necessary.
*/
- if (otherBuffer != InvalidBuffer)
+ recheckVmPins = false;
+ if (unlockedTargetBuffer)
{
+ /* released lock on target buffer above */
+ if (otherBuffer != InvalidBuffer)
+ LockBuffer(otherBuffer, BUFFER_LOCK_EXCLUSIVE);
+ LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
+ recheckVmPins = true;
+ }
+ else if (otherBuffer != InvalidBuffer)
+ {
+ /*
+ * We did not release the target buffer, and otherBuffer is valid,
+ * need to lock the other buffer. It's guaranteed to be of a lower
+ * page number than the new page. To conform with the deadlock
+ * prevent rules, we ought to lock otherBuffer first, but that would
+ * give other backends a chance to put tuples on our page. To reduce
+ * the likelihood of that, attempt to lock the other buffer
+ * conditionally, that's very likely to work.
+ *
+ * Alternatively, we could acquire the lock on otherBuffer before
+ * extending the relation, but that'd require holding the lock while
+ * performing IO, which seems worse than an unlikely retry.
+ */
Assert(otherBuffer != buffer);
- targetBlock = BufferGetBlockNumber(buffer);
Assert(targetBlock > otherBlock);
if (unlikely(!ConditionalLockBuffer(otherBuffer)))
{
+ unlockedTargetBuffer = true;
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
LockBuffer(otherBuffer, BUFFER_LOCK_EXCLUSIVE);
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
}
+ recheckVmPins = true;
+ }
- /*
- * Because the buffers were unlocked for a while, it's possible,
- * although unlikely, that an all-visible flag became set or that
- * somebody used up the available space in the new page. We can use
- * GetVisibilityMapPins to deal with the first case. In the second
- * case, just retry from start.
- */
- GetVisibilityMapPins(relation, otherBuffer, buffer,
- otherBlock, targetBlock, vmbuffer_other,
- vmbuffer);
+ /*
+ * If one of the buffers was unlocked (always the case if otherBuffer is
+ * valid), it's possible, although unlikely, that an all-visible flag
+ * became set. We can use GetVisibilityMapPins to deal with that. It's
+ * possible that GetVisibilityMapPins() might need to temporarily release
+ * buffer locks, in which case we'll need to check if there's still enough
+ * space on the page below.
+ */
+ if (recheckVmPins)
+ {
+ if (GetVisibilityMapPins(relation, otherBuffer, buffer,
+ otherBlock, targetBlock, vmbuffer_other,
+ vmbuffer))
+ unlockedTargetBuffer = true;
+ }
- /*
- * Note that we have to check the available space even if our
- * conditional lock succeeded, because GetVisibilityMapPins might've
- * transiently released lock on the target buffer to acquire a VM pin
- * for the otherBuffer.
- */
- if (len > PageGetHeapFreeSpace(page))
+ /*
+ * If the target buffer was temporarily unlocked since the relation
+ * extension, it's possible, although unlikely, that all the space on the
+ * page was already used. If so, we just retry from the start. If we
+ * didn't unlock, something has gone wrong if there's not enough space -
+ * the test at the top should have prevented reaching this case.
+ */
+ pageFreeSpace = PageGetHeapFreeSpace(page);
+ if (len > pageFreeSpace)
+ {
+ if (unlockedTargetBuffer)
{
- LockBuffer(otherBuffer, BUFFER_LOCK_UNLOCK);
+ if (otherBuffer != InvalidBuffer)
+ LockBuffer(otherBuffer, BUFFER_LOCK_UNLOCK);
UnlockReleaseBuffer(buffer);
goto loop;
}
- }
- else if (len > PageGetHeapFreeSpace(page))
- {
- /* We should not get here given the test at the top */
elog(PANIC, "tuple is too big: size %zu", len);
}
@@ -741,7 +785,7 @@ loop:
* current backend to make more insertions or not, which is probably a
* good bet most of the time. So for now, don't add it to FSM yet.
*/
- RelationSetTargetBlock(relation, BufferGetBlockNumber(buffer));
+ RelationSetTargetBlock(relation, targetBlock);
return buffer;
}
--
2.38.0
--ju7ntqqtbf66a3ug
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0012-hio-Use-ExtendBufferedRelBy.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Using defines for protocol characters
@ 2023-08-15 15:58 Nathan Bossart <[email protected]>
2023-08-15 21:25 ` Re: Using defines for protocol characters Tatsuo Ishii <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Nathan Bossart @ 2023-08-15 15:58 UTC (permalink / raw)
To: Tatsuo Ishii <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
On Tue, Aug 15, 2023 at 02:46:24PM +0900, Tatsuo Ishii wrote:
> Is it possible to put the new define staff into protocol.h then let
> pqcomm.h include protocol.h? This makes Pgpool-II and other middle
> ware/drivers written in C easier to use the defines so that they only
> include protocol.h to use the defines.
It is possible, of course, but are there any reasons such programs couldn't
include pqcomm.h? It looks relatively inexpensive to me. That being said,
I'm fine with the approach you describe if the folks in this thread agree.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Using defines for protocol characters
2023-08-15 15:58 Re: Using defines for protocol characters Nathan Bossart <[email protected]>
@ 2023-08-15 21:25 ` Tatsuo Ishii <[email protected]>
2023-08-15 21:37 ` Re: Using defines for protocol characters Michael Paquier <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Tatsuo Ishii @ 2023-08-15 21:25 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
> On Tue, Aug 15, 2023 at 02:46:24PM +0900, Tatsuo Ishii wrote:
>> Is it possible to put the new define staff into protocol.h then let
>> pqcomm.h include protocol.h? This makes Pgpool-II and other middle
>> ware/drivers written in C easier to use the defines so that they only
>> include protocol.h to use the defines.
>
> It is possible, of course, but are there any reasons such programs couldn't
> include pqcomm.h? It looks relatively inexpensive to me. That being said,
> I'm fine with the approach you describe if the folks in this thread agree.
Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But
what about other middleware?
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Using defines for protocol characters
2023-08-15 15:58 Re: Using defines for protocol characters Nathan Bossart <[email protected]>
2023-08-15 21:25 ` Re: Using defines for protocol characters Tatsuo Ishii <[email protected]>
@ 2023-08-15 21:37 ` Michael Paquier <[email protected]>
2023-08-15 21:44 ` Re: Using defines for protocol characters Tatsuo Ishii <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Michael Paquier @ 2023-08-15 21:37 UTC (permalink / raw)
To: Tatsuo Ishii <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
On Wed, Aug 16, 2023 at 06:25:09AM +0900, Tatsuo Ishii wrote:
> Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But
> what about other middleware?
Why do you need to include directly c.h? There are definitions in
there that are not intended to be exposed.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Using defines for protocol characters
2023-08-15 15:58 Re: Using defines for protocol characters Nathan Bossart <[email protected]>
2023-08-15 21:25 ` Re: Using defines for protocol characters Tatsuo Ishii <[email protected]>
2023-08-15 21:37 ` Re: Using defines for protocol characters Michael Paquier <[email protected]>
@ 2023-08-15 21:44 ` Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Tatsuo Ishii @ 2023-08-15 21:44 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
> On Wed, Aug 16, 2023 at 06:25:09AM +0900, Tatsuo Ishii wrote:
>> Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But
>> what about other middleware?
>
> Why do you need to include directly c.h? There are definitions in
> there that are not intended to be exposed.
pqcomm.h has this:
#define UNIXSOCK_PATH(path, port, sockdir) \
(AssertMacro(sockdir), \
AssertMacro(*(sockdir) != '\0'), \
snprintf(path, sizeof(path), "%s/.s.PGSQL.%d", \
(sockdir), (port)))
AssertMacro is defined in c.h.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2023-08-15 21:44 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-03-29 01:17 [PATCH v6 04/17] hio: Don't pin the VM while holding buffer lock while extending Andres Freund <[email protected]>
2023-04-03 18:26 [PATCH v3 2/2] hio: Don't pin the VM while holding buffer lock while extending Andres Freund <[email protected]>
2023-04-03 18:26 [PATCH v7 11/14] hio: Don't pin the VM while holding buffer lock while extending Andres Freund <[email protected]>
2023-08-15 15:58 Re: Using defines for protocol characters Nathan Bossart <[email protected]>
2023-08-15 21:25 ` Re: Using defines for protocol characters Tatsuo Ishii <[email protected]>
2023-08-15 21:37 ` Re: Using defines for protocol characters Michael Paquier <[email protected]>
2023-08-15 21:44 ` Re: Using defines for protocol characters Tatsuo Ishii <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox