From: Andres Freund <andres@anarazel.de>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: vignesh C <vignesh21@gmail.com>
Cc: pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>
Cc: Melanie Plageman <melanieplageman@gmail.com>
Cc: Yura Sokolov <y.sokolov@postgrespro.ru>
Cc: Robert Haas <robertmhaas@gmail.com>
Subject: Re: refactoring relation extension and BufferAlloc(), faster COPY
Date: Sun, 26 Mar 2023 12:26:59 -0700
Message-ID: <20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de> (raw)
In-Reply-To: <20230301223515.pucbj7nb54n4i4nv@awork3.anarazel.de>
References: <3d8b0834-c8eb-88cd-deac-5321870652b0@iki.fi>
<20230221163331.7wxvu66kqzktbteb@alvherre.pgsql>
<20230301073318.56klnz6t6ynlw2cm@awork3.anarazel.de>
<1e2df302-d0c4-8baa-fbd6-50a810b4e1ad@iki.fi>
<20230301223515.pucbj7nb54n4i4nv@awork3.anarazel.de>
Hi,
Attached is v5. Lots of comment polishing, a bit of renaming. I extracted the
relation extension related code in hio.c back into its own function.
While reviewing the hio.c code, I did realize that too much stuff is done
while holding the buffer lock. See also the pre-existing issue
https://postgr.es/m/20230325025740.wzvchp2kromw4zqz%40awork3.anarazel.de
Greetings,
Andres Freund
Attachments:
[text/x-diff] v5-0001-Add-some-error-checking-around-pinning.patch (3.1K, ../20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de/2-v5-0001-Add-some-error-checking-around-pinning.patch)
download | inline diff:
From d0e47591a10e5bb2745a366fce2a51d9964f28cb Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 1 Jul 2020 19:06:45 -0700
Subject: [PATCH v5 01/14] Add some error checking around pinning
---
src/include/storage/bufmgr.h | 1 +
src/backend/storage/buffer/bufmgr.c | 42 ++++++++++++++++++++---------
2 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.hindex b8a18b8081f..973547a8baf 100644--- a/src/include/storage/bufmgr.h+++ b/src/include/storage/bufmgr.h@@ -134,6 +134,7 @@ extern void ReleaseBuffer(Buffer buffer);
extern void UnlockReleaseBuffer(Buffer buffer);
extern void MarkBufferDirty(Buffer buffer);
extern void IncrBufferRefCount(Buffer buffer);
+extern void BufferCheckOneLocalPin(Buffer buffer);
extern Buffer ReleaseAndReadBuffer(Buffer buffer, Relation relation,
BlockNumber blockNum);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.cindex 95212a39416..fa20fab5a29 100644--- a/src/backend/storage/buffer/bufmgr.c+++ b/src/backend/storage/buffer/bufmgr.c@@ -1755,6 +1755,8 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy)
bool result;
PrivateRefCountEntry *ref;
+ Assert(!BufferIsLocal(b));+
ref = GetPrivateRefCountEntry(b, true);
if (ref == NULL)
@@ -1900,6 +1902,8 @@ UnpinBuffer(BufferDesc *buf)
PrivateRefCountEntry *ref;
Buffer b = BufferDescriptorGetBuffer(buf);
+ Assert(!BufferIsLocal(b));+
/* not moving as we're likely deleting it soon anyway */
ref = GetPrivateRefCountEntry(b, false);
Assert(ref != NULL);
@@ -4283,6 +4287,25 @@ ConditionalLockBuffer(Buffer buffer)
LW_EXCLUSIVE);
}
+void+BufferCheckOneLocalPin(Buffer buffer)+{+ if (BufferIsLocal(buffer))+ {+ /* There should be exactly one pin */+ if (LocalRefCount[-buffer - 1] != 1)+ elog(ERROR, "incorrect local pin count: %d",+ LocalRefCount[-buffer - 1]);+ }+ else+ {+ /* There should be exactly one local pin */+ if (GetPrivateRefCount(buffer) != 1)+ elog(ERROR, "incorrect local pin count: %d",+ GetPrivateRefCount(buffer));+ }+}+
/*
* LockBufferForCleanup - lock a buffer in preparation for deleting items
*
@@ -4310,20 +4333,11 @@ LockBufferForCleanup(Buffer buffer)
Assert(BufferIsPinned(buffer));
Assert(PinCountWaitBuf == NULL);
- if (BufferIsLocal(buffer))- {- /* There should be exactly one pin */- if (LocalRefCount[-buffer - 1] != 1)- elog(ERROR, "incorrect local pin count: %d",- LocalRefCount[-buffer - 1]);- /* Nobody else to wait for */- return;- }+ BufferCheckOneLocalPin(buffer);- /* There should be exactly one local pin */- if (GetPrivateRefCount(buffer) != 1)- elog(ERROR, "incorrect local pin count: %d",- GetPrivateRefCount(buffer));+ /* Nobody else to wait for */+ if (BufferIsLocal(buffer))+ return;
bufHdr = GetBufferDescriptor(buffer - 1);
@@ -4824,6 +4838,8 @@ LockBufHdr(BufferDesc *desc)
SpinDelayStatus delayStatus;
uint32 old_buf_state;
+ Assert(!BufferIsLocal(BufferDescriptorGetBuffer(desc)));+
init_local_spin_delay(&delayStatus);
while (true)
--
2.38.0
[text/x-diff] v5-0002-hio-Release-extension-lock-before-initializing-pa.patch (2.0K, ../20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de/3-v5-0002-hio-Release-extension-lock-before-initializing-pa.patch)
download | inline diff:
From fcd9364e3bbd93410d748bd4a308a40b93cc6451 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 24 Oct 2022 12:28:06 -0700
Subject: [PATCH v5 02/14] hio: Release extension lock before initializing page
/ pinning VM
PageInit() while holding the extension lock is unnecessary after 0d1fe9f74e3
started to use RBM_ZERO_AND_LOCK - nobody can look at the new page before we
release the page lock. PageInit() zeroes the page, which isn't that cheap, so
deferring it until after the extension lock is released seems like a good idea.
Doing visibilitymap_pin() while holding the extension lock, introduced in
7db0cd2145f2, looks like an accident. Due to the restrictions on
HEAP_INSERT_FROZEN it's unlikely to be a performance issue, but it still seems
better to move it out.
---
src/backend/access/heap/hio.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.cindex e152807d2dc..7479212d4e0 100644--- a/src/backend/access/heap/hio.c+++ b/src/backend/access/heap/hio.c@@ -623,6 +623,13 @@ loop:
*/
buffer = ReadBufferBI(relation, P_NEW, RBM_ZERO_AND_LOCK, bistate);
+ /*+ * Release the file-extension lock; it's now OK for someone else to extend+ * the relation some more.+ */+ if (needLock)+ UnlockRelationForExtension(relation, ExclusiveLock);+
/*
* 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
@@ -647,13 +654,6 @@ loop:
visibilitymap_pin(relation, BufferGetBlockNumber(buffer), vmbuffer);
}
- /*- * Release the file-extension lock; it's now OK for someone else to extend- * the relation some more.- */- if (needLock)- UnlockRelationForExtension(relation, ExclusiveLock);-
/*
* 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
--
2.38.0
[text/x-diff] v5-0003-Add-smgrzeroextend-FileZero-FileFallocate.patch (12.1K, ../20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de/4-v5-0003-Add-smgrzeroextend-FileZero-FileFallocate.patch)
download | inline diff:
From 5aa4e216299176ce0f7e0e54e7d97269b418c8d0 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 27 Feb 2023 17:36:37 -0800
Subject: [PATCH v5 03/14] Add smgrzeroextend(), FileZero(), FileFallocate()
smgrzeroextend() uses FileFallocate() to efficiently extend files by multiple
blocks. When extending by a small number of blocks, use FileZero() instead, as
using posix_fallocate() for small numbers of blocks is inefficient for some
file systems / operating systems. FileZero() is also used as the fallback for
FileFallocate() on platforms / filesystems that don't support fallocate.
Author:
Reviewed-by:
Discussion: https://postgr.es/m/20221029025420.eplyow6k7tgu6he3@awork3.anarazel.de
Backpatch:
---
src/include/storage/fd.h | 3 +
src/include/storage/md.h | 2 +
src/include/storage/smgr.h | 2 +
src/backend/storage/file/fd.c | 90 ++++++++++++++++++++++++++
src/backend/storage/smgr/md.c | 111 ++++++++++++++++++++++++++++++++
src/backend/storage/smgr/smgr.c | 28 ++++++++
6 files changed, 236 insertions(+)
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.hindex f85de97d083..daceafd4732 100644--- a/src/include/storage/fd.h+++ b/src/include/storage/fd.h@@ -106,6 +106,9 @@ extern int FilePrefetch(File file, off_t offset, off_t amount, uint32 wait_event
extern int FileRead(File file, void *buffer, size_t amount, off_t offset, uint32 wait_event_info);
extern int FileWrite(File file, const void *buffer, size_t amount, off_t offset, uint32 wait_event_info);
extern int FileSync(File file, uint32 wait_event_info);
+extern int FileZero(File file, off_t offset, off_t amount, uint32 wait_event_info);+extern int FileFallocate(File file, off_t offset, off_t amount, uint32 wait_event_info);+
extern off_t FileSize(File file);
extern int FileTruncate(File file, off_t offset, uint32 wait_event_info);
extern void FileWriteback(File file, off_t offset, off_t nbytes, uint32 wait_event_info);
diff --git a/src/include/storage/md.h b/src/include/storage/md.hindex 8f32af9ef3d..941879ee6a8 100644--- a/src/include/storage/md.h+++ b/src/include/storage/md.h@@ -28,6 +28,8 @@ extern bool mdexists(SMgrRelation reln, ForkNumber forknum);
extern void mdunlink(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo);
extern void mdextend(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, const void *buffer, bool skipFsync);
+extern void mdzeroextend(SMgrRelation reln, ForkNumber forknum,+ BlockNumber blocknum, int nblocks, bool skipFsync);
extern bool mdprefetch(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum);
extern void mdread(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.hindex 0935144f425..a9a179aabac 100644--- a/src/include/storage/smgr.h+++ b/src/include/storage/smgr.h@@ -92,6 +92,8 @@ extern void smgrdosyncall(SMgrRelation *rels, int nrels);
extern void smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo);
extern void smgrextend(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, const void *buffer, bool skipFsync);
+extern void smgrzeroextend(SMgrRelation reln, ForkNumber forknum,+ BlockNumber blocknum, int nblocks, bool skipFsync);
extern bool smgrprefetch(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum);
extern void smgrread(SMgrRelation reln, ForkNumber forknum,
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.cindex 9fd8444ed4d..c34ed41d52e 100644--- a/src/backend/storage/file/fd.c+++ b/src/backend/storage/file/fd.c@@ -2206,6 +2206,92 @@ FileSync(File file, uint32 wait_event_info)
return returnCode;
}
+/*+ * Zero a region of the file.+ *+ * Returns 0 on success, -1 otherwise. In the latter case errno is set to the+ * appropriate error.+ */+int+FileZero(File file, off_t offset, off_t amount, uint32 wait_event_info)+{+ int returnCode;+ ssize_t written;++ Assert(FileIsValid(file));+ returnCode = FileAccess(file);+ if (returnCode < 0)+ return returnCode;++ pgstat_report_wait_start(wait_event_info);+ written = pg_pwrite_zeros(VfdCache[file].fd, amount, offset);+ pgstat_report_wait_end();++ if (written < 0)+ return -1;+ else if (written != amount)+ {+ /* if errno is unset, assume problem is no disk space */+ if (errno == 0)+ errno = ENOSPC;+ return -1;+ }++ return 0;+}++/*+ * Try to reserve file space with posix_fallocate(). If posix_fallocate() is+ * not implemented on the operating system or fails with EINVAL / EOPNOTSUPP,+ * use FileZero() instead.+ *+ * Note that at least glibc() implements posix_fallocate() in userspace if not+ * implemented by the filesystem. That's not the case for all environments+ * though.+ *+ * Returns 0 on success, -1 otherwise. In the latter case errno is set to the+ * appropriate error.+ *+ * Even though posix_fallocate has the offset before the length argument, it+ * seems better to keep the argument order consistent with most of the other+ * functions in this file.+ */+int+FileFallocate(File file, off_t offset, off_t amount, uint32 wait_event_info)+{+#ifdef HAVE_POSIX_FALLOCATE+ int returnCode;++ Assert(FileIsValid(file));+ returnCode = FileAccess(file);+ if (returnCode < 0)+ return returnCode;++ pgstat_report_wait_start(wait_event_info);+ returnCode = posix_fallocate(VfdCache[file].fd, offset, amount);+ pgstat_report_wait_end();++ if (returnCode == 0)+ return 0;++ /* for compatibility with %m printing etc */+ errno = returnCode;++ /*+ * Return in cases of a "real" failure, if fallocate is not supported,+ * fall through to the FileZero() backed implementation.+ */+ if (returnCode != EINVAL && returnCode != EOPNOTSUPP)+ return returnCode;++ if (returnCode == 0 ||+ (returnCode != EINVAL && returnCode != EINVAL))+ return returnCode;+#endif++ return FileZero(file, offset, amount, wait_event_info);+}+
off_t
FileSize(File file)
{
@@ -2278,6 +2364,10 @@ int
FileGetRawDesc(File file)
{
Assert(FileIsValid(file));
++ if (FileAccess(file) < 0)+ return -1;+
return VfdCache[file].fd;
}
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.cindex 352958e1feb..59a65a8305c 100644--- a/src/backend/storage/smgr/md.c+++ b/src/backend/storage/smgr/md.c@@ -28,6 +28,7 @@
#include "access/xlog.h"
#include "access/xlogutils.h"
#include "commands/tablespace.h"
+#include "common/file_utils.h"
#include "miscadmin.h"
#include "pg_trace.h"
#include "pgstat.h"
@@ -500,6 +501,116 @@ mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
Assert(_mdnblocks(reln, forknum, v) <= ((BlockNumber) RELSEG_SIZE));
}
+/*+ * mdzeroextend() -- Add ew zeroed out blocks to the specified relation.+ *+ * Similar to mdrextend(), except the relation can be extended by+ * multiple blocks at once, and that the added blocks will be filled with+ * zeroes.+ */+void+mdzeroextend(SMgrRelation reln, ForkNumber forknum,+ BlockNumber blocknum, int nblocks, bool skipFsync)+{+ MdfdVec *v;+ BlockNumber curblocknum = blocknum;+ int remblocks = nblocks;++ Assert(nblocks > 0);++ /* This assert is too expensive to have on normally ... */+#ifdef CHECK_WRITE_VS_EXTEND+ Assert(blocknum >= mdnblocks(reln, forknum));+#endif++ /*+ * If a relation manages to grow to 2^32-1 blocks, refuse to extend it any+ * more --- we mustn't create a block whose number actually is+ * InvalidBlockNumber or larger.+ */+ if ((uint64) blocknum + nblocks >= (uint64) InvalidBlockNumber)+ ereport(ERROR,+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),+ errmsg("cannot extend file \"%s\" beyond %u blocks",+ relpath(reln->smgr_rlocator, forknum),+ InvalidBlockNumber)));++ while (remblocks > 0)+ {+ int segstartblock = curblocknum % ((BlockNumber) RELSEG_SIZE);+ int segendblock = (curblocknum % ((BlockNumber) RELSEG_SIZE)) + remblocks;+ off_t seekpos = (off_t) BLCKSZ * segstartblock;+ int numblocks;++ if (segendblock > RELSEG_SIZE)+ segendblock = RELSEG_SIZE;++ numblocks = segendblock - segstartblock;++ v = _mdfd_getseg(reln, forknum, curblocknum, skipFsync, EXTENSION_CREATE);++ Assert(segstartblock < RELSEG_SIZE);+ Assert(segendblock <= RELSEG_SIZE);++ /*+ * If available and useful, use posix_fallocate() (via FileAllocate())+ * to extend the relation. That's often more efficient than using+ * write(), as it commonly won't cause the kernel to allocate page+ * cache space for the extended pages.+ *+ * However, we don't use FileAllocate() for small extensions, as it+ * defeats delayed allocation on some filesystems. Not clear where+ * that decision should be made though? For now just use a cutoff of+ * 8, anything between 4 and 8 worked OK in some local testing.+ */+ if (numblocks > 8)+ {+ int ret;++ ret = FileFallocate(v->mdfd_vfd,+ seekpos, (off_t) BLCKSZ * numblocks,+ WAIT_EVENT_DATA_FILE_EXTEND);+ if (ret != 0)+ {+ ereport(ERROR,+ errcode_for_file_access(),+ errmsg("could not extend file \"%s\" with posix_fallocate(): %m",+ FilePathName(v->mdfd_vfd)),+ errhint("Check free disk space."));+ }+ }+ else+ {+ int ret;++ /*+ * Even if we don't want to use fallocate, we can still extend a+ * bit more efficiently than writing each 8kB block individually.+ * pg_pwrite_zeroes() (via FileZero()) uses+ * pg_pwritev_with_retry() to avoid multiple writes or needing a+ * zeroed buffer for the whole length of the extension.+ */+ ret = FileZero(v->mdfd_vfd,+ seekpos, (off_t) BLCKSZ * numblocks,+ WAIT_EVENT_DATA_FILE_EXTEND);+ if (ret < 0)+ ereport(ERROR,+ errcode_for_file_access(),+ errmsg("could not extend file \"%s\": %m",+ FilePathName(v->mdfd_vfd)),+ errhint("Check free disk space."));+ }++ if (!skipFsync && !SmgrIsTemp(reln))+ register_dirty_segment(reln, forknum, v);++ Assert(_mdnblocks(reln, forknum, v) <= ((BlockNumber) RELSEG_SIZE));++ remblocks -= segendblock - segstartblock;+ curblocknum += segendblock - segstartblock;+ }+}+
/*
* mdopenfork() -- Open one fork of the specified relation.
*
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.cindex dc466e54145..5224ca52592 100644--- a/src/backend/storage/smgr/smgr.c+++ b/src/backend/storage/smgr/smgr.c@@ -50,6 +50,8 @@ typedef struct f_smgr
bool isRedo);
void (*smgr_extend) (SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, const void *buffer, bool skipFsync);
+ void (*smgr_zeroextend) (SMgrRelation reln, ForkNumber forknum,+ BlockNumber blocknum, int nblocks, bool skipFsync);
bool (*smgr_prefetch) (SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum);
void (*smgr_read) (SMgrRelation reln, ForkNumber forknum,
@@ -75,6 +77,7 @@ static const f_smgr smgrsw[] = {
.smgr_exists = mdexists,
.smgr_unlink = mdunlink,
.smgr_extend = mdextend,
+ .smgr_zeroextend = mdzeroextend,
.smgr_prefetch = mdprefetch,
.smgr_read = mdread,
.smgr_write = mdwrite,
@@ -507,6 +510,31 @@ smgrextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
reln->smgr_cached_nblocks[forknum] = InvalidBlockNumber;
}
+/*+ * smgrzeroextend() -- Add new zeroed out blocks to a file.+ *+ * Similar to smgrextend(), except the relation can be extended by+ * multiple blocks at once, and that the added blocks will be filled with+ * zeroes.+ */+void+smgrzeroextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,+ int nblocks, bool skipFsync)+{+ smgrsw[reln->smgr_which].smgr_zeroextend(reln, forknum, blocknum,+ nblocks, skipFsync);++ /*+ * Normally we expect this to increase the fork size by nblocks, but if+ * the cached value isn't as expected, just invalidate it so the next call+ * asks the kernel.+ */+ if (reln->smgr_cached_nblocks[forknum] == blocknum)+ reln->smgr_cached_nblocks[forknum] = blocknum + nblocks;+ else+ reln->smgr_cached_nblocks[forknum] = InvalidBlockNumber;+}+
/*
* smgrprefetch() -- Initiate asynchronous read of the specified block of a relation.
*
--
2.38.0
[text/x-diff] v5-0004-bufmgr-Add-Pin-UnpinLocalBuffer.patch (6.7K, ../20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de/5-v5-0004-bufmgr-Add-Pin-UnpinLocalBuffer.patch)
download | inline diff:
From 4216dadd94583dac85f29c6763bb8df540abe457 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 26 Oct 2022 12:05:07 -0700
Subject: [PATCH v5 04/14] bufmgr: Add Pin/UnpinLocalBuffer()
So far these were open-coded in quite a few places, without a good reason.
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/include/storage/buf_internals.h | 2 +
src/include/storage/bufmgr.h | 2 +-
src/backend/storage/buffer/bufmgr.c | 36 +++-------------
src/backend/storage/buffer/localbuf.c | 62 +++++++++++++++++----------
4 files changed, 49 insertions(+), 53 deletions(-)
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.hindex 0b448147407..fa5c451b1a9 100644--- a/src/include/storage/buf_internals.h+++ b/src/include/storage/buf_internals.h@@ -415,6 +415,8 @@ extern int BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id);
extern void BufTableDelete(BufferTag *tagPtr, uint32 hashcode);
/* localbuf.c */
+extern bool PinLocalBuffer(BufferDesc *buf_hdr, bool adjust_usagecount);+extern void UnpinLocalBuffer(Buffer buffer);
extern PrefetchBufferResult PrefetchLocalBuffer(SMgrRelation smgr,
ForkNumber forkNum,
BlockNumber blockNum);
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.hindex 973547a8baf..d30733c65a1 100644--- a/src/include/storage/bufmgr.h+++ b/src/include/storage/bufmgr.h@@ -134,7 +134,7 @@ extern void ReleaseBuffer(Buffer buffer);
extern void UnlockReleaseBuffer(Buffer buffer);
extern void MarkBufferDirty(Buffer buffer);
extern void IncrBufferRefCount(Buffer buffer);
-extern void BufferCheckOneLocalPin(Buffer buffer);+extern void BufferCheckWePinOnce(Buffer buffer);
extern Buffer ReleaseAndReadBuffer(Buffer buffer, Relation relation,
BlockNumber blockNum);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.cindex fa20fab5a29..6f50dbd212e 100644--- a/src/backend/storage/buffer/bufmgr.c+++ b/src/backend/storage/buffer/bufmgr.c@@ -636,20 +636,7 @@ ReadRecentBuffer(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber blockN
/* Is it still valid and holding the right tag? */
if ((buf_state & BM_VALID) && BufferTagsEqual(&tag, &bufHdr->tag))
{
- /*- * Bump buffer's ref and usage counts. This is equivalent of- * PinBuffer for a shared buffer.- */- if (LocalRefCount[b] == 0)- {- if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)- {- buf_state += BUF_USAGECOUNT_ONE;- pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);- }- }- LocalRefCount[b]++;- ResourceOwnerRememberBuffer(CurrentResourceOwner, recent_buffer);+ PinLocalBuffer(bufHdr, true);
pgBufferUsage.local_blks_hit++;
@@ -1708,8 +1695,7 @@ ReleaseAndReadBuffer(Buffer buffer,
BufTagMatchesRelFileLocator(&bufHdr->tag, &relation->rd_locator) &&
BufTagGetForkNum(&bufHdr->tag) == forkNum)
return buffer;
- ResourceOwnerForgetBuffer(CurrentResourceOwner, buffer);- LocalRefCount[-buffer - 1]--;+ UnpinLocalBuffer(buffer);
}
else
{
@@ -4012,15 +3998,9 @@ ReleaseBuffer(Buffer buffer)
elog(ERROR, "bad buffer ID: %d", buffer);
if (BufferIsLocal(buffer))
- {- ResourceOwnerForgetBuffer(CurrentResourceOwner, buffer);-- Assert(LocalRefCount[-buffer - 1] > 0);- LocalRefCount[-buffer - 1]--;- return;- }-- UnpinBuffer(GetBufferDescriptor(buffer - 1));+ UnpinLocalBuffer(buffer);+ else+ UnpinBuffer(GetBufferDescriptor(buffer - 1));
}
/*
@@ -4288,18 +4268,16 @@ ConditionalLockBuffer(Buffer buffer)
}
void
-BufferCheckOneLocalPin(Buffer buffer)+BufferCheckWePinOnce(Buffer buffer)
{
if (BufferIsLocal(buffer))
{
- /* There should be exactly one pin */
if (LocalRefCount[-buffer - 1] != 1)
elog(ERROR, "incorrect local pin count: %d",
LocalRefCount[-buffer - 1]);
}
else
{
- /* There should be exactly one local pin */
if (GetPrivateRefCount(buffer) != 1)
elog(ERROR, "incorrect local pin count: %d",
GetPrivateRefCount(buffer));
@@ -4333,7 +4311,7 @@ LockBufferForCleanup(Buffer buffer)
Assert(BufferIsPinned(buffer));
Assert(PinCountWaitBuf == NULL);
- BufferCheckOneLocalPin(buffer);+ BufferCheckWePinOnce(buffer);
/* Nobody else to wait for */
if (BufferIsLocal(buffer))
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.cindex 5325ddb663d..798c5b93a87 100644--- a/src/backend/storage/buffer/localbuf.c+++ b/src/backend/storage/buffer/localbuf.c@@ -145,27 +145,8 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
fprintf(stderr, "LB ALLOC (%u,%d,%d) %d\n",
smgr->smgr_rlocator.locator.relNumber, forkNum, blockNum, -b - 1);
#endif
- buf_state = pg_atomic_read_u32(&bufHdr->state);- /* this part is equivalent to PinBuffer for a shared buffer */- if (LocalRefCount[b] == 0)- {- if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)- {- buf_state += BUF_USAGECOUNT_ONE;- pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);- }- }- LocalRefCount[b]++;- ResourceOwnerRememberBuffer(CurrentResourceOwner,- BufferDescriptorGetBuffer(bufHdr));- if (buf_state & BM_VALID)- *foundPtr = true;- else- {- /* Previous read attempt must have failed; try again */- *foundPtr = false;- }+ *foundPtr = PinLocalBuffer(bufHdr, true);
return bufHdr;
}
@@ -202,9 +183,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
else
{
/* Found a usable buffer */
- LocalRefCount[b]++;- ResourceOwnerRememberBuffer(CurrentResourceOwner,- BufferDescriptorGetBuffer(bufHdr));+ PinLocalBuffer(bufHdr, false);
break;
}
}
@@ -491,6 +470,43 @@ InitLocalBuffers(void)
NLocBuffer = nbufs;
}
+bool+PinLocalBuffer(BufferDesc *buf_hdr, bool adjust_usagecount)+{+ uint32 buf_state;+ Buffer buffer = BufferDescriptorGetBuffer(buf_hdr);+ int bufid = -(buffer + 1);++ buf_state = pg_atomic_read_u32(&buf_hdr->state);++ if (LocalRefCount[bufid] == 0)+ {+ if (adjust_usagecount &&+ BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)+ {+ buf_state += BUF_USAGECOUNT_ONE;+ pg_atomic_unlocked_write_u32(&buf_hdr->state, buf_state);+ }+ }+ LocalRefCount[bufid]++;+ ResourceOwnerRememberBuffer(CurrentResourceOwner,+ BufferDescriptorGetBuffer(buf_hdr));++ return buf_state & BM_VALID;+}++void+UnpinLocalBuffer(Buffer buffer)+{+ int buffid = -buffer - 1;++ Assert(BufferIsLocal(buffer));+ Assert(LocalRefCount[buffid] > 0);++ ResourceOwnerForgetBuffer(CurrentResourceOwner, buffer);+ LocalRefCount[buffid]--;+}+
/*
* GUC check_hook for temp_buffers
*/
--
2.38.0
[text/x-diff] v5-0005-bufmgr-Remove-buffer-write-dirty-tracepoints.patch (3.1K, ../20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de/6-v5-0005-bufmgr-Remove-buffer-write-dirty-tracepoints.patch)
download | inline diff:
From ef5aa196aff618585c9cb1466623c8207a643713 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Fri, 17 Feb 2023 18:21:57 -0800
Subject: [PATCH v5 05/14] bufmgr: Remove buffer-write-dirty tracepoints
The trace point was using the relfileno / fork / block for the to-be-read-in
buffer. Some upcoming work would make that more expensive to provide. We still
have buffer-flush-start/done, which can serve most tracing needs that
buffer-write-dirty could serve.
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/storage/buffer/bufmgr.c | 10 ----------
doc/src/sgml/monitoring.sgml | 17 -----------------
2 files changed, 27 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.cindex 6f50dbd212e..3d0683593fd 100644--- a/src/backend/storage/buffer/bufmgr.c+++ b/src/backend/storage/buffer/bufmgr.c@@ -1277,21 +1277,11 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
}
/* OK, do the I/O */
- TRACE_POSTGRESQL_BUFFER_WRITE_DIRTY_START(forkNum, blockNum,- smgr->smgr_rlocator.locator.spcOid,- smgr->smgr_rlocator.locator.dbOid,- smgr->smgr_rlocator.locator.relNumber);-
FlushBuffer(buf, NULL, IOOBJECT_RELATION, *io_context);
LWLockRelease(BufferDescriptorGetContentLock(buf));
ScheduleBufferTagForWriteback(&BackendWritebackContext,
&buf->tag);
-- TRACE_POSTGRESQL_BUFFER_WRITE_DIRTY_DONE(forkNum, blockNum,- smgr->smgr_rlocator.locator.spcOid,- smgr->smgr_rlocator.locator.dbOid,- smgr->smgr_rlocator.locator.relNumber);
}
else
{
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgmlindex c809ff1ba4a..b2ccd8d7fef 100644--- a/doc/src/sgml/monitoring.sgml+++ b/doc/src/sgml/monitoring.sgml@@ -7806,23 +7806,6 @@ FROM pg_stat_get_backend_idset() AS backendid;
it's typically not actually been written to disk yet.)
The arguments are the same as for <literal>buffer-flush-start</literal>.</entry>
</row>
- <row>- <entry><literal>buffer-write-dirty-start</literal></entry>- <entry><literal>(ForkNumber, BlockNumber, Oid, Oid, Oid)</literal></entry>- <entry>Probe that fires when a server process begins to write a dirty- buffer. (If this happens often, it implies that- <xref linkend="guc-shared-buffers"/> is too- small or the background writer control parameters need adjustment.)- arg0 and arg1 contain the fork and block numbers of the page.- arg2, arg3, and arg4 contain the tablespace, database, and relation OIDs- identifying the relation.</entry>- </row>- <row>- <entry><literal>buffer-write-dirty-done</literal></entry>- <entry><literal>(ForkNumber, BlockNumber, Oid, Oid, Oid)</literal></entry>- <entry>Probe that fires when a dirty-buffer write is complete.- The arguments are the same as for <literal>buffer-write-dirty-start</literal>.</entry>- </row>
<row>
<entry><literal>wal-buffer-write-dirty-start</literal></entry>
<entry><literal>()</literal></entry>
--
2.38.0
[text/x-diff] v5-0006-bufmgr-Acquire-and-clean-victim-buffer-separately.patch (27.8K, ../20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de/7-v5-0006-bufmgr-Acquire-and-clean-victim-buffer-separately.patch)
download | inline diff:
From 063d53b677eea7bc3a9ff2cb37d4b7fe05b52fea Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Fri, 17 Feb 2023 18:26:34 -0800
Subject: [PATCH v5 06/14] bufmgr: Acquire and clean victim buffer separately
Previously we held buffer locks for two buffer mapping partitions at the same
time to change the identity of buffers. Particularly for extending relations
needing to hold the extension lock while acquiring a victim buffer is
painful. By separating out the victim buffer acquisition, future commits will
be able to change relation extensions to scale better.
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/storage/buffer/bufmgr.c | 581 ++++++++++++++------------
src/backend/storage/buffer/localbuf.c | 115 ++---
2 files changed, 379 insertions(+), 317 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.cindex 3d0683593fd..02281303440 100644--- a/src/backend/storage/buffer/bufmgr.c+++ b/src/backend/storage/buffer/bufmgr.c@@ -473,6 +473,7 @@ static BufferDesc *BufferAlloc(SMgrRelation smgr,
BlockNumber blockNum,
BufferAccessStrategy strategy,
bool *foundPtr, IOContext *io_context);
+static Buffer GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context);
static void FlushBuffer(BufferDesc *buf, SMgrRelation reln,
IOObject io_object, IOContext io_context);
static void FindAndDropRelationBuffers(RelFileLocator rlocator,
@@ -1128,18 +1129,14 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
BufferAccessStrategy strategy,
bool *foundPtr, IOContext *io_context)
{
- bool from_ring;
BufferTag newTag; /* identity of requested block */
uint32 newHash; /* hash value for newTag */
LWLock *newPartitionLock; /* buffer partition lock for it */
- BufferTag oldTag; /* previous identity of selected buffer */- uint32 oldHash; /* hash value for oldTag */- LWLock *oldPartitionLock; /* buffer partition lock for it */- uint32 oldFlags;- int buf_id;- BufferDesc *buf;- bool valid;- uint32 buf_state;+ int existing_buf_id;++ Buffer victim_buffer;+ BufferDesc *victim_buf_hdr;+ uint32 victim_buf_state;
/* create a tag so we can lookup the buffer */
InitBufferTag(&newTag, &smgr->smgr_rlocator.locator, forkNum, blockNum);
@@ -1150,15 +1147,18 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
/* see if the block is in the buffer pool already */
LWLockAcquire(newPartitionLock, LW_SHARED);
- buf_id = BufTableLookup(&newTag, newHash);- if (buf_id >= 0)+ existing_buf_id = BufTableLookup(&newTag, newHash);+ if (existing_buf_id >= 0)
{
+ BufferDesc *buf;+ bool valid;+
/*
* Found it. Now, pin the buffer so no one can steal it from the
* buffer pool, and check to see if the correct data has been loaded
* into the buffer.
*/
- buf = GetBufferDescriptor(buf_id);+ buf = GetBufferDescriptor(existing_buf_id);
valid = PinBuffer(buf, strategy);
@@ -1200,293 +1200,111 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
*io_context = IOContextForStrategy(strategy);
- /* Loop here in case we have to try another victim buffer */- for (;;)+ /*+ * Acquire a victim buffer. Somebody else might try to do the same, we+ * don't hold any conflicting locks. If so we'll have to undo our work+ * later.+ */+ victim_buffer = GetVictimBuffer(strategy, *io_context);+ victim_buf_hdr = GetBufferDescriptor(victim_buffer - 1);++ /*+ * Try to make a hashtable entry for the buffer under its new tag. If+ * somebody else inserted another buffer for the tag, we'll release the+ * victim buffer we acquired and use the already inserted one.+ */+ LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);+ existing_buf_id = BufTableInsert(&newTag, newHash, victim_buf_hdr->buf_id);+ if (existing_buf_id >= 0)
{
- /*- * Ensure, while the spinlock's not yet held, that there's a free- * refcount entry.- */- ReservePrivateRefCountEntry();+ BufferDesc *existing_buf_hdr;+ bool valid;
/*
- * Select a victim buffer. The buffer is returned with its header- * spinlock still held!+ * Got a collision. Someone has already done what we were about to do.+ * We'll just handle this as if it were found in the buffer pool in+ * the first place. First, give up the buffer we were planning to+ * use.+ *+ * We could do this after releasing the partition lock, but then we'd+ * have to call ResourceOwnerEnlargeBuffers() &+ * ReservePrivateRefCountEntry() before acquiring the lock, for the+ * rare case of such a collision.
*/
- buf = StrategyGetBuffer(strategy, &buf_state, &from_ring);+ UnpinBuffer(victim_buf_hdr);- Assert(BUF_STATE_GET_REFCOUNT(buf_state) == 0);+ /* FIXME: Should we put the victim buffer onto the freelist? */- /* Must copy buffer flags while we still hold the spinlock */- oldFlags = buf_state & BUF_FLAG_MASK;+ /* remaining code should match code at top of routine */- /* Pin the buffer and then release the buffer spinlock */- PinBuffer_Locked(buf);+ existing_buf_hdr = GetBufferDescriptor(existing_buf_id);- /*- * If the buffer was dirty, try to write it out. There is a race- * condition here, in that someone might dirty it after we released it- * above, or even while we are writing it out (since our share-lock- * won't prevent hint-bit updates). We will recheck the dirty bit- * after re-locking the buffer header.- */- if (oldFlags & BM_DIRTY)- {- /*- * We need a share-lock on the buffer contents to write it out- * (else we might write invalid data, eg because someone else is- * compacting the page contents while we write). We must use a- * conditional lock acquisition here to avoid deadlock. Even- * though the buffer was not pinned (and therefore surely not- * locked) when StrategyGetBuffer returned it, someone else could- * have pinned and exclusive-locked it by the time we get here. If- * we try to get the lock unconditionally, we'd block waiting for- * them; if they later block waiting for us, deadlock ensues.- * (This has been observed to happen when two backends are both- * trying to split btree index pages, and the second one just- * happens to be trying to split the page the first one got from- * StrategyGetBuffer.)- */- if (LWLockConditionalAcquire(BufferDescriptorGetContentLock(buf),- LW_SHARED))- {- /*- * If using a nondefault strategy, and writing the buffer- * would require a WAL flush, let the strategy decide whether- * to go ahead and write/reuse the buffer or to choose another- * victim. We need lock to inspect the page LSN, so this- * can't be done inside StrategyGetBuffer.- */- if (strategy != NULL)- {- XLogRecPtr lsn;+ valid = PinBuffer(existing_buf_hdr, strategy);- /* Read the LSN while holding buffer header lock */- buf_state = LockBufHdr(buf);- lsn = BufferGetLSN(buf);- UnlockBufHdr(buf, buf_state);-- if (XLogNeedsFlush(lsn) &&- StrategyRejectBuffer(strategy, buf, from_ring))- {- /* Drop lock/pin and loop around for another buffer */- LWLockRelease(BufferDescriptorGetContentLock(buf));- UnpinBuffer(buf);- continue;- }- }-- /* OK, do the I/O */- FlushBuffer(buf, NULL, IOOBJECT_RELATION, *io_context);- LWLockRelease(BufferDescriptorGetContentLock(buf));-- ScheduleBufferTagForWriteback(&BackendWritebackContext,- &buf->tag);- }- else- {- /*- * Someone else has locked the buffer, so give it up and loop- * back to get another one.- */- UnpinBuffer(buf);- continue;- }- }-- /*- * To change the association of a valid buffer, we'll need to have- * exclusive lock on both the old and new mapping partitions.- */- if (oldFlags & BM_TAG_VALID)- {- /*- * Need to compute the old tag's hashcode and partition lock ID.- * XXX is it worth storing the hashcode in BufferDesc so we need- * not recompute it here? Probably not.- */- oldTag = buf->tag;- oldHash = BufTableHashCode(&oldTag);- oldPartitionLock = BufMappingPartitionLock(oldHash);-- /*- * Must lock the lower-numbered partition first to avoid- * deadlocks.- */- if (oldPartitionLock < newPartitionLock)- {- LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);- }- else if (oldPartitionLock > newPartitionLock)- {- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);- LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);- }- else- {- /* only one partition, only one lock */- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);- }- }- else- {- /* if it wasn't valid, we need only the new partition */- LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);- /* remember we have no old-partition lock or tag */- oldPartitionLock = NULL;- /* keep the compiler quiet about uninitialized variables */- oldHash = 0;- }-- /*- * Try to make a hashtable entry for the buffer under its new tag.- * This could fail because while we were writing someone else- * allocated another buffer for the same block we want to read in.- * Note that we have not yet removed the hashtable entry for the old- * tag.- */- buf_id = BufTableInsert(&newTag, newHash, buf->buf_id);-- if (buf_id >= 0)- {- /*- * Got a collision. Someone has already done what we were about to- * do. We'll just handle this as if it were found in the buffer- * pool in the first place. First, give up the buffer we were- * planning to use.- */- UnpinBuffer(buf);-- /* Can give up that buffer's mapping partition lock now */- if (oldPartitionLock != NULL &&- oldPartitionLock != newPartitionLock)- LWLockRelease(oldPartitionLock);-- /* remaining code should match code at top of routine */-- buf = GetBufferDescriptor(buf_id);-- valid = PinBuffer(buf, strategy);-- /* Can release the mapping lock as soon as we've pinned it */- LWLockRelease(newPartitionLock);-- *foundPtr = true;-- if (!valid)- {- /*- * We can only get here if (a) someone else is still reading- * in the page, or (b) a previous read attempt failed. We- * have to wait for any active read attempt to finish, and- * then set up our own read attempt if the page is still not- * BM_VALID. StartBufferIO does it all.- */- if (StartBufferIO(buf, true))- {- /*- * If we get here, previous attempts to read the buffer- * must have failed ... but we shall bravely try again.- */- *foundPtr = false;- }- }-- return buf;- }-- /*- * Need to lock the buffer header too in order to change its tag.- */- buf_state = LockBufHdr(buf);-- /*- * Somebody could have pinned or re-dirtied the buffer while we were- * doing the I/O and making the new hashtable entry. If so, we can't- * recycle this buffer; we must undo everything we've done and start- * over with a new victim buffer.- */- oldFlags = buf_state & BUF_FLAG_MASK;- if (BUF_STATE_GET_REFCOUNT(buf_state) == 1 && !(oldFlags & BM_DIRTY))- break;-- UnlockBufHdr(buf, buf_state);- BufTableDelete(&newTag, newHash);- if (oldPartitionLock != NULL &&- oldPartitionLock != newPartitionLock)- LWLockRelease(oldPartitionLock);+ /* Can release the mapping lock as soon as we've pinned it */
LWLockRelease(newPartitionLock);
- UnpinBuffer(buf);++ *foundPtr = true;++ if (!valid)+ {+ /*+ * We can only get here if (a) someone else is still reading in+ * the page, or (b) a previous read attempt failed. We have to+ * wait for any active read attempt to finish, and then set up our+ * own read attempt if the page is still not BM_VALID.+ * StartBufferIO does it all.+ */+ if (StartBufferIO(existing_buf_hdr, true))+ {+ /*+ * If we get here, previous attempts to read the buffer must+ * have failed ... but we shall bravely try again.+ */+ *foundPtr = false;+ }+ }++ return existing_buf_hdr;
}
/*
- * Okay, it's finally safe to rename the buffer.- *- * Clearing BM_VALID here is necessary, clearing the dirtybits is just- * paranoia. We also reset the usage_count since any recency of use of- * the old content is no longer relevant. (The usage_count starts out at- * 1 so that the buffer can survive one clock-sweep pass.)- *+ * Need to lock the buffer header too in order to change its tag.+ */+ victim_buf_state = LockBufHdr(victim_buf_hdr);++ /* some sanity checks while we hold the buffer header lock */+ Assert(BUF_STATE_GET_REFCOUNT(victim_buf_state) == 1);+ Assert(!(victim_buf_state & (BM_TAG_VALID | BM_VALID | BM_DIRTY | BM_IO_IN_PROGRESS)));++ victim_buf_hdr->tag = newTag;++ /*
* Make sure BM_PERMANENT is set for buffers that must be written at every
* checkpoint. Unlogged buffers only need to be written at shutdown
* checkpoints, except for their "init" forks, which need to be treated
* just like permanent relations.
*/
- buf->tag = newTag;- buf_state &= ~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED |- BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT |- BUF_USAGECOUNT_MASK);+ victim_buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;
if (relpersistence == RELPERSISTENCE_PERMANENT || forkNum == INIT_FORKNUM)
- buf_state |= BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;- else- buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;+ victim_buf_state |= BM_PERMANENT;- UnlockBufHdr(buf, buf_state);-- if (oldPartitionLock != NULL)- {- BufTableDelete(&oldTag, oldHash);- if (oldPartitionLock != newPartitionLock)- LWLockRelease(oldPartitionLock);- }+ UnlockBufHdr(victim_buf_hdr, victim_buf_state);
LWLockRelease(newPartitionLock);
- if (oldFlags & BM_VALID)- {- /*- * When a BufferAccessStrategy is in use, blocks evicted from shared- * buffers are counted as IOOP_EVICT in the corresponding context- * (e.g. IOCONTEXT_BULKWRITE). Shared buffers are evicted by a- * strategy in two cases: 1) while initially claiming buffers for the- * strategy ring 2) to replace an existing strategy ring buffer- * because it is pinned or in use and cannot be reused.- *- * Blocks evicted from buffers already in the strategy ring are- * counted as IOOP_REUSE in the corresponding strategy context.- *- * At this point, we can accurately count evictions and reuses,- * because we have successfully claimed the valid buffer. Previously,- * we may have been forced to release the buffer due to concurrent- * pinners or erroring out.- */- pgstat_count_io_op(IOOBJECT_RELATION, *io_context,- from_ring ? IOOP_REUSE : IOOP_EVICT);- }-
/*
* Buffer contents are currently invalid. Try to obtain the right to
* start I/O. If StartBufferIO returns false, then someone else managed
* to read it before we did, so there's nothing left for BufferAlloc() to
* do.
*/
- if (StartBufferIO(buf, true))+ if (StartBufferIO(victim_buf_hdr, true))
*foundPtr = false;
else
*foundPtr = true;
- return buf;+ return victim_buf_hdr;
}
/*
@@ -1595,6 +1413,237 @@ retry:
StrategyFreeBuffer(buf);
}
+/*+ * Helper routine for GetVictimBuffer()+ *+ * Needs to be called on a buffer with a valid tag, pinned, but without the+ * buffer header spinlock held.+ *+ * Returns true if the buffer can be reused, in which case the buffer is only+ * pinned by this backend and marked as invalid, false otherwise.+ */+static bool+InvalidateVictimBuffer(BufferDesc *buf_hdr)+{+ uint32 buf_state;+ uint32 hash;+ LWLock *partition_lock;+ BufferTag tag;++ Assert(GetPrivateRefCount(BufferDescriptorGetBuffer(buf_hdr)) == 1);++ /* have buffer pinned, so it's safe to read tag without lock */+ tag = buf_hdr->tag;++ hash = BufTableHashCode(&tag);+ partition_lock = BufMappingPartitionLock(hash);++ LWLockAcquire(partition_lock, LW_EXCLUSIVE);++ /* lock the buffer header */+ buf_state = LockBufHdr(buf_hdr);++ /*+ * We have the buffer pinned nobody else should have been able to unset+ * this concurrently.+ */+ Assert(buf_state & BM_TAG_VALID);+ Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);+ Assert(BufferTagsEqual(&buf_hdr->tag, &tag));++ /*+ * If somebody else pinned the buffer since, or even worse, dirtied+ * it, give up on this buffer: It's clearly in use.+ */+ if (BUF_STATE_GET_REFCOUNT(buf_state) != 1 || (buf_state & BM_DIRTY))+ {+ Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);++ UnlockBufHdr(buf_hdr, buf_state);+ LWLockRelease(partition_lock);++ return false;+ }++ /*+ * Clear out the buffer's tag and flags and usagecount. This is not+ * strictly required, as BM_TAG_VALID/BM_VALID needs to be checked before+ * doing anything with the buffer. But currently it's beneficial as the+ * pre-check for several linear scans of shared buffers just checks the+ * tag.+ */+ ClearBufferTag(&buf_hdr->tag);+ buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK);+ UnlockBufHdr(buf_hdr, buf_state);++ Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);++ /* finally delete buffer from the buffer mapping table */+ BufTableDelete(&tag, hash);++ LWLockRelease(partition_lock);++ Assert(!(buf_state & (BM_DIRTY | BM_VALID | BM_TAG_VALID)));+ Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);+ Assert(BUF_STATE_GET_REFCOUNT(pg_atomic_read_u32(&buf_hdr->state)) > 0);++ return true;+}++static Buffer+GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context)+{+ BufferDesc *buf_hdr;+ Buffer buf;+ uint32 buf_state;+ bool from_ring;++ /*+ * Ensure, while the spinlock's not yet held, that there's a free refcount+ * entry.+ */+ ReservePrivateRefCountEntry();+ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);++ /* we return here if a prospective victim buffer gets used concurrently */+again:++ /*+ * Select a victim buffer. The buffer is returned with its header+ * spinlock still held!+ */+ buf_hdr = StrategyGetBuffer(strategy, &buf_state, &from_ring);+ buf = BufferDescriptorGetBuffer(buf_hdr);++ Assert(BUF_STATE_GET_REFCOUNT(buf_state) == 0);++ /* Pin the buffer and then release the buffer spinlock */+ PinBuffer_Locked(buf_hdr);++ /*+ * We shouldn't have any other pins for this buffer.+ */+ BufferCheckWePinOnce(buf);++ /*+ * If the buffer was dirty, try to write it out. There is a race+ * condition here, in that someone might dirty it after we released the+ * buffer header lock above, or even while we are writing it out (since+ * our share-lock won't prevent hint-bit updates). We will recheck the+ * dirty bit after re-locking the buffer header.+ */+ if (buf_state & BM_DIRTY)+ {+ LWLock *content_lock;++ Assert(buf_state & BM_TAG_VALID);+ Assert(buf_state & BM_VALID);++ /*+ * We need a share-lock on the buffer contents to write it out (else+ * we might write invalid data, eg because someone else is compacting+ * the page contents while we write). We must use a conditional lock+ * acquisition here to avoid deadlock. Even though the buffer was not+ * pinned (and therefore surely not locked) when StrategyGetBuffer+ * returned it, someone else could have pinned and exclusive-locked it+ * by the time we get here. If we try to get the lock unconditionally,+ * we'd block waiting for them; if they later block waiting for us,+ * deadlock ensues. (This has been observed to happen when two+ * backends are both trying to split btree index pages, and the second+ * one just happens to be trying to split the page the first one got+ * from StrategyGetBuffer.)+ */+ content_lock = BufferDescriptorGetContentLock(buf_hdr);+ if (!LWLockConditionalAcquire(content_lock, LW_SHARED))+ {+ /*+ * Someone else has locked the buffer, so give it up and loop back+ * to get another one.+ */+ UnpinBuffer(buf_hdr);+ goto again;+ }++ /*+ * If using a nondefault strategy, and writing the buffer would+ * require a WAL flush, let the strategy decide whether to go ahead+ * and write/reuse the buffer or to choose another victim. We need+ * lock to inspect the page LSN, so this can't be done inside+ * StrategyGetBuffer.+ */+ if (strategy != NULL)+ {+ XLogRecPtr lsn;++ /* Read the LSN while holding buffer header lock */+ buf_state = LockBufHdr(buf_hdr);+ lsn = BufferGetLSN(buf_hdr);+ UnlockBufHdr(buf_hdr, buf_state);++ if (XLogNeedsFlush(lsn)+ && StrategyRejectBuffer(strategy, buf_hdr, from_ring))+ {+ LWLockRelease(content_lock);+ UnpinBuffer(buf_hdr);+ goto again;+ }+ }++ /* OK, do the I/O */+ FlushBuffer(buf_hdr, NULL, IOOBJECT_RELATION, io_context);+ LWLockRelease(content_lock);++ ScheduleBufferTagForWriteback(&BackendWritebackContext,+ &buf_hdr->tag);+ }+++ if (buf_state & BM_VALID)+ {+ /*+ * When a BufferAccessStrategy is in use, blocks evicted from shared+ * buffers are counted as IOOP_EVICT in the corresponding context+ * (e.g. IOCONTEXT_BULKWRITE). Shared buffers are evicted by a+ * strategy in two cases: 1) while initially claiming buffers for the+ * strategy ring 2) to replace an existing strategy ring buffer+ * because it is pinned or in use and cannot be reused.+ *+ * Blocks evicted from buffers already in the strategy ring are+ * counted as IOOP_REUSE in the corresponding strategy context.+ *+ * At this point, we can accurately count evictions and reuses,+ * because we have successfully claimed the valid buffer. Previously,+ * we may have been forced to release the buffer due to concurrent+ * pinners or erroring out.+ */+ pgstat_count_io_op(IOOBJECT_RELATION, io_context,+ from_ring ? IOOP_REUSE : IOOP_EVICT);+ }++ /*+ * If the buffer has an entry in the buffer mapping table, delete it. This+ * can fail because another backend could have pinned or dirtied the+ * buffer.+ */+ if ((buf_state & BM_TAG_VALID) && !InvalidateVictimBuffer(buf_hdr))+ {+ UnpinBuffer(buf_hdr);+ goto again;+ }++ /* a final set of sanity checks */+#ifdef USE_ASSERT_CHECKING+ buf_state = pg_atomic_read_u32(&buf_hdr->state);++ Assert(BUF_STATE_GET_REFCOUNT(buf_state) == 1);+ Assert(!(buf_state & (BM_TAG_VALID | BM_VALID | BM_DIRTY)));++ BufferCheckWePinOnce(buf);+#endif++ return buf;+}+
/*
* MarkBufferDirty
*
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.cindex 798c5b93a87..5b44b0be8b5 100644--- a/src/backend/storage/buffer/localbuf.c+++ b/src/backend/storage/buffer/localbuf.c@@ -45,13 +45,14 @@ BufferDesc *LocalBufferDescriptors = NULL;
Block *LocalBufferBlockPointers = NULL;
int32 *LocalRefCount = NULL;
-static int nextFreeLocalBuf = 0;+static int nextFreeLocalBufId = 0;
static HTAB *LocalBufHash = NULL;
static void InitLocalBuffers(void);
static Block GetLocalBufferStorage(void);
+static Buffer GetLocalVictimBuffer(void);
/*
@@ -113,10 +114,9 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
BufferTag newTag; /* identity of requested block */
LocalBufferLookupEnt *hresult;
BufferDesc *bufHdr;
- int b;- int trycounter;+ Buffer victim_buffer;+ int bufid;
bool found;
- uint32 buf_state;
InitBufferTag(&newTag, &smgr->smgr_rlocator.locator, forkNum, blockNum);
@@ -138,23 +138,51 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
if (hresult)
{
- b = hresult->id;- bufHdr = GetLocalBufferDescriptor(b);+ bufid = hresult->id;+ bufHdr = GetLocalBufferDescriptor(bufid);
Assert(BufferTagsEqual(&bufHdr->tag, &newTag));
-#ifdef LBDEBUG- fprintf(stderr, "LB ALLOC (%u,%d,%d) %d\n",- smgr->smgr_rlocator.locator.relNumber, forkNum, blockNum, -b - 1);-#endif
*foundPtr = PinLocalBuffer(bufHdr, true);
- return bufHdr;+ }+ else+ {+ uint32 buf_state;++ victim_buffer = GetLocalVictimBuffer();+ bufid = -(victim_buffer + 1);+ bufHdr = GetLocalBufferDescriptor(bufid);++ hresult = (LocalBufferLookupEnt *)+ hash_search(LocalBufHash, &newTag, HASH_ENTER, &found);+ if (found) /* shouldn't happen */+ elog(ERROR, "local buffer hash table corrupted");+ hresult->id = bufid;++ /*+ * it's all ours now.+ */+ bufHdr->tag = newTag;++ buf_state = pg_atomic_read_u32(&bufHdr->state);+ buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK);+ buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);++ *foundPtr = false;
}
-#ifdef LBDEBUG- fprintf(stderr, "LB ALLOC (%u,%d,%d) %d\n",- smgr->smgr_rlocator.locator.relNumber, forkNum, blockNum,- -nextFreeLocalBuf - 1);-#endif+ return bufHdr;+}++static Buffer+GetLocalVictimBuffer(void)+{+ int victim_bufid;+ int trycounter;+ uint32 buf_state;+ BufferDesc *bufHdr;++ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
/*
* Need to get a new buffer. We use a clock sweep algorithm (essentially
@@ -163,14 +191,14 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
trycounter = NLocBuffer;
for (;;)
{
- b = nextFreeLocalBuf;+ victim_bufid = nextFreeLocalBufId;- if (++nextFreeLocalBuf >= NLocBuffer)- nextFreeLocalBuf = 0;+ if (++nextFreeLocalBufId >= NLocBuffer)+ nextFreeLocalBufId = 0;- bufHdr = GetLocalBufferDescriptor(b);+ bufHdr = GetLocalBufferDescriptor(victim_bufid);- if (LocalRefCount[b] == 0)+ if (LocalRefCount[victim_bufid] == 0)
{
buf_state = pg_atomic_read_u32(&bufHdr->state);
@@ -193,6 +221,15 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
errmsg("no empty local buffer available")));
}
+ /*+ * lazy memory allocation: allocate space on first use of a buffer.+ */+ if (LocalBufHdrGetBlock(bufHdr) == NULL)+ {+ /* Set pointer for use by BufferGetBlock() macro */+ LocalBufHdrGetBlock(bufHdr) = GetLocalBufferStorage();+ }+
/*
* this buffer is not referenced but it might still be dirty. if that's
* the case, write it out before reusing it!
@@ -223,48 +260,24 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
}
/*
- * lazy memory allocation: allocate space on first use of a buffer.- */- if (LocalBufHdrGetBlock(bufHdr) == NULL)- {- /* Set pointer for use by BufferGetBlock() macro */- LocalBufHdrGetBlock(bufHdr) = GetLocalBufferStorage();- }-- /*- * Update the hash table: remove old entry, if any, and make new one.+ * Remove the victim buffer from the hashtable and mark as invalid.
*/
if (buf_state & BM_TAG_VALID)
{
+ LocalBufferLookupEnt *hresult;+
hresult = (LocalBufferLookupEnt *)
hash_search(LocalBufHash, &bufHdr->tag, HASH_REMOVE, NULL);
if (!hresult) /* shouldn't happen */
elog(ERROR, "local buffer hash table corrupted");
/* mark buffer invalid just in case hash insert fails */
ClearBufferTag(&bufHdr->tag);
- buf_state &= ~(BM_VALID | BM_TAG_VALID);+ buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK);
pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
pgstat_count_io_op(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_EVICT);
}
- hresult = (LocalBufferLookupEnt *)- hash_search(LocalBufHash, &newTag, HASH_ENTER, &found);- if (found) /* shouldn't happen */- elog(ERROR, "local buffer hash table corrupted");- hresult->id = b;-- /*- * it's all ours now.- */- bufHdr->tag = newTag;- buf_state &= ~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED | BM_IO_ERROR);- buf_state |= BM_TAG_VALID;- buf_state &= ~BUF_USAGECOUNT_MASK;- buf_state += BUF_USAGECOUNT_ONE;- pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);-- *foundPtr = false;- return bufHdr;+ return BufferDescriptorGetBuffer(bufHdr);
}
/*
@@ -431,7 +444,7 @@ InitLocalBuffers(void)
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory")));
- nextFreeLocalBuf = 0;+ nextFreeLocalBufId = 0;
/* initialize fields that need to start off nonzero */
for (i = 0; i < nbufs; i++)
--
2.38.0
[text/x-diff] v5-0007-bufmgr-Support-multiple-in-progress-IOs-by-using-.patch (12.4K, ../20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de/8-v5-0007-bufmgr-Support-multiple-in-progress-IOs-by-using-.patch)
download | inline diff:
From bb5e72f3c1dbea32199f92ec077c9b2c3bf3401b Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 24 Oct 2022 16:44:16 -0700
Subject: [PATCH v5 07/14] bufmgr: Support multiple in-progress IOs by using
resowner
---
src/include/storage/bufmgr.h | 2 +-
src/include/utils/resowner_private.h | 5 ++
src/backend/access/transam/xact.c | 4 +-
src/backend/postmaster/autovacuum.c | 1 -
src/backend/postmaster/bgwriter.c | 1 -
src/backend/postmaster/checkpointer.c | 1 -
src/backend/postmaster/walwriter.c | 1 -
src/backend/storage/buffer/bufmgr.c | 86 ++++++++++++---------------
src/backend/utils/resowner/resowner.c | 60 +++++++++++++++++++
9 files changed, 105 insertions(+), 56 deletions(-)
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.hindex d30733c65a1..9ba25521ea1 100644--- a/src/include/storage/bufmgr.h+++ b/src/include/storage/bufmgr.h@@ -181,7 +181,7 @@ extern bool ConditionalLockBufferForCleanup(Buffer buffer);
extern bool IsBufferCleanupOK(Buffer buffer);
extern bool HoldingBufferPinThatDelaysRecovery(void);
-extern void AbortBufferIO(void);+extern void AbortBufferIO(Buffer buffer);
extern void BufmgrCommit(void);
extern bool BgBufferSync(struct WritebackContext *wb_context);
diff --git a/src/include/utils/resowner_private.h b/src/include/utils/resowner_private.hindex 1b1f3181b54..ae58438ec76 100644--- a/src/include/utils/resowner_private.h+++ b/src/include/utils/resowner_private.h@@ -30,6 +30,11 @@ extern void ResourceOwnerEnlargeBuffers(ResourceOwner owner);
extern void ResourceOwnerRememberBuffer(ResourceOwner owner, Buffer buffer);
extern void ResourceOwnerForgetBuffer(ResourceOwner owner, Buffer buffer);
+/* support for IO-in-progress management */+extern void ResourceOwnerEnlargeBufferIOs(ResourceOwner owner);+extern void ResourceOwnerRememberBufferIO(ResourceOwner owner, Buffer buffer);+extern void ResourceOwnerForgetBufferIO(ResourceOwner owner, Buffer buffer);+
/* support for local lock management */
extern void ResourceOwnerRememberLock(ResourceOwner owner, LOCALLOCK *locallock);
extern void ResourceOwnerForgetLock(ResourceOwner owner, LOCALLOCK *locallock);
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.cindex b8764012607..a0f53e9936a 100644--- a/src/backend/access/transam/xact.c+++ b/src/backend/access/transam/xact.c@@ -2725,8 +2725,7 @@ AbortTransaction(void)
pgstat_report_wait_end();
pgstat_progress_end_command();
- /* Clean up buffer I/O and buffer context locks, too */- AbortBufferIO();+ /* Clean up buffer context locks, too */
UnlockBuffers();
/* Reset WAL record construction state */
@@ -5086,7 +5085,6 @@ AbortSubTransaction(void)
pgstat_report_wait_end();
pgstat_progress_end_command();
- AbortBufferIO();
UnlockBuffers();
/* Reset WAL record construction state */
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.cindex 585d28148ca..e9ba0dc17cd 100644--- a/src/backend/postmaster/autovacuum.c+++ b/src/backend/postmaster/autovacuum.c@@ -526,7 +526,6 @@ AutoVacLauncherMain(int argc, char *argv[])
*/
LWLockReleaseAll();
pgstat_report_wait_end();
- AbortBufferIO();
UnlockBuffers();
/* this is probably dead code, but let's be safe: */
if (AuxProcessResourceOwner)
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.cindex 9bb47da404d..caad642ec93 100644--- a/src/backend/postmaster/bgwriter.c+++ b/src/backend/postmaster/bgwriter.c@@ -167,7 +167,6 @@ BackgroundWriterMain(void)
*/
LWLockReleaseAll();
ConditionVariableCancelSleep();
- AbortBufferIO();
UnlockBuffers();
ReleaseAuxProcessResources(false);
AtEOXact_Buffers(false);
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.cindex aaad5c52281..ace9893d957 100644--- a/src/backend/postmaster/checkpointer.c+++ b/src/backend/postmaster/checkpointer.c@@ -271,7 +271,6 @@ CheckpointerMain(void)
LWLockReleaseAll();
ConditionVariableCancelSleep();
pgstat_report_wait_end();
- AbortBufferIO();
UnlockBuffers();
ReleaseAuxProcessResources(false);
AtEOXact_Buffers(false);
diff --git a/src/backend/postmaster/walwriter.c b/src/backend/postmaster/walwriter.cindex 513e580c513..65e84be39b9 100644--- a/src/backend/postmaster/walwriter.c+++ b/src/backend/postmaster/walwriter.c@@ -163,7 +163,6 @@ WalWriterMain(void)
LWLockReleaseAll();
ConditionVariableCancelSleep();
pgstat_report_wait_end();
- AbortBufferIO();
UnlockBuffers();
ReleaseAuxProcessResources(false);
AtEOXact_Buffers(false);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.cindex 02281303440..41153f99bc2 100644--- a/src/backend/storage/buffer/bufmgr.c+++ b/src/backend/storage/buffer/bufmgr.c@@ -159,10 +159,6 @@ int checkpoint_flush_after = DEFAULT_CHECKPOINT_FLUSH_AFTER;
int bgwriter_flush_after = DEFAULT_BGWRITER_FLUSH_AFTER;
int backend_flush_after = DEFAULT_BACKEND_FLUSH_AFTER;
-/* local state for StartBufferIO and related functions */-static BufferDesc *InProgressBuf = NULL;-static bool IsForInput;-
/* local state for LockBufferForCleanup */
static BufferDesc *PinCountWaitBuf = NULL;
@@ -2712,7 +2708,6 @@ InitBufferPoolAccess(void)
static void
AtProcExit_Buffers(int code, Datum arg)
{
- AbortBufferIO();
UnlockBuffers();
CheckForBufferLeaks();
@@ -4658,7 +4653,7 @@ StartBufferIO(BufferDesc *buf, bool forInput)
{
uint32 buf_state;
- Assert(!InProgressBuf);+ ResourceOwnerEnlargeBufferIOs(CurrentResourceOwner);
for (;;)
{
@@ -4682,8 +4677,8 @@ StartBufferIO(BufferDesc *buf, bool forInput)
buf_state |= BM_IO_IN_PROGRESS;
UnlockBufHdr(buf, buf_state);
- InProgressBuf = buf;- IsForInput = forInput;+ ResourceOwnerRememberBufferIO(CurrentResourceOwner,+ BufferDescriptorGetBuffer(buf));
return true;
}
@@ -4709,8 +4704,6 @@ TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint32 set_flag_bits)
{
uint32 buf_state;
- Assert(buf == InProgressBuf);-
buf_state = LockBufHdr(buf);
Assert(buf_state & BM_IO_IN_PROGRESS);
@@ -4722,13 +4715,14 @@ TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint32 set_flag_bits)
buf_state |= set_flag_bits;
UnlockBufHdr(buf, buf_state);
- InProgressBuf = NULL;+ ResourceOwnerForgetBufferIO(CurrentResourceOwner,+ BufferDescriptorGetBuffer(buf));
ConditionVariableBroadcast(BufferDescriptorGetIOCV(buf));
}
/*
- * AbortBufferIO: Clean up any active buffer I/O after an error.+ * AbortBufferIO: Clean up active buffer I/O after an error.
*
* All LWLocks we might have held have been released,
* but we haven't yet released buffer pins, so the buffer is still pinned.
@@ -4737,46 +4731,42 @@ TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint32 set_flag_bits)
* possible the error condition wasn't related to the I/O.
*/
void
-AbortBufferIO(void)+AbortBufferIO(Buffer buf)
{
- BufferDesc *buf = InProgressBuf;+ BufferDesc *buf_hdr = GetBufferDescriptor(buf - 1);+ uint32 buf_state;- if (buf)+ buf_state = LockBufHdr(buf_hdr);+ Assert(buf_state & (BM_IO_IN_PROGRESS | BM_TAG_VALID));++ if (!(buf_state & BM_VALID))
{
- uint32 buf_state;-- buf_state = LockBufHdr(buf);- Assert(buf_state & BM_IO_IN_PROGRESS);- if (IsForInput)- {- Assert(!(buf_state & BM_DIRTY));-- /* We'd better not think buffer is valid yet */- Assert(!(buf_state & BM_VALID));- UnlockBufHdr(buf, buf_state);- }- else- {- Assert(buf_state & BM_DIRTY);- UnlockBufHdr(buf, buf_state);- /* Issue notice if this is not the first failure... */- if (buf_state & BM_IO_ERROR)- {- /* Buffer is pinned, so we can read tag without spinlock */- char *path;-- path = relpathperm(BufTagGetRelFileLocator(&buf->tag),- BufTagGetForkNum(&buf->tag));- ereport(WARNING,- (errcode(ERRCODE_IO_ERROR),- errmsg("could not write block %u of %s",- buf->tag.blockNum, path),- errdetail("Multiple failures --- write error might be permanent.")));- pfree(path);- }- }- TerminateBufferIO(buf, false, BM_IO_ERROR);+ Assert(!(buf_state & BM_DIRTY));+ UnlockBufHdr(buf_hdr, buf_state);
}
+ else+ {+ Assert(!(buf_state & BM_DIRTY));+ UnlockBufHdr(buf_hdr, buf_state);++ /* Issue notice if this is not the first failure... */+ if (buf_state & BM_IO_ERROR)+ {+ /* Buffer is pinned, so we can read tag without spinlock */+ char *path;++ path = relpathperm(BufTagGetRelFileLocator(&buf_hdr->tag),+ BufTagGetForkNum(&buf_hdr->tag));+ ereport(WARNING,+ (errcode(ERRCODE_IO_ERROR),+ errmsg("could not write block %u of %s",+ buf_hdr->tag.blockNum, path),+ errdetail("Multiple failures --- write error might be permanent.")));+ pfree(path);+ }+ }++ TerminateBufferIO(buf_hdr, false, BM_IO_ERROR);
}
/*
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.cindex 19b6241e45d..fccc59b39dd 100644--- a/src/backend/utils/resowner/resowner.c+++ b/src/backend/utils/resowner/resowner.c@@ -121,6 +121,7 @@ typedef struct ResourceOwnerData
/* We have built-in support for remembering: */
ResourceArray bufferarr; /* owned buffers */
+ ResourceArray bufferioarr; /* in-progress buffer IO */
ResourceArray catrefarr; /* catcache references */
ResourceArray catlistrefarr; /* catcache-list pins */
ResourceArray relrefarr; /* relcache references */
@@ -441,6 +442,7 @@ ResourceOwnerCreate(ResourceOwner parent, const char *name)
}
ResourceArrayInit(&(owner->bufferarr), BufferGetDatum(InvalidBuffer));
+ ResourceArrayInit(&(owner->bufferioarr), BufferGetDatum(InvalidBuffer));
ResourceArrayInit(&(owner->catrefarr), PointerGetDatum(NULL));
ResourceArrayInit(&(owner->catlistrefarr), PointerGetDatum(NULL));
ResourceArrayInit(&(owner->relrefarr), PointerGetDatum(NULL));
@@ -517,6 +519,24 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
if (phase == RESOURCE_RELEASE_BEFORE_LOCKS)
{
+ /*+ * Abort failed buffer IO. AbortBufferIO()->TerminateBufferIO() calls+ * ResourceOwnerForgetBufferIOs(), so we just have to iterate till+ * there are none.+ *+ * Needs to be before we release buffer pins.+ *+ * During a commit, there shouldn't be any in-progress IO.+ */+ while (ResourceArrayGetAny(&(owner->bufferioarr), &foundres))+ {+ Buffer res = DatumGetBuffer(foundres);++ if (isCommit)+ elog(PANIC, "lost track of buffer IO on buffer %u", res);+ AbortBufferIO(res);+ }+
/*
* Release buffer pins. Note that ReleaseBuffer will remove the
* buffer entry from our array, so we just have to iterate till there
@@ -746,6 +766,7 @@ ResourceOwnerDelete(ResourceOwner owner)
/* And it better not own any resources, either */
Assert(owner->bufferarr.nitems == 0);
+ Assert(owner->bufferioarr.nitems == 0);
Assert(owner->catrefarr.nitems == 0);
Assert(owner->catlistrefarr.nitems == 0);
Assert(owner->relrefarr.nitems == 0);
@@ -775,6 +796,7 @@ ResourceOwnerDelete(ResourceOwner owner)
/* And free the object. */
ResourceArrayFree(&(owner->bufferarr));
+ ResourceArrayFree(&(owner->bufferioarr));
ResourceArrayFree(&(owner->catrefarr));
ResourceArrayFree(&(owner->catlistrefarr));
ResourceArrayFree(&(owner->relrefarr));
@@ -976,6 +998,44 @@ ResourceOwnerForgetBuffer(ResourceOwner owner, Buffer buffer)
buffer, owner->name);
}
++/*+ * Make sure there is room for at least one more entry in a ResourceOwner's+ * buffer array.+ *+ * This is separate from actually inserting an entry because if we run out+ * of memory, it's critical to do so *before* acquiring the resource.+ */+void+ResourceOwnerEnlargeBufferIOs(ResourceOwner owner)+{+ /* We used to allow pinning buffers without a resowner, but no more */+ Assert(owner != NULL);+ ResourceArrayEnlarge(&(owner->bufferioarr));+}++/*+ * Remember that a buffer IO is owned by a ResourceOwner+ *+ * Caller must have previously done ResourceOwnerEnlargeBufferIOs()+ */+void+ResourceOwnerRememberBufferIO(ResourceOwner owner, Buffer buffer)+{+ ResourceArrayAdd(&(owner->bufferioarr), BufferGetDatum(buffer));+}++/*+ * Forget that a buffer IO is owned by a ResourceOwner+ */+void+ResourceOwnerForgetBufferIO(ResourceOwner owner, Buffer buffer)+{+ if (!ResourceArrayRemove(&(owner->bufferioarr), BufferGetDatum(buffer)))+ elog(PANIC, "buffer IO %d is not owned by resource owner %s",+ buffer, owner->name);+}+
/*
* Remember that a Local Lock is owned by a ResourceOwner
*
--
2.38.0
[text/x-diff] v5-0008-bufmgr-Move-relation-extension-handling-into-Exte.patch (43.8K, ../20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de/9-v5-0008-bufmgr-Move-relation-extension-handling-into-Exte.patch)
download | inline diff:
From 8f256d4f260921a93ad9b0f66d4a97218036059d Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 1 Mar 2023 13:24:19 -0800
Subject: [PATCH v5 08/14] bufmgr: Move relation extension handling into
ExtendBufferedRel{By,To,}
---
src/include/pgstat.h | 1 +
src/include/storage/buf_internals.h | 7 +
src/include/storage/bufmgr.h | 65 ++
src/backend/storage/buffer/bufmgr.c | 782 +++++++++++++++++++------
src/backend/storage/buffer/localbuf.c | 156 ++++-
src/backend/utils/activity/pgstat_io.c | 8 +-
src/backend/utils/probes.d | 6 +-
doc/src/sgml/monitoring.sgml | 43 +-
8 files changed, 886 insertions(+), 182 deletions(-)
diff --git a/src/include/pgstat.h b/src/include/pgstat.hindex c2bae8358a2..4aac1115664 100644--- a/src/include/pgstat.h+++ b/src/include/pgstat.h@@ -500,6 +500,7 @@ extern PgStat_CheckpointerStats *pgstat_fetch_stat_checkpointer(void);
extern bool pgstat_bktype_io_stats_valid(PgStat_BktypeIO *context_ops,
BackendType bktype);
extern void pgstat_count_io_op(IOObject io_object, IOContext io_context, IOOp io_op);
+extern void pgstat_count_io_op_n(IOObject io_object, IOContext io_context, IOOp io_op, uint32 cnt);
extern PgStat_IO *pgstat_fetch_stat_io(void);
extern const char *pgstat_get_io_context_name(IOContext io_context);
extern const char *pgstat_get_io_object_name(IOObject io_object);
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.hindex fa5c451b1a9..feca19f5620 100644--- a/src/include/storage/buf_internals.h+++ b/src/include/storage/buf_internals.h@@ -422,6 +422,13 @@ extern PrefetchBufferResult PrefetchLocalBuffer(SMgrRelation smgr,
BlockNumber blockNum);
extern BufferDesc *LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum,
BlockNumber blockNum, bool *foundPtr, IOContext *io_context);
+extern BlockNumber ExtendBufferedRelLocal(ExtendBufferedWhat eb,+ ForkNumber fork,+ uint32 flags,+ uint32 extend_by,+ BlockNumber extend_upto,+ Buffer *buffers,+ uint32 *extended_by);
extern void MarkLocalBufferDirty(Buffer buffer);
extern void DropRelationLocalBuffers(RelFileLocator rlocator,
ForkNumber forkNum,
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.hindex 9ba25521ea1..da0abf43150 100644--- a/src/include/storage/bufmgr.h+++ b/src/include/storage/bufmgr.h@@ -60,6 +60,53 @@ typedef struct PrefetchBufferResult
bool initiated_io; /* If true, a miss resulting in async I/O */
} PrefetchBufferResult;
+/*+ * Flags influencing the behaviour of ExtendBufferedRel*+ */+typedef enum ExtendBufferedFlags+{+ /*+ * Don't acquire extension lock. This is safe only if the relation isn't+ * shared, an access exclusive lock is held or if this is the startup+ * process.+ */+ EB_SKIP_EXTENSION_LOCK = (1 << 0),++ /* Is this extension part of recovery? */+ EB_PERFORMING_RECOVERY = (1 << 1),++ /*+ * Should the fork be created if it does not currently exist? This likely+ * only ever makes sense for relation forks.+ */+ EB_CREATE_FORK_IF_NEEDED = (1 << 2),++ /* Should the first (possibly only) return buffer be returned locked? */+ EB_LOCK_FIRST = (1 << 3),++ /* Should the smgr size cache be cleared? */+ EB_CLEAR_SIZE_CACHE = (1 << 4),++ /* internal flags follow */+ EB_LOCK_TARGET = (1 << 5),+} ExtendBufferedFlags;++/*+ * To identify the relation - either relation or smgr + relpersistence has to+ * be specified. Used via the EB_REL()/EB_SMGR() macros below. This allows us+ * to use the same function for both crash recovery and normal operation.+ */+typedef struct ExtendBufferedWhat+{+ Relation rel;+ struct SMgrRelationData *smgr;+ char relpersistence;+} ExtendBufferedWhat;++#define EB_REL(p_rel) ((ExtendBufferedWhat){.rel = p_rel})+#define EB_SMGR(p_smgr, p_relpersistence) ((ExtendBufferedWhat){.smgr = p_smgr, .relpersistence = p_relpersistence})++
/* forward declared, to avoid having to expose buf_internals.h here */
struct WritebackContext;
@@ -138,6 +185,24 @@ extern void BufferCheckWePinOnce(Buffer buffer);
extern Buffer ReleaseAndReadBuffer(Buffer buffer, Relation relation,
BlockNumber blockNum);
+extern Buffer ExtendBufferedRel(ExtendBufferedWhat eb,+ ForkNumber forkNum,+ BufferAccessStrategy strategy,+ uint32 flags);+extern BlockNumber ExtendBufferedRelBy(ExtendBufferedWhat eb,+ ForkNumber fork,+ BufferAccessStrategy strategy,+ uint32 flags,+ uint32 extend_by,+ Buffer *buffers,+ uint32 *extended_by);+extern Buffer ExtendBufferedRelTo(ExtendBufferedWhat eb,+ ForkNumber fork,+ BufferAccessStrategy strategy,+ uint32 flags,+ BlockNumber extend_to,+ ReadBufferMode mode);+
extern void InitBufferPoolAccess(void);
extern void AtEOXact_Buffers(bool isCommit);
extern void PrintBufferLeakWarning(Buffer buffer);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.cindex 41153f99bc2..699eab5243d 100644--- a/src/backend/storage/buffer/bufmgr.c+++ b/src/backend/storage/buffer/bufmgr.c@@ -48,6 +48,7 @@
#include "storage/buf_internals.h"
#include "storage/bufmgr.h"
#include "storage/ipc.h"
+#include "storage/lmgr.h"
#include "storage/proc.h"
#include "storage/smgr.h"
#include "storage/standby.h"
@@ -450,6 +451,22 @@ static Buffer ReadBuffer_common(SMgrRelation smgr, char relpersistence,
ForkNumber forkNum, BlockNumber blockNum,
ReadBufferMode mode, BufferAccessStrategy strategy,
bool *hit);
+static BlockNumber ExtendBufferedRelCommon(ExtendBufferedWhat eb,+ ForkNumber fork,+ BufferAccessStrategy strategy,+ uint32 flags,+ uint32 extend_by,+ BlockNumber extend_upto,+ Buffer *buffers,+ uint32 *extended_by);+static BlockNumber ExtendBufferedRelShared(ExtendBufferedWhat eb,+ ForkNumber fork,+ BufferAccessStrategy strategy,+ uint32 flags,+ uint32 extend_by,+ BlockNumber extend_upto,+ Buffer *buffers,+ uint32 *extended_by);
static bool PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy);
static void PinBuffer_Locked(BufferDesc *buf);
static void UnpinBuffer(BufferDesc *buf);
@@ -785,6 +802,179 @@ ReadBufferWithoutRelcache(RelFileLocator rlocator, ForkNumber forkNum,
mode, strategy, &hit);
}
+/*+ * Convenience wrapper around ExtendBufferedRelBy() extending by one block.+ */+Buffer+ExtendBufferedRel(ExtendBufferedWhat eb,+ ForkNumber forkNum,+ BufferAccessStrategy strategy,+ uint32 flags)+{+ Buffer buf;+ uint32 extend_by = 1;++ ExtendBufferedRelBy(eb, forkNum, strategy, flags, extend_by,+ &buf, &extend_by);++ return buf;+}++/*+ * Extend relation by multiple blocks.+ *+ * Tries to extend the relation by extend_by blocks. Depending on the+ * availability of resources the relation may end up being extended by a+ * smaller number of pages (unless an error is thrown, always by at least one+ * page). *extended_by is updated to the number of pages the relation has been+ * extended to.+ *+ * buffers needs to be an array that is at least extend_by long. Upon+ * completion, the first extend_by array elements will point to a pinned+ * buffer.+ *+ * If EB_LOCK_FIRST is part of flags, the first returned buffer is+ * locked. This is useful for callers that want a buffer that is guaranteed to+ * be empty.+ */+BlockNumber+ExtendBufferedRelBy(ExtendBufferedWhat eb,+ ForkNumber fork,+ BufferAccessStrategy strategy,+ uint32 flags,+ uint32 extend_by,+ Buffer *buffers,+ uint32 *extended_by)+{+ Assert((eb.rel != NULL) ^ (eb.smgr != NULL));+ Assert(eb.smgr == NULL || eb.relpersistence != 0);+ Assert(extend_by > 0);++ if (eb.smgr == NULL)+ {+ eb.smgr = RelationGetSmgr(eb.rel);+ eb.relpersistence = eb.rel->rd_rel->relpersistence;+ }++ return ExtendBufferedRelCommon(eb, fork, strategy, flags,+ extend_by, InvalidBlockNumber,+ buffers, extended_by);+}++/*+ * Extend the relation so it is at least extend_to blocks large, read buffer+ * (extend_to - 1).+ *+ * This is useful for callers that want to write a specific page, regardless+ * of the current size of the relation (e.g. useful for visibilitymap and for+ * crash recovery).+ */+Buffer+ExtendBufferedRelTo(ExtendBufferedWhat eb,+ ForkNumber fork,+ BufferAccessStrategy strategy,+ uint32 flags,+ BlockNumber extend_to,+ ReadBufferMode mode)+{+ BlockNumber current_size;+ uint32 extended_by = 0;+ Buffer buffer = InvalidBuffer;+ Buffer buffers[64];++ Assert((eb.rel != NULL) ^ (eb.smgr != NULL));+ Assert(eb.smgr == NULL || eb.relpersistence != 0);+ Assert(extend_to != InvalidBlockNumber && extend_to > 0);+ Assert(mode == RBM_NORMAL || mode == RBM_ZERO_ON_ERROR ||+ mode == RBM_ZERO_AND_LOCK);++ if (eb.smgr == NULL)+ {+ eb.smgr = RelationGetSmgr(eb.rel);+ eb.relpersistence = eb.rel->rd_rel->relpersistence;+ }++ /*+ * If desired, create the file if it doesn't exist. If+ * smgr_cached_nblocks[fork] is positive then it must exist, no need for+ * an smgrexists call.+ */+ if ((flags & EB_CREATE_FORK_IF_NEEDED) &&+ (eb.smgr->smgr_cached_nblocks[fork] == 0 ||+ eb.smgr->smgr_cached_nblocks[fork] == InvalidBlockNumber) &&+ !smgrexists(eb.smgr, fork))+ {+ LockRelationForExtension(eb.rel, ExclusiveLock);++ /* could have been closed while waiting for lock */+ eb.smgr = RelationGetSmgr(eb.rel);++ /* recheck, fork might have been created concurrently */+ if (!smgrexists(eb.smgr, fork))+ smgrcreate(eb.smgr, fork, flags & EB_PERFORMING_RECOVERY);++ UnlockRelationForExtension(eb.rel, ExclusiveLock);+ }++ /*+ * If requested, invalidate size cache, so that smgrnblocks asks the+ * kernel.+ */+ if (flags & EB_CLEAR_SIZE_CACHE)+ eb.smgr->smgr_cached_nblocks[fork] = InvalidBlockNumber;++ /*+ * Estimate how many pages we'll need to extend by. This avoids acquiring+ * unnecessarily many victim buffers.+ */+ current_size = smgrnblocks(eb.smgr, fork);++ if (mode == RBM_ZERO_AND_LOCK)+ flags |= EB_LOCK_TARGET;++ while (current_size < extend_to)+ {+ uint32 num_pages = lengthof(buffers);+ BlockNumber first_block;++ if ((uint64) current_size + num_pages > extend_to)+ num_pages = extend_to - current_size;++ first_block = ExtendBufferedRelCommon(eb, fork, strategy, flags,+ num_pages, extend_to,+ buffers, &extended_by);++ current_size = first_block + extended_by;+ Assert(current_size <= extend_to);+ Assert(num_pages != 0 || current_size >= extend_to);++ for (int i = 0; i < extended_by; i++)+ {+ if (first_block + i != extend_to - 1)+ ReleaseBuffer(buffers[i]);+ else+ buffer = buffers[i];+ }+ }++ /*+ * It's possible that another backend concurrently extended the+ * relation. In that case read the buffer.+ *+ * XXX: Should we control this via a flag?+ */+ if (buffer == InvalidBuffer)+ {+ bool hit;++ Assert(extended_by == 0);+ buffer = ReadBuffer_common(eb.smgr, eb.relpersistence,+ fork, extend_to - 1, mode, strategy,+ &hit);+ }++ return buffer;+}
/*
* ReadBuffer_common -- common logic for all ReadBuffer variants
@@ -801,35 +991,36 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
bool found;
IOContext io_context;
IOObject io_object;
- bool isExtend;
bool isLocalBuf = SmgrIsTemp(smgr);
*hit = false;
+ /*+ * Backward compatibility path, most code should use+ * ExtendRelationBuffered() instead, as acquiring the extension lock+ * inside ExtendRelationBuffered() scales a lot better.+ */+ if (unlikely(blockNum == P_NEW))+ {+ uint32 flags = EB_SKIP_EXTENSION_LOCK;++ Assert(mode == RBM_NORMAL || mode == RBM_ZERO_AND_LOCK);++ if (mode == RBM_ZERO_AND_LOCK)+ flags |= EB_LOCK_FIRST;++ return ExtendBufferedRel(EB_SMGR(smgr, relpersistence),+ forkNum, strategy, flags);+ }+
/* Make sure we will have room to remember the buffer pin */
ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
- isExtend = (blockNum == P_NEW);-
TRACE_POSTGRESQL_BUFFER_READ_START(forkNum, blockNum,
smgr->smgr_rlocator.locator.spcOid,
smgr->smgr_rlocator.locator.dbOid,
smgr->smgr_rlocator.locator.relNumber,
- smgr->smgr_rlocator.backend,- isExtend);-- /* Substitute proper block number if caller asked for P_NEW */- if (isExtend)- {- blockNum = smgrnblocks(smgr, forkNum);- /* Fail if relation is already at maximum possible length */- if (blockNum == P_NEW)- ereport(ERROR,- (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),- errmsg("cannot extend relation %s beyond %u blocks",- relpath(smgr->smgr_rlocator, forkNum),- P_NEW)));- }+ smgr->smgr_rlocator.backend);
if (isLocalBuf)
{
@@ -843,8 +1034,6 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
bufHdr = LocalBufferAlloc(smgr, forkNum, blockNum, &found, &io_context);
if (found)
pgBufferUsage.local_blks_hit++;
- else if (isExtend)- pgBufferUsage.local_blks_written++;
else if (mode == RBM_NORMAL || mode == RBM_NORMAL_NO_LOG ||
mode == RBM_ZERO_ON_ERROR)
pgBufferUsage.local_blks_read++;
@@ -859,8 +1048,6 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
strategy, &found, &io_context);
if (found)
pgBufferUsage.shared_blks_hit++;
- else if (isExtend)- pgBufferUsage.shared_blks_written++;
else if (mode == RBM_NORMAL || mode == RBM_NORMAL_NO_LOG ||
mode == RBM_ZERO_ON_ERROR)
pgBufferUsage.shared_blks_read++;
@@ -871,103 +1058,40 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
/* if it was already in the buffer pool, we're done */
if (found)
{
- if (!isExtend)- {- /* Just need to update stats before we exit */- *hit = true;- VacuumPageHit++;+ /* Just need to update stats before we exit */+ *hit = true;+ VacuumPageHit++;- if (VacuumCostActive)- VacuumCostBalance += VacuumCostPageHit;+ if (VacuumCostActive)+ VacuumCostBalance += VacuumCostPageHit;- TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,- smgr->smgr_rlocator.locator.spcOid,- smgr->smgr_rlocator.locator.dbOid,- smgr->smgr_rlocator.locator.relNumber,- smgr->smgr_rlocator.backend,- isExtend,- found);-- /*- * In RBM_ZERO_AND_LOCK mode the caller expects the page to be- * locked on return.- */- if (!isLocalBuf)- {- if (mode == RBM_ZERO_AND_LOCK)- LWLockAcquire(BufferDescriptorGetContentLock(bufHdr),- LW_EXCLUSIVE);- else if (mode == RBM_ZERO_AND_CLEANUP_LOCK)- LockBufferForCleanup(BufferDescriptorGetBuffer(bufHdr));- }-- return BufferDescriptorGetBuffer(bufHdr);- }+ TRACE_POSTGRESQL_BUFFER_READ_DONE(forkNum, blockNum,+ smgr->smgr_rlocator.locator.spcOid,+ smgr->smgr_rlocator.locator.dbOid,+ smgr->smgr_rlocator.locator.relNumber,+ smgr->smgr_rlocator.backend,+ found);
/*
- * We get here only in the corner case where we are trying to extend- * the relation but we found a pre-existing buffer marked BM_VALID.- * This can happen because mdread doesn't complain about reads beyond- * EOF (when zero_damaged_pages is ON) and so a previous attempt to- * read a block beyond EOF could have left a "valid" zero-filled- * buffer. Unfortunately, we have also seen this case occurring- * because of buggy Linux kernels that sometimes return an- * lseek(SEEK_END) result that doesn't account for a recent write. In- * that situation, the pre-existing buffer would contain valid data- * that we don't want to overwrite. Since the legitimate case should- * always have left a zero-filled buffer, complain if not PageIsNew.+ * In RBM_ZERO_AND_LOCK mode the caller expects the page to be locked+ * on return.
*/
- bufBlock = isLocalBuf ? LocalBufHdrGetBlock(bufHdr) : BufHdrGetBlock(bufHdr);- if (!PageIsNew((Page) bufBlock))- ereport(ERROR,- (errmsg("unexpected data beyond EOF in block %u of relation %s",- blockNum, relpath(smgr->smgr_rlocator, forkNum)),- errhint("This has been seen to occur with buggy kernels; consider updating your system.")));-- /*- * We *must* do smgrextend before succeeding, else the page will not- * be reserved by the kernel, and the next P_NEW call will decide to- * return the same page. Clear the BM_VALID bit, do the StartBufferIO- * call that BufferAlloc didn't, and proceed.- */- if (isLocalBuf)+ if (!isLocalBuf)
{
- /* Only need to adjust flags */- uint32 buf_state = pg_atomic_read_u32(&bufHdr->state);-- Assert(buf_state & BM_VALID);- buf_state &= ~BM_VALID;- pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);+ if (mode == RBM_ZERO_AND_LOCK)+ LWLockAcquire(BufferDescriptorGetContentLock(bufHdr),+ LW_EXCLUSIVE);+ else if (mode == RBM_ZERO_AND_CLEANUP_LOCK)+ LockBufferForCleanup(BufferDescriptorGetBuffer(bufHdr));
}
- else- {- /*- * Loop to handle the very small possibility that someone re-sets- * BM_VALID between our clearing it and StartBufferIO inspecting- * it.- */- do- {- uint32 buf_state = LockBufHdr(bufHdr);- Assert(buf_state & BM_VALID);- buf_state &= ~BM_VALID;- UnlockBufHdr(bufHdr, buf_state);- } while (!StartBufferIO(bufHdr, true));- }+ return BufferDescriptorGetBuffer(bufHdr);
}
/*
* if we have gotten to this point, we have allocated a buffer for the
* page but its contents are not yet valid. IO_IN_PROGRESS is set for it,
* if it's a shared buffer.
- *- * Note: if smgrextend fails, we will end up with a buffer that is- * allocated but not marked BM_VALID. P_NEW will still select the same- * block number (because the relation didn't get any longer on disk) and- * so future attempts to extend the relation will find the same buffer (if- * it's not been recycled) but come right back here to try smgrextend- * again.
*/
Assert(!(pg_atomic_read_u32(&bufHdr->state) & BM_VALID)); /* spinlock not needed */
@@ -982,72 +1106,51 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
io_object = IOOBJECT_RELATION;
}
- if (isExtend)- {- /* new buffers are zero-filled */+ /*+ * Read in the page, unless the caller intends to overwrite it and just+ * wants us to allocate a buffer.+ */+ if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK)
MemSet((char *) bufBlock, 0, BLCKSZ);
- /* don't set checksum for all-zero page */- smgrextend(smgr, forkNum, blockNum, bufBlock, false);-- pgstat_count_io_op(io_object, io_context, IOOP_EXTEND);-- /*- * NB: we're *not* doing a ScheduleBufferTagForWriteback here;- * although we're essentially performing a write. At least on linux- * doing so defeats the 'delayed allocation' mechanism, leading to- * increased file fragmentation.- */- }
else
{
- /*- * Read in the page, unless the caller intends to overwrite it and- * just wants us to allocate a buffer.- */- if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK)- MemSet((char *) bufBlock, 0, BLCKSZ);- else+ instr_time io_start,+ io_time;++ if (track_io_timing)+ INSTR_TIME_SET_CURRENT(io_start);++ smgrread(smgr, forkNum, blockNum, bufBlock);++ if (track_io_timing)
{
- instr_time io_start,- io_time;+ INSTR_TIME_SET_CURRENT(io_time);+ INSTR_TIME_SUBTRACT(io_time, io_start);+ pgstat_count_buffer_read_time(INSTR_TIME_GET_MICROSEC(io_time));+ INSTR_TIME_ADD(pgBufferUsage.blk_read_time, io_time);+ }- if (track_io_timing)- INSTR_TIME_SET_CURRENT(io_start);+ pgstat_count_io_op(io_object, io_context, IOOP_READ);++ /* check for garbage data */+ if (!PageIsVerifiedExtended((Page) bufBlock, blockNum,+ PIV_LOG_WARNING | PIV_REPORT_STAT))+ {+ if (mode == RBM_ZERO_ON_ERROR || zero_damaged_pages)+ {+ ereport(WARNING,+ (errcode(ERRCODE_DATA_CORRUPTED),+ errmsg("invalid page in block %u of relation %s; zeroing out page",+ blockNum,+ relpath(smgr->smgr_rlocator, forkNum))));+ MemSet((char *) bufBlock, 0, BLCKSZ);+ }
else
- INSTR_TIME_SET_ZERO(io_start);-- smgrread(smgr, forkNum, blockNum, bufBlock);-- pgstat_count_io_op(io_object, io_context, IOOP_READ);-- if (track_io_timing)- {- INSTR_TIME_SET_CURRENT(io_time);- INSTR_TIME_SUBTRACT(io_time, io_start);- pgstat_count_buffer_read_time(INSTR_TIME_GET_MICROSEC(io_time));- INSTR_TIME_ADD(pgBufferUsage.blk_read_time, io_time);- }-- /* check for garbage data */- if (!PageIsVerifiedExtended((Page) bufBlock, blockNum,- PIV_LOG_WARNING | PIV_REPORT_STAT))- {- if (mode == RBM_ZERO_ON_ERROR || zero_damaged_pages)- {- ereport(WARNING,- (errcode(ERRCODE_DATA_CORRUPTED),- errmsg("invalid page in block %u of relation %s; zeroing out page",- blockNum,- relpath(smgr->smgr_rlocator, forkNum))));- MemSet((char *) bufBlock, 0, BLCKSZ);- }- else- ereport(ERROR,- (errcode(ERRCODE_DATA_CORRUPTED),- errmsg("invalid page in block %u of relation %s",- blockNum,- relpath(smgr->smgr_rlocator, forkNum))));- }+ ereport(ERROR,+ (errcode(ERRCODE_DATA_CORRUPTED),+ errmsg("invalid page in block %u of relation %s",+ blockNum,+ relpath(smgr->smgr_rlocator, forkNum))));
}
}
@@ -1090,7 +1193,6 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
smgr->smgr_rlocator.locator.dbOid,
smgr->smgr_rlocator.locator.relNumber,
smgr->smgr_rlocator.backend,
- isExtend,
found);
return BufferDescriptorGetBuffer(bufHdr);
@@ -1640,6 +1742,354 @@ again:
return buf;
}
+/*+ * Limit the number of pins a batch operation may additionally acquire, to+ * avoid running out of pinnable buffers.+ *+ * One additional pin is always allowed, as otherwise the operation likely+ * cannot be performed at all.+ *+ * The number of allowed pins for a backend is computed based on+ * shared_buffers and the maximum number of connections possible. That's very+ * pessimistic, but oustide of toy-sized shared_buffers it should allow+ * sufficient pins.+ */+static void+LimitAdditionalPins(uint32 *additional_pins)+{+ uint32 max_backends;+ int max_proportional_pins;++ if (*additional_pins <= 1)+ return;++ max_backends = MaxBackends + NUM_AUXILIARY_PROCS;+ max_proportional_pins = NBuffers / max_backends;++ /*+ * Subtract the approximate number of buffers already pinned by this+ * backend. We get the number of "overflowed" pins for free, but don't+ * know the number of pins in PrivateRefCountArray. The cost of+ * calculating that exactly doesn't seem worth it, so just assume the max.+ */+ max_proportional_pins -= PrivateRefCountOverflowed + REFCOUNT_ARRAY_ENTRIES;++ if (max_proportional_pins < 0)+ max_proportional_pins = 1;++ if (*additional_pins > max_proportional_pins)+ *additional_pins = max_proportional_pins;+}++/*+ * Logic shared between ExtendBufferedRelBy(), ExtendBufferedRelTo(). Just to+ * avoid duplicating the tracing and relpersistence related logic.+ */+static BlockNumber+ExtendBufferedRelCommon(ExtendBufferedWhat eb,+ ForkNumber fork,+ BufferAccessStrategy strategy,+ uint32 flags,+ uint32 extend_by,+ BlockNumber extend_upto,+ Buffer *buffers,+ uint32 *extended_by)+{+ BlockNumber first_block;++ TRACE_POSTGRESQL_BUFFER_EXTEND_START(fork,+ eb.smgr->smgr_rlocator.locator.spcOid,+ eb.smgr->smgr_rlocator.locator.dbOid,+ eb.smgr->smgr_rlocator.locator.relNumber,+ eb.smgr->smgr_rlocator.backend,+ extend_by);++ if (eb.relpersistence == RELPERSISTENCE_TEMP)+ first_block = ExtendBufferedRelLocal(eb, fork, flags,+ extend_by, extend_upto,+ buffers, &extend_by);+ else+ first_block = ExtendBufferedRelShared(eb, fork, strategy, flags,+ extend_by, extend_upto,+ buffers, &extend_by);+ *extended_by = extend_by;++ TRACE_POSTGRESQL_BUFFER_EXTEND_DONE(fork,+ eb.smgr->smgr_rlocator.locator.spcOid,+ eb.smgr->smgr_rlocator.locator.dbOid,+ eb.smgr->smgr_rlocator.locator.relNumber,+ eb.smgr->smgr_rlocator.backend,+ *extended_by,+ first_block);++ return first_block;+}++/*+ * Implementation of ExtendBufferedRelBy() and ExtendBufferedRelTo() for+ * shared buffers.+ */+static BlockNumber+ExtendBufferedRelShared(ExtendBufferedWhat eb,+ ForkNumber fork,+ BufferAccessStrategy strategy,+ uint32 flags,+ uint32 extend_by,+ BlockNumber extend_upto,+ Buffer *buffers,+ uint32 *extended_by)+{+ BlockNumber first_block;+ IOContext io_context = IOContextForStrategy(strategy);++ LimitAdditionalPins(&extend_by);++ /*+ * Acquire victim buffers for extension without holding extension lock.+ * Writing out victim buffers is the most expensive part of extending the+ * relation, particularly when doing so requires WAL flushes. Zeroing out+ * the buffers is also quite expensive, so do that before holding the+ * extension lock as well.+ *+ * These pages are pinned by us and not valid. While we hold the pin they+ * can't be acquired as victim buffers by another backend.+ */+ for (uint32 i = 0; i < extend_by; i++)+ {+ Block buf_block;++ buffers[i] = GetVictimBuffer(strategy, io_context);+ buf_block = BufHdrGetBlock(GetBufferDescriptor(buffers[i] - 1));++ /* new buffers are zero-filled */+ MemSet((char *) buf_block, 0, BLCKSZ);+ }++ /*+ * Lock relation against concurrent extensions, unless requested not to.+ *+ * We use the same extension lock for all forks. That's unnecessarily+ * restrictive, but currently extensions for forks don't happen often+ * enough to make it worth locking more granularly.+ *+ * Note that another backend might have extended the relation by the time+ * we get the lock.+ */+ if (!(flags & EB_SKIP_EXTENSION_LOCK))+ {+ LockRelationForExtension(eb.rel, ExclusiveLock);+ eb.smgr = RelationGetSmgr(eb.rel);+ }++ /*+ * If requested, invalidate size cache, so that smgrnblocks asks the+ * kernel.+ */+ if (flags & EB_CLEAR_SIZE_CACHE)+ eb.smgr->smgr_cached_nblocks[fork] = InvalidBlockNumber;++ first_block = smgrnblocks(eb.smgr, fork);++ if (extend_upto != InvalidBlockNumber)+ {+ uint32 old_num_pages = extend_by;++ if (first_block > extend_upto)+ extend_by = 0;+ else if ((uint64) first_block + extend_by > extend_upto)+ extend_by = extend_upto - first_block;++ for (uint32 i = extend_by; i < old_num_pages; i++)+ {+ BufferDesc *buf_hdr = GetBufferDescriptor(buffers[i] - 1);++ /*+ * The victim buffer we acquired peviously is clean and unused,+ * let it be found again quickly+ */+ StrategyFreeBuffer(buf_hdr);+ UnpinBuffer(buf_hdr);+ }++ if (extend_by == 0)+ {+ UnlockRelationForExtension(eb.rel, ExclusiveLock);+ *extended_by = extend_by;+ return first_block;+ }+ }++ /* Fail if relation is already at maximum possible length */+ if ((uint64) first_block + extend_by >= MaxBlockNumber)+ ereport(ERROR,+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),+ errmsg("cannot extend relation %s beyond %u blocks",+ relpath(eb.smgr->smgr_rlocator, fork),+ MaxBlockNumber)));++ /*+ * Insert buffers into buffer table, mark as IO_IN_PROGRESS.+ *+ * This needs to happen before we extend the relation, because as soon as+ * we do, other backends can start to read in those pages.+ */+ for (int i = 0; i < extend_by; i++)+ {+ Buffer victim_buf = buffers[i];+ BufferDesc *victim_buf_hdr = GetBufferDescriptor(victim_buf - 1);+ BufferTag tag;+ uint32 hash;+ LWLock *partition_lock;+ int existing_id;++ InitBufferTag(&tag, &eb.smgr->smgr_rlocator.locator, fork, first_block + i);+ hash = BufTableHashCode(&tag);+ partition_lock = BufMappingPartitionLock(hash);++ LWLockAcquire(partition_lock, LW_EXCLUSIVE);++ existing_id = BufTableInsert(&tag, hash, victim_buf_hdr->buf_id);++ /*+ * We get here only in the corner case where we are trying to extend+ * the relation but we found a pre-existing buffer. This can happen+ * because a prior attempt at extending the relation failed, and+ * because mdread doesn't complain about reads beyond EOF (when+ * zero_damaged_pages is ON) and so a previous attempt to read a block+ * beyond EOF could have left a "valid" zero-filled buffer.+ * Unfortunately, we have also seen this case occurring because of+ * buggy Linux kernels that sometimes return an lseek(SEEK_END) result+ * that doesn't account for a recent write. In that situation, the+ * pre-existing buffer would contain valid data that we don't want to+ * overwrite. Since the legitimate cases should always have left a+ * zero-filled buffer, complain if not PageIsNew.+ */+ if (existing_id >= 0)+ {+ BufferDesc *existing_hdr = GetBufferDescriptor(existing_id);+ Block buf_block;+ bool valid;++ /*+ * Pin the existing buffer before releasing the partition lock,+ * preventing it from being evicted.+ */+ valid = PinBuffer(existing_hdr, strategy);++ LWLockRelease(partition_lock);++ /*+ * The victim buffer we acquired peviously is clean and unused,+ * let it be found again quickly+ */+ StrategyFreeBuffer(victim_buf_hdr);+ UnpinBuffer(victim_buf_hdr);++ buffers[i] = BufferDescriptorGetBuffer(existing_hdr);+ buf_block = BufHdrGetBlock(existing_hdr);++ if (valid && !PageIsNew((Page) buf_block))+ ereport(ERROR,+ (errmsg("unexpected data beyond EOF in block %u of relation %s",+ existing_hdr->tag.blockNum, relpath(eb.smgr->smgr_rlocator, fork)),+ errhint("This has been seen to occur with buggy kernels; consider updating your system.")));++ /*+ * We *must* do smgr[zero]extend before succeeding, else the page+ * will not be reserved by the kernel, and the next P_NEW call+ * will decide to return the same page. Clear the BM_VALID bit,+ * do StartBufferIO() and proceed.+ *+ * Loop to handle the very small possibility that someone re-sets+ * BM_VALID between our clearing it and StartBufferIO inspecting+ * it.+ */+ do+ {+ uint32 buf_state = LockBufHdr(existing_hdr);++ buf_state &= ~BM_VALID;+ UnlockBufHdr(existing_hdr, buf_state);+ } while (!StartBufferIO(existing_hdr, true));+ }+ else+ {+ uint32 buf_state;++ buf_state = LockBufHdr(victim_buf_hdr);++ /* some sanity checks while we hold the buffer header lock */+ Assert(!(buf_state & (BM_VALID | BM_TAG_VALID | BM_DIRTY | BM_JUST_DIRTIED)));+ Assert(BUF_STATE_GET_REFCOUNT(buf_state) == 1);++ victim_buf_hdr->tag = tag;++ buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;+ if (eb.relpersistence == RELPERSISTENCE_PERMANENT || fork == INIT_FORKNUM)+ buf_state |= BM_PERMANENT;++ UnlockBufHdr(victim_buf_hdr, buf_state);++ LWLockRelease(partition_lock);++ /* XXX: could combine the locked operations in it with the above */+ StartBufferIO(victim_buf_hdr, true);+ }+ }++ /*+ * Note: if smgzerorextend fails, we will end up with buffers that are+ * allocated but not marked BM_VALID. The next relation extension will+ * still select the same block number (because the relation didn't get any+ * longer on disk) and so future attempts to extend the relation will find+ * the same buffers (if they have not been recycled) but come right back+ * here to try smgrzeroextend again.+ *+ * We don't need to set checksum for all-zero pages.+ */+ smgrzeroextend(eb.smgr, fork, first_block, extend_by, false);++ /*+ * Release the file-extension lock; it's now OK for someone else to extend+ * the relation some more.+ *+ * We remove IO_IN_PROGRESS after this, as waking up waiting backends can+ * take noticeable time.+ */+ if (!(flags & EB_SKIP_EXTENSION_LOCK))+ UnlockRelationForExtension(eb.rel, ExclusiveLock);++ /* Set BM_VALID, terminate IO, and wake up any waiters */+ for (int i = 0; i < extend_by; i++)+ {+ Buffer buf = buffers[i];+ BufferDesc *buf_hdr = GetBufferDescriptor(buf - 1);+ bool lock = false;++ if (flags & EB_LOCK_FIRST && i == 0)+ lock = true;+ else if (flags & EB_LOCK_TARGET)+ {+ Assert(extend_upto != InvalidBlockNumber);+ if (first_block + i + 1 == extend_upto)+ lock = true;+ }++ if (lock)+ LWLockAcquire(BufferDescriptorGetContentLock(buf_hdr), LW_EXCLUSIVE);++ TerminateBufferIO(buf_hdr, false, BM_VALID);+ }++ pgBufferUsage.shared_blks_written += extend_by;+ pgstat_count_io_op_n(IOOBJECT_RELATION, io_context, IOOP_EXTEND,+ extend_by);++ *extended_by = extend_by;++ return first_block;+}+
/*
* MarkBufferDirty
*
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.cindex 5b44b0be8b5..0528fddf992 100644--- a/src/backend/storage/buffer/localbuf.c+++ b/src/backend/storage/buffer/localbuf.c@@ -49,6 +49,9 @@ static int nextFreeLocalBufId = 0;
static HTAB *LocalBufHash = NULL;
+/* number of local buffers pinned at least once */+static int NLocalPinnedBuffers = 0;+
static void InitLocalBuffers(void);
static Block GetLocalBufferStorage(void);
@@ -280,6 +283,154 @@ GetLocalVictimBuffer(void)
return BufferDescriptorGetBuffer(bufHdr);
}
+/* see LimitAdditionalPins() */+static void+LimitAdditionalLocalPins(uint32 *additional_pins)+{+ uint32 max_pins;++ if (*additional_pins <= 1)+ return;++ /*+ * In contrast to LimitAdditionalPins() other backends don't play a role+ * here. We can allow up to NLocBuffer pins in total.+ */+ max_pins = (NLocBuffer - NLocalPinnedBuffers);++ if (*additional_pins >= max_pins)+ *additional_pins = max_pins;+}++/*+ * Implementation of ExtendBufferedRelBy() and ExtendBufferedRelTo() for+ * temporary buffers.+ */+BlockNumber+ExtendBufferedRelLocal(ExtendBufferedWhat eb,+ ForkNumber fork,+ uint32 flags,+ uint32 extend_by,+ BlockNumber extend_upto,+ Buffer *buffers,+ uint32 *extended_by)+{+ BlockNumber first_block;++ /* Initialize local buffers if first request in this session */+ if (LocalBufHash == NULL)+ InitLocalBuffers();++ LimitAdditionalLocalPins(&extend_by);++ for (uint32 i = 0; i < extend_by; i++)+ {+ BufferDesc *buf_hdr;+ Block buf_block;++ buffers[i] = GetLocalVictimBuffer();+ buf_hdr = GetLocalBufferDescriptor(-(buffers[i] + 1));+ buf_block = LocalBufHdrGetBlock(buf_hdr);++ /* new buffers are zero-filled */+ MemSet((char *) buf_block, 0, BLCKSZ);+ }++ first_block = smgrnblocks(eb.smgr, fork);++ if (extend_upto != InvalidBlockNumber)+ {+ /*+ * In contranst to shared relations, nothing could change the relation+ * size concurrently. Thus we shouldn't end up finding that we don't+ * need to do anything.+ */+ Assert(first_block <= extend_upto);++ Assert((uint64) first_block + extend_by <= extend_upto);+ }++ /* Fail if relation is already at maximum possible length */+ if ((uint64) first_block + extend_by >= MaxBlockNumber)+ ereport(ERROR,+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),+ errmsg("cannot extend relation %s beyond %u blocks",+ relpath(eb.smgr->smgr_rlocator, fork),+ MaxBlockNumber)));++ for (int i = 0; i < extend_by; i++)+ {+ int victim_buf_id;+ BufferDesc *victim_buf_hdr;+ BufferTag tag;+ LocalBufferLookupEnt *hresult;+ bool found;++ victim_buf_id = -(buffers[i] + 1);+ victim_buf_hdr = GetLocalBufferDescriptor(victim_buf_id);++ InitBufferTag(&tag, &eb.smgr->smgr_rlocator.locator, fork, first_block + i);++ hresult = (LocalBufferLookupEnt *)+ hash_search(LocalBufHash, (void *) &tag, HASH_ENTER, &found);+ if (found)+ {+ BufferDesc *existing_hdr = GetLocalBufferDescriptor(hresult->id);+ uint32 buf_state;++ UnpinLocalBuffer(BufferDescriptorGetBuffer(victim_buf_hdr));++ existing_hdr = GetLocalBufferDescriptor(hresult->id);+ PinLocalBuffer(existing_hdr, false);+ buffers[i] = BufferDescriptorGetBuffer(existing_hdr);++ buf_state = pg_atomic_read_u32(&existing_hdr->state);+ Assert(buf_state & BM_TAG_VALID);+ Assert(!(buf_state & BM_DIRTY));+ buf_state &= BM_VALID;+ pg_atomic_unlocked_write_u32(&existing_hdr->state, buf_state);+ }+ else+ {+ uint32 buf_state = pg_atomic_read_u32(&victim_buf_hdr->state);++ Assert(!(buf_state & (BM_VALID | BM_TAG_VALID | BM_DIRTY | BM_JUST_DIRTIED)));++ victim_buf_hdr->tag = tag;++ buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;++ pg_atomic_unlocked_write_u32(&victim_buf_hdr->state, buf_state);++ hresult->id = victim_buf_id;+ }+ }++ /* actually extend relation */+ smgrzeroextend(eb.smgr, fork, first_block, extend_by, false);++ for (int i = 0; i < extend_by; i++)+ {+ Buffer buf = buffers[i];+ BufferDesc *buf_hdr;+ uint32 buf_state;++ buf_hdr = GetLocalBufferDescriptor(-(buf + 1));++ buf_state = pg_atomic_read_u32(&buf_hdr->state);+ buf_state |= BM_VALID;+ pg_atomic_unlocked_write_u32(&buf_hdr->state, buf_state);+ }++ *extended_by = extend_by;++ pgBufferUsage.temp_blks_written += extend_by;+ pgstat_count_io_op_n(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_EXTEND,+ extend_by);++ return first_block;+}+
/*
* MarkLocalBufferDirty -
* mark a local buffer dirty
@@ -494,6 +645,7 @@ PinLocalBuffer(BufferDesc *buf_hdr, bool adjust_usagecount)
if (LocalRefCount[bufid] == 0)
{
+ NLocalPinnedBuffers++;
if (adjust_usagecount &&
BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)
{
@@ -515,9 +667,11 @@ UnpinLocalBuffer(Buffer buffer)
Assert(BufferIsLocal(buffer));
Assert(LocalRefCount[buffid] > 0);
+ Assert(NLocalPinnedBuffers > 0);
ResourceOwnerForgetBuffer(CurrentResourceOwner, buffer);
- LocalRefCount[buffid]--;+ if (--LocalRefCount[buffid] == 0)+ NLocalPinnedBuffers--;
}
/*
diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.cindex af5d5546101..f2f6eae8031 100644--- a/src/backend/utils/activity/pgstat_io.c+++ b/src/backend/utils/activity/pgstat_io.c@@ -64,13 +64,19 @@ pgstat_bktype_io_stats_valid(PgStat_BktypeIO *backend_io,
void
pgstat_count_io_op(IOObject io_object, IOContext io_context, IOOp io_op)
+{+ pgstat_count_io_op_n(io_object, io_context, io_op, 1);+}++void+pgstat_count_io_op_n(IOObject io_object, IOContext io_context, IOOp io_op, uint32 cnt)
{
Assert((unsigned int) io_object < IOOBJECT_NUM_TYPES);
Assert((unsigned int) io_context < IOCONTEXT_NUM_TYPES);
Assert((unsigned int) io_op < IOOP_NUM_TYPES);
Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op));
- PendingIOStats.data[io_object][io_context][io_op]++;+ PendingIOStats.data[io_object][io_context][io_op] += cnt;
have_iostats = true;
}
diff --git a/src/backend/utils/probes.d b/src/backend/utils/probes.dindex c064d679e94..fd3df2f7900 100644--- a/src/backend/utils/probes.d+++ b/src/backend/utils/probes.d@@ -55,10 +55,12 @@ provider postgresql {
probe sort__start(int, bool, int, int, bool, int);
probe sort__done(bool, long);
- probe buffer__read__start(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool);- probe buffer__read__done(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool, bool);+ probe buffer__read__start(ForkNumber, BlockNumber, Oid, Oid, Oid, int);+ probe buffer__read__done(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool);
probe buffer__flush__start(ForkNumber, BlockNumber, Oid, Oid, Oid);
probe buffer__flush__done(ForkNumber, BlockNumber, Oid, Oid, Oid);
+ probe buffer__extend__start(ForkNumber, Oid, Oid, Oid, int, unsigned int);+ probe buffer__extend__done(ForkNumber, Oid, Oid, Oid, int, unsigned int, BlockNumber);
probe buffer__checkpoint__start(int);
probe buffer__checkpoint__sync__start();
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgmlindex b2ccd8d7fef..00df5fcd9ba 100644--- a/doc/src/sgml/monitoring.sgml+++ b/doc/src/sgml/monitoring.sgml@@ -7761,33 +7761,52 @@ FROM pg_stat_get_backend_idset() AS backendid;
<entry>Probe that fires when the two-phase portion of a checkpoint is
complete.</entry>
</row>
+ <row>+ <entry><literal>buffer-extend-start</literal></entry>+ <entry><literal>(ForkNumber, BlockNumber, Oid, Oid, Oid, int, unsigned int)</literal></entry>+ <entry>Probe that fires when a relation extension starts.+ arg0 contains the fork to be extended. arg1, arg2, and arg3 contain the+ tablespace, database, and relation OIDs identifying the relation. arg4+ is the ID of the backend which created the temporary relation for a+ local buffer, or <symbol>InvalidBackendId</symbol> (-1) for a shared+ buffer. arg5 is the number of blocks the caller would like to extend+ by.</entry>+ </row>+ <row>+ <entry><literal>buffer-extend-done</literal></entry>+ <entry><literal>(ForkNumber, BlockNumber, Oid, Oid, Oid, int, unsigned int, BlockNumber)</literal></entry>+ <entry>Probe that fires when a relation extension is complete.+ arg0 contains the fork to be extended. arg1, arg2, and arg3 contain the+ tablespace, database, and relation OIDs identifying the relation. arg4+ is the ID of the backend which created the temporary relation for a+ local buffer, or <symbol>InvalidBackendId</symbol> (-1) for a shared+ buffer. arg5 is the number of blocks the relation was extended by, this+ can be less than the number in the+ <literal>buffer-extend-start</literal> due to resource+ constraints. arg6 contains the BlockNumber of the first new+ block.</entry>+ </row>
<row>
<entry><literal>buffer-read-start</literal></entry>
- <entry><literal>(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool)</literal></entry>+ <entry><literal>(ForkNumber, BlockNumber, Oid, Oid, Oid, int)</literal></entry>
<entry>Probe that fires when a buffer read is started.
- arg0 and arg1 contain the fork and block numbers of the page (but- arg1 will be -1 if this is a relation extension request).+ arg0 and arg1 contain the fork and block numbers of the page.
arg2, arg3, and arg4 contain the tablespace, database, and relation OIDs
identifying the relation.
arg5 is the ID of the backend which created the temporary relation for a
local buffer, or <symbol>InvalidBackendId</symbol> (-1) for a shared buffer.
- arg6 is true for a relation extension request, false for normal- read.</entry>+ </entry>
</row>
<row>
<entry><literal>buffer-read-done</literal></entry>
- <entry><literal>(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool, bool)</literal></entry>+ <entry><literal>(ForkNumber, BlockNumber, Oid, Oid, Oid, int, bool)</literal></entry>
<entry>Probe that fires when a buffer read is complete.
- arg0 and arg1 contain the fork and block numbers of the page (if this- is a relation extension request, arg1 now contains the block number- of the newly added block).+ arg0 and arg1 contain the fork and block numbers of the page.
arg2, arg3, and arg4 contain the tablespace, database, and relation OIDs
identifying the relation.
arg5 is the ID of the backend which created the temporary relation for a
local buffer, or <symbol>InvalidBackendId</symbol> (-1) for a shared buffer.
- arg6 is true for a relation extension request, false for normal- read.- arg7 is true if the buffer was found in the pool, false if not.</entry>+ arg6 is true if the buffer was found in the pool, false if not.</entry>
</row>
<row>
<entry><literal>buffer-flush-start</literal></entry>
--
2.38.0
[text/x-diff] v5-0009-Convert-a-few-places-to-ExtendBufferedRel.patch (12.9K, ../20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de/10-v5-0009-Convert-a-few-places-to-ExtendBufferedRel.patch)
download | inline diff:
From 9f92ad2d5e09fab6d2348ba24fcf33c6d1fc7572 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 24 Oct 2022 12:18:18 -0700
Subject: [PATCH v5 09/14] Convert a few places to ExtendBufferedRel
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/access/brin/brin.c | 9 +++----
src/backend/access/brin/brin_pageops.c | 4 +++
src/backend/access/brin/brin_revmap.c | 15 +++---------
src/backend/access/gin/gininsert.c | 10 +++-----
src/backend/access/gin/ginutil.c | 13 ++--------
src/backend/access/gin/ginvacuum.c | 8 ++++++
src/backend/access/gist/gist.c | 4 +--
src/backend/access/gist/gistutil.c | 14 ++---------
src/backend/access/gist/gistvacuum.c | 3 +++
src/backend/access/nbtree/nbtpage.c | 34 +++++++-------------------
src/backend/access/nbtree/nbtree.c | 3 +++
src/backend/access/spgist/spgutils.c | 13 ++--------
contrib/bloom/blutils.c | 12 ++-------
13 files changed, 48 insertions(+), 94 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.cindex 53e4721a54e..41bf950a4af 100644--- a/src/backend/access/brin/brin.c+++ b/src/backend/access/brin/brin.c@@ -837,9 +837,9 @@ brinbuild(Relation heap, Relation index, IndexInfo *indexInfo)
* whole relation will be rolled back.
*/
- meta = ReadBuffer(index, P_NEW);+ meta = ExtendBufferedRel(EB_REL(index), MAIN_FORKNUM, NULL,+ EB_LOCK_FIRST | EB_SKIP_EXTENSION_LOCK);
Assert(BufferGetBlockNumber(meta) == BRIN_METAPAGE_BLKNO);
- LockBuffer(meta, BUFFER_LOCK_EXCLUSIVE);
brin_metapage_init(BufferGetPage(meta), BrinGetPagesPerRange(index),
BRIN_CURRENT_VERSION);
@@ -904,9 +904,8 @@ brinbuildempty(Relation index)
Buffer metabuf;
/* An empty BRIN index has a metapage only. */
- metabuf =- ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);- LockBuffer(metabuf, BUFFER_LOCK_EXCLUSIVE);+ metabuf = ExtendBufferedRel(EB_REL(index), INIT_FORKNUM, NULL,+ EB_LOCK_FIRST | EB_SKIP_EXTENSION_LOCK);
/* Initialize and xlog metabuffer. */
START_CRIT_SECTION();
diff --git a/src/backend/access/brin/brin_pageops.c b/src/backend/access/brin/brin_pageops.cindex ad5a89bd051..b578d259545 100644--- a/src/backend/access/brin/brin_pageops.c+++ b/src/backend/access/brin/brin_pageops.c@@ -730,6 +730,10 @@ brin_getinsertbuffer(Relation irel, Buffer oldbuf, Size itemsz,
* There's not enough free space in any existing index page,
* according to the FSM: extend the relation to obtain a shiny new
* page.
+ *+ * XXX: It's likely possible to use RBM_ZERO_AND_LOCK here,+ * which'd avoid the need to hold the extension lock during buffer+ * reclaim.
*/
if (!RELATION_IS_LOCAL(irel))
{
diff --git a/src/backend/access/brin/brin_revmap.c b/src/backend/access/brin/brin_revmap.cindex 7fc5226bf74..f4271ba31c9 100644--- a/src/backend/access/brin/brin_revmap.c+++ b/src/backend/access/brin/brin_revmap.c@@ -538,7 +538,6 @@ revmap_physical_extend(BrinRevmap *revmap)
BlockNumber mapBlk;
BlockNumber nblocks;
Relation irel = revmap->rm_irel;
- bool needLock = !RELATION_IS_LOCAL(irel);
/*
* Lock the metapage. This locks out concurrent extensions of the revmap,
@@ -570,10 +569,8 @@ revmap_physical_extend(BrinRevmap *revmap)
}
else
{
- if (needLock)- LockRelationForExtension(irel, ExclusiveLock);-- buf = ReadBuffer(irel, P_NEW);+ buf = ExtendBufferedRel(EB_REL(irel), MAIN_FORKNUM, NULL,+ EB_LOCK_FIRST);
if (BufferGetBlockNumber(buf) != mapBlk)
{
/*
@@ -582,17 +579,11 @@ revmap_physical_extend(BrinRevmap *revmap)
* up and have caller start over. We will have to evacuate that
* page from under whoever is using it.
*/
- if (needLock)- UnlockRelationForExtension(irel, ExclusiveLock);
LockBuffer(revmap->rm_metaBuf, BUFFER_LOCK_UNLOCK);
- ReleaseBuffer(buf);+ UnlockReleaseBuffer(buf);
return;
}
- LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
page = BufferGetPage(buf);
-- if (needLock)- UnlockRelationForExtension(irel, ExclusiveLock);
}
/* Check that it's a regular block (or an empty page) */
diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.cindex d5d748009ea..be1841de7bf 100644--- a/src/backend/access/gin/gininsert.c+++ b/src/backend/access/gin/gininsert.c@@ -440,12 +440,10 @@ ginbuildempty(Relation index)
MetaBuffer;
/* An empty GIN index has two pages. */
- MetaBuffer =- ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);- LockBuffer(MetaBuffer, BUFFER_LOCK_EXCLUSIVE);- RootBuffer =- ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);- LockBuffer(RootBuffer, BUFFER_LOCK_EXCLUSIVE);+ MetaBuffer = ExtendBufferedRel(EB_REL(index), INIT_FORKNUM, NULL,+ EB_LOCK_FIRST | EB_SKIP_EXTENSION_LOCK);+ RootBuffer = ExtendBufferedRel(EB_REL(index), INIT_FORKNUM, NULL,+ EB_LOCK_FIRST | EB_SKIP_EXTENSION_LOCK);
/* Initialize and xlog metabuffer and root buffer. */
START_CRIT_SECTION();
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.cindex 03fec1704e9..3c6f87236c5 100644--- a/src/backend/access/gin/ginutil.c+++ b/src/backend/access/gin/ginutil.c@@ -299,7 +299,6 @@ Buffer
GinNewBuffer(Relation index)
{
Buffer buffer;
- bool needLock;
/* First, try to get a page from FSM */
for (;;)
@@ -327,16 +326,8 @@ GinNewBuffer(Relation index)
ReleaseBuffer(buffer);
}
- /* Must extend the file */- needLock = !RELATION_IS_LOCAL(index);- if (needLock)- LockRelationForExtension(index, ExclusiveLock);-- buffer = ReadBuffer(index, P_NEW);- LockBuffer(buffer, GIN_EXCLUSIVE);-- if (needLock)- UnlockRelationForExtension(index, ExclusiveLock);+ buffer = ExtendBufferedRel(EB_REL(index), MAIN_FORKNUM, NULL,+ EB_LOCK_FIRST);
return buffer;
}
diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.cindex e5d310d8362..13251d7e07d 100644--- a/src/backend/access/gin/ginvacuum.c+++ b/src/backend/access/gin/ginvacuum.c@@ -736,6 +736,10 @@ ginvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
*/
needLock = !RELATION_IS_LOCAL(index);
+ /*+ * FIXME: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't+ * think this is still required?+ */
if (needLock)
LockRelationForExtension(index, ExclusiveLock);
npages = RelationGetNumberOfBlocks(index);
@@ -786,6 +790,10 @@ ginvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
stats->pages_free = totFreePages;
+ /*+ * FIXME: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't+ * think this is still required?+ */
if (needLock)
LockRelationForExtension(index, ExclusiveLock);
stats->num_pages = RelationGetNumberOfBlocks(index);
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.cindex ea72bcce1bc..d3539e96d87 100644--- a/src/backend/access/gist/gist.c+++ b/src/backend/access/gist/gist.c@@ -134,8 +134,8 @@ gistbuildempty(Relation index)
Buffer buffer;
/* Initialize the root page */
- buffer = ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);- LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);+ buffer = ExtendBufferedRel(EB_REL(index), INIT_FORKNUM, NULL,+ EB_SKIP_EXTENSION_LOCK | EB_LOCK_FIRST);
/* Initialize and xlog buffer */
START_CRIT_SECTION();
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.cindex b4d843a0ff1..446fcb3cc5e 100644--- a/src/backend/access/gist/gistutil.c+++ b/src/backend/access/gist/gistutil.c@@ -824,7 +824,6 @@ Buffer
gistNewBuffer(Relation r)
{
Buffer buffer;
- bool needLock;
/* First, try to get a page from FSM */
for (;;)
@@ -877,17 +876,8 @@ gistNewBuffer(Relation r)
ReleaseBuffer(buffer);
}
- /* Must extend the file */- needLock = !RELATION_IS_LOCAL(r);-- if (needLock)- LockRelationForExtension(r, ExclusiveLock);-- buffer = ReadBuffer(r, P_NEW);- LockBuffer(buffer, GIST_EXCLUSIVE);-- if (needLock)- UnlockRelationForExtension(r, ExclusiveLock);+ buffer = ExtendBufferedRel(EB_REL(r), MAIN_FORKNUM, NULL,+ EB_LOCK_FIRST);
return buffer;
}
diff --git a/src/backend/access/gist/gistvacuum.c b/src/backend/access/gist/gistvacuum.cindex 3f60d3274d2..cc711b04986 100644--- a/src/backend/access/gist/gistvacuum.c+++ b/src/backend/access/gist/gistvacuum.c@@ -203,6 +203,9 @@ gistvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
* we must already have processed any tuples due to be moved into such a
* page.
*
+ * FIXME: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't+ * think this issue still exists?+ *
* We can skip locking for new or temp relations, however, since no one
* else could be accessing them.
*/
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.cindex 3feee28d197..c4f16b3d2b7 100644--- a/src/backend/access/nbtree/nbtpage.c+++ b/src/backend/access/nbtree/nbtpage.c@@ -881,7 +881,6 @@ _bt_getbuf(Relation rel, BlockNumber blkno, int access)
}
else
{
- bool needLock;
Page page;
Assert(access == BT_WRITE);
@@ -962,31 +961,16 @@ _bt_getbuf(Relation rel, BlockNumber blkno, int access)
}
/*
- * Extend the relation by one page.- *- * We have to use a lock to ensure no one else is extending the rel at- * the same time, else we will both try to initialize the same new- * page. We can skip locking for new or temp relations, however,- * since no one else could be accessing them.+ * Extend the relation by one page. Need to use RBM_ZERO_AND_LOCK or+ * we risk a race condition against btvacuumscan --- see comments+ * therein. This forces us to repeat the valgrind request that+ * _bt_lockbuf() otherwise would make, as we can't use _bt_lockbuf()+ * without introducing a race.
*/
- needLock = !RELATION_IS_LOCAL(rel);-- if (needLock)- LockRelationForExtension(rel, ExclusiveLock);-- buf = ReadBuffer(rel, P_NEW);-- /* Acquire buffer lock on new page */- _bt_lockbuf(rel, buf, BT_WRITE);-- /*- * Release the file-extension lock; it's now OK for someone else to- * extend the relation some more. Note that we cannot release this- * lock before we have buffer lock on the new page, or we risk a race- * condition against btvacuumscan --- see comments therein.- */- if (needLock)- UnlockRelationForExtension(rel, ExclusiveLock);+ buf = ExtendBufferedRel(EB_REL(rel), MAIN_FORKNUM, NULL,+ EB_LOCK_FIRST);+ if (!RelationUsesLocalBuffers(rel))+ VALGRIND_MAKE_MEM_DEFINED(BufferGetPage(buf), BLCKSZ);
/* Initialize the new page before returning it */
page = BufferGetPage(buf);
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.cindex a68dd07534f..86010443269 100644--- a/src/backend/access/nbtree/nbtree.c+++ b/src/backend/access/nbtree/nbtree.c@@ -970,6 +970,9 @@ btvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
* write-lock on the left page before it adds a right page, so we must
* already have processed any tuples due to be moved into such a page.
*
+ * FIXME: Now that new pages are locked with RBM_ZERO_AND_LOCK, I don't+ * think this issue still exists?+ *
* We can skip locking for new or temp relations, however, since no one
* else could be accessing them.
*/
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.cindex 4e7ff1d1603..190e4f76a9e 100644--- a/src/backend/access/spgist/spgutils.c+++ b/src/backend/access/spgist/spgutils.c@@ -366,7 +366,6 @@ Buffer
SpGistNewBuffer(Relation index)
{
Buffer buffer;
- bool needLock;
/* First, try to get a page from FSM */
for (;;)
@@ -406,16 +405,8 @@ SpGistNewBuffer(Relation index)
ReleaseBuffer(buffer);
}
- /* Must extend the file */- needLock = !RELATION_IS_LOCAL(index);- if (needLock)- LockRelationForExtension(index, ExclusiveLock);-- buffer = ReadBuffer(index, P_NEW);- LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);-- if (needLock)- UnlockRelationForExtension(index, ExclusiveLock);+ buffer = ExtendBufferedRel(EB_REL(index), MAIN_FORKNUM, NULL,+ EB_LOCK_FIRST);
return buffer;
}
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.cindex a6d9f09f315..d935ed8fbdf 100644--- a/contrib/bloom/blutils.c+++ b/contrib/bloom/blutils.c@@ -353,7 +353,6 @@ Buffer
BloomNewBuffer(Relation index)
{
Buffer buffer;
- bool needLock;
/* First, try to get a page from FSM */
for (;;)
@@ -387,15 +386,8 @@ BloomNewBuffer(Relation index)
}
/* Must extend the file */
- needLock = !RELATION_IS_LOCAL(index);- if (needLock)- LockRelationForExtension(index, ExclusiveLock);-- buffer = ReadBuffer(index, P_NEW);- LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);-- if (needLock)- UnlockRelationForExtension(index, ExclusiveLock);+ buffer = ExtendBufferedRel(EB_REL(index), MAIN_FORKNUM, NULL,+ EB_LOCK_FIRST);
return buffer;
}
--
2.38.0
[text/x-diff] v5-0010-heapam-Add-num_pages-to-RelationGetBufferForTuple.patch (6.8K, ../20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de/11-v5-0010-heapam-Add-num_pages-to-RelationGetBufferForTuple.patch)
download | inline diff:
From 58cf78333a8fcb15f45a2b60174c8fee989b785b Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Sun, 23 Oct 2022 14:44:43 -0700
Subject: [PATCH v5 10/14] heapam: Add num_pages to RelationGetBufferForTuple()
This will be useful to compute the number of pages to extend a relation by.
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
src/include/access/hio.h | 14 +++++++-
src/backend/access/heap/heapam.c | 59 +++++++++++++++++++++++++++++---
src/backend/access/heap/hio.c | 8 ++++-
3 files changed, 75 insertions(+), 6 deletions(-)
diff --git a/src/include/access/hio.h b/src/include/access/hio.hindex 3f20b585326..7f877817597 100644--- a/src/include/access/hio.h+++ b/src/include/access/hio.h@@ -30,6 +30,17 @@ typedef struct BulkInsertStateData
{
BufferAccessStrategy strategy; /* our BULKWRITE strategy object */
Buffer current_buf; /* current insertion target page */
++ /*+ * State for bulk extensions. Further pages that were unused at the time+ * of the extension. They might be in use by the time we use them though,+ * so rechecks are needed.+ *+ * FIXME: Perhaps these should live in RelationData instead, alongside the+ * targetblock?+ */+ BlockNumber next_free;+ BlockNumber last_free;
} BulkInsertStateData;
@@ -38,6 +49,7 @@ extern void RelationPutHeapTuple(Relation relation, Buffer buffer,
extern Buffer RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer, int options,
BulkInsertStateData *bistate,
- Buffer *vmbuffer, Buffer *vmbuffer_other);+ Buffer *vmbuffer, Buffer *vmbuffer_other,+ int num_pages);
#endif /* HIO_H */
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.cindex 8abc101c8cb..e1ca6938cd0 100644--- a/src/backend/access/heap/heapam.c+++ b/src/backend/access/heap/heapam.c@@ -1774,6 +1774,8 @@ GetBulkInsertState(void)
bistate = (BulkInsertState) palloc(sizeof(BulkInsertStateData));
bistate->strategy = GetAccessStrategy(BAS_BULKWRITE);
bistate->current_buf = InvalidBuffer;
+ bistate->next_free = InvalidBlockNumber;+ bistate->last_free = InvalidBlockNumber;
return bistate;
}
@@ -1847,7 +1849,8 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
*/
buffer = RelationGetBufferForTuple(relation, heaptup->t_len,
InvalidBuffer, options, bistate,
- &vmbuffer, NULL);+ &vmbuffer, NULL,+ 0);
/*
* We're about to do the actual insert -- but check for conflict first, to
@@ -2050,6 +2053,33 @@ heap_prepare_insert(Relation relation, HeapTuple tup, TransactionId xid,
return tup;
}
+/*+ * Helper for heap_multi_insert() that computes the number of full pages s+ */+static int+heap_multi_insert_pages(HeapTuple *heaptuples, int done, int ntuples, Size saveFreeSpace)+{+ size_t page_avail;+ int npages = 0;++ page_avail = BLCKSZ - SizeOfPageHeaderData - saveFreeSpace;+ npages++;++ for (int i = done; i < ntuples; i++)+ {+ size_t tup_sz = sizeof(ItemIdData) + MAXALIGN(heaptuples[i]->t_len);++ if (page_avail < tup_sz)+ {+ npages++;+ page_avail = BLCKSZ - SizeOfPageHeaderData - saveFreeSpace;+ }+ page_avail -= tup_sz;+ }++ return npages;+}+
/*
* heap_multi_insert - insert multiple tuples into a heap
*
@@ -2076,6 +2106,9 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
Size saveFreeSpace;
bool need_tuple_data = RelationIsLogicallyLogged(relation);
bool need_cids = RelationIsAccessibleInLogicalDecoding(relation);
+ bool starting_with_empty_page = false;+ int npages = 0;+ int npages_used = 0;
/* currently not needed (thus unsupported) for heap_multi_insert() */
Assert(!(options & HEAP_INSERT_NO_LOGICAL));
@@ -2126,13 +2159,29 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
while (ndone < ntuples)
{
Buffer buffer;
- bool starting_with_empty_page;
bool all_visible_cleared = false;
bool all_frozen_set = false;
int nthispage;
CHECK_FOR_INTERRUPTS();
+ /*+ * Compute number of pages needed to insert tuples in the worst case.+ * This will be used to determine how much to extend the relation by+ * in RelationGetBufferForTuple(), if needed. If we filled a prior+ * page from scratch, we can just update our last computation, but if+ * we started with a partially filled page recompute from scratch, the+ * number of potentially required pages can vary due to tuples needing+ * to fit onto the page, page headers etc.+ */+ if (ndone == 0 || !starting_with_empty_page)+ {+ npages = heap_multi_insert_pages(heaptuples, ndone, ntuples, saveFreeSpace);+ npages_used = 0;+ }+ else+ npages_used++;+
/*
* Find buffer where at least the next tuple will fit. If the page is
* all-visible, this will also pin the requisite visibility map page.
@@ -2142,7 +2191,8 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
*/
buffer = RelationGetBufferForTuple(relation, heaptuples[ndone]->t_len,
InvalidBuffer, options, bistate,
- &vmbuffer, NULL);+ &vmbuffer, NULL,+ npages - npages_used);
page = BufferGetPage(buffer);
starting_with_empty_page = PageGetMaxOffsetNumber(page) == 0;
@@ -3576,7 +3626,8 @@ l2:
/* It doesn't fit, must use RelationGetBufferForTuple. */
newbuf = RelationGetBufferForTuple(relation, heaptup->t_len,
buffer, 0, NULL,
- &vmbuffer_new, &vmbuffer);+ &vmbuffer_new, &vmbuffer,+ 0);
/* We're all done. */
break;
}
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.cindex 7479212d4e0..65886839e70 100644--- a/src/backend/access/heap/hio.c+++ b/src/backend/access/heap/hio.c@@ -275,6 +275,11 @@ RelationAddExtraBlocks(Relation relation, BulkInsertState bistate)
* Returns pinned and exclusive-locked buffer of a page in given relation
* with free space >= given len.
*
+ * If num_pages is > 1, the relation will be extended by at least that many+ * pages when we decide to extend the relation. This is more efficient for+ * callers that know they will need multiple pages+ * (e.g. heap_multi_insert()).+ *
* If otherBuffer is not InvalidBuffer, then it references a previously
* pinned buffer of another page in the same relation; on return, this
* buffer will also be exclusive-locked. (This case is used by heap_update;
@@ -333,7 +338,8 @@ Buffer
RelationGetBufferForTuple(Relation relation, Size len,
Buffer otherBuffer, int options,
BulkInsertState bistate,
- Buffer *vmbuffer, Buffer *vmbuffer_other)+ Buffer *vmbuffer, Buffer *vmbuffer_other,+ int num_pages)
{
bool use_fsm = !(options & HEAP_INSERT_SKIP_FSM);
Buffer buffer = InvalidBuffer;
--
2.38.0
[text/x-diff] v5-0011-hio-Use-ExtendBufferedRelBy.patch (13.0K, ../20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de/12-v5-0011-hio-Use-ExtendBufferedRelBy.patch)
download | inline diff:
From a803931d1790c178eee0c7ee0c099e453edd782c Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 26 Oct 2022 14:14:11 -0700
Subject: [PATCH v5 11/14] hio: Use ExtendBufferedRelBy()
---
src/backend/access/heap/hio.c | 332 ++++++++++++++++++++--------------
1 file changed, 194 insertions(+), 138 deletions(-)
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.cindex 65886839e70..40f53d7177e 100644--- a/src/backend/access/heap/hio.c+++ b/src/backend/access/heap/hio.c@@ -186,89 +186,176 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
}
/*
- * Extend a relation by multiple blocks to avoid future contention on the- * relation extension lock. Our goal is to pre-extend the relation by an- * amount which ramps up as the degree of contention ramps up, but limiting- * the result to some sane overall value.+ * Extend the relation. By multiple pages, if beneficial.+ *+ * If the caller needs multiple pages (num_pages > 1), we always try to extend+ * by at least that much.+ *+ * If there is contention on the extension lock, we don't just extend "for+ * ourselves", but we try to help others. We can do so by adding empty pages+ * into the FSM. Typically there is no contention when we can't use the FSM.+ *+ * We do have to limit the number of pages to extend by to some value, as the+ * buffers for all the extended pages need to, temporarily, be pinned. For now+ * we define MAX_BUFFERS_TO_EXTEND_BY to be 64 buffers, it's hard to see+ * benefits with higher numbers. This partially is because copyfrom.c's+ * MAX_BUFFERED_TUPLES / MAX_BUFFERED_BYTES prevents larger multi_inserts.
*/
-static void-RelationAddExtraBlocks(Relation relation, BulkInsertState bistate)+static Buffer+RelationAddBlocks(Relation relation, BulkInsertState bistate,+ int num_pages, bool use_fsm)
{
- BlockNumber blockNum,- firstBlock = InvalidBlockNumber;- int extraBlocks;- int lockWaiters;-- /* Use the length of the lock wait queue to judge how much to extend. */- lockWaiters = RelationExtensionLockWaiterCount(relation);- if (lockWaiters <= 0)- return;+#define MAX_BUFFERS_TO_EXTEND_BY 64+ Buffer victim_buffers[MAX_BUFFERS_TO_EXTEND_BY];+ BlockNumber firstBlock = InvalidBlockNumber;+ BlockNumber firstBlockFSM = InvalidBlockNumber;+ uint32 extend_by_pages;+ uint32 not_in_fsm_pages;+ BlockNumber curBlock;+ uint32 waitcount;+ Buffer buffer;
/*
- * It might seem like multiplying the number of lock waiters by as much as- * 20 is too aggressive, but benchmarking revealed that smaller numbers- * were insufficient. 512 is just an arbitrary cap to prevent- * pathological results.+ * Determine by how many pages to try to extend by.
*/
- extraBlocks = Min(512, lockWaiters * 20);-- do+ if (bistate == NULL && !use_fsm)
{
- Buffer buffer;- Page page;- Size freespace;-
/*
- * Extend by one page. This should generally match the main-line- * extension code in RelationGetBufferForTuple, except that we hold- * the relation extension lock throughout, and we don't immediately- * initialize the page (see below).+ * If we have neither bistate, nor can use the FSM, we can't bulk+ * extend - there'd be no way to find the additional pages.
*/
- buffer = ReadBufferBI(relation, P_NEW, RBM_ZERO_AND_LOCK, bistate);- page = BufferGetPage(buffer);-- if (!PageIsNew(page))- elog(ERROR, "page %u of relation \"%s\" should be empty but is not",- BufferGetBlockNumber(buffer),- RelationGetRelationName(relation));-- /*- * Add the page to the FSM without initializing. If we were to- * initialize here, the page would potentially get flushed out to disk- * before we add any useful content. There's no guarantee that that'd- * happen before a potential crash, so we need to deal with- * uninitialized pages anyway, thus avoid the potential for- * unnecessary writes.- */-- /* we'll need this info below */- blockNum = BufferGetBlockNumber(buffer);- freespace = BufferGetPageSize(buffer) - SizeOfPageHeaderData;-- UnlockReleaseBuffer(buffer);-- /* Remember first block number thus added. */- if (firstBlock == InvalidBlockNumber)- firstBlock = blockNum;-- /*- * Immediately update the bottom level of the FSM. This has a good- * chance of making this page visible to other concurrently inserting- * backends, and we want that to happen without delay.- */- RecordPageWithFreeSpace(relation, blockNum, freespace);+ extend_by_pages = 1;+ }+ else+ {+ /*+ * Try to extend at least by the number of pages the caller needs. We+ * can remember the additional pages (either via FSM or bistate).+ */+ extend_by_pages = num_pages;++ if (!RELATION_IS_LOCAL(relation))+ waitcount = RelationExtensionLockWaiterCount(relation);+ else+ waitcount = 0;++ /*+ * Multiply the number of pages to extend by the number of waiters. Do+ * this even if we're not using the FSM, as it still relieves+ * contention, by deferring the next time this backend needs to+ * extend. In that case the extended pages will be found via+ * bistate->next_free.+ */+ extend_by_pages += extend_by_pages * waitcount;++ /*+ * Can't extend by more than MAX_BUFFERS, we need to pin them all+ * concurrently.+ */+ extend_by_pages = Min(extend_by_pages, MAX_BUFFERS_TO_EXTEND_BY);
}
- while (--extraBlocks > 0);
/*
- * Updating the upper levels of the free space map is too expensive to do- * for every block, but it's worth doing once at the end to make sure that- * subsequent insertion activity sees all of those nifty free pages we- * just inserted.+ * How many of the extended pages should be entered into the FSM?+ *+ * If we have a bistate, only enter pages that we don't need ourselves+ * into the FSM. Otherwise every other backend will immediately try to+ * use the pages this backend neds itself, causing unnecessary+ * contention. If we don't have a bistate, we can't avoid the FSM.+ *+ * Never enter the page returned into the FSM, we'll immediately use it.
*/
- FreeSpaceMapVacuumRange(relation, firstBlock, blockNum + 1);+ if (num_pages > 1 && bistate == NULL)+ not_in_fsm_pages = 1;+ else+ not_in_fsm_pages = num_pages;++ /* prepare to put another buffer into the bistate */+ if (bistate && bistate->current_buf != InvalidBuffer)+ {+ ReleaseBuffer(bistate->current_buf);+ bistate->current_buf = InvalidBuffer;+ }++ /*+ * Extend the relation. We ask for the first returned page to be locked,+ * so that we are sure that nobody has inserted into the page+ * concurrently.+ *+ * With the current MAX_BUFFERS_TO_EXTEND_BY there's no danger of+ * [auto]vacuum trying to truncate later pages as REL_TRUNCATE_MINIMUM is+ * way larger.+ */+ firstBlock = ExtendBufferedRelBy(EB_REL(relation), MAIN_FORKNUM,+ bistate ? bistate->strategy : NULL,+ EB_LOCK_FIRST,+ extend_by_pages,+ victim_buffers,+ &extend_by_pages);+ /* the buffer the function will return */+ buffer = victim_buffers[0];++ /*+ * Relation is now extended. Release pins on all buffers, except for the+ * first (which we'll return). If we decided to put pages into the FSM,+ * we can do that as part of the same loop.+ *+ * FIXME: Figure out how to better deal with doing this operation while+ * holding a buffer lock. Likely we can just release the buffer lock (to+ * reacquire it later) after initializing the page - the target page isn't+ * in the FSM, so it's going to be very rare that it's found.+ */+ curBlock = firstBlock;+ for (uint32 i = 0; i < extend_by_pages; i++, curBlock++)+ {+ Assert(curBlock == BufferGetBlockNumber(victim_buffers[i]));+ Assert(BlockNumberIsValid(curBlock));++ /* don't release the pin on the page returned by this function */+ if (i > 0)+ ReleaseBuffer(victim_buffers[i]);++ if (i >= not_in_fsm_pages && use_fsm)+ {+ if (firstBlockFSM == InvalidBlockNumber)+ firstBlockFSM = curBlock;++ RecordPageWithFreeSpace(relation,+ curBlock,+ BufferGetPageSize(victim_buffers[i]) - SizeOfPageHeaderData);+ }+ }++ if (use_fsm && firstBlockFSM != InvalidBlockNumber)+ FreeSpaceMapVacuumRange(relation, firstBlockFSM, firstBlock + num_pages);++ if (bistate)+ {+ /*+ * Remember the pages we extended by so we can use them without+ * looking into the FSM.+ */+ if (extend_by_pages > 1)+ {+ bistate->next_free = firstBlock + 1;+ bistate->last_free = firstBlock + extend_by_pages - 1;+ }+ else+ {+ bistate->next_free = InvalidBlockNumber;+ bistate->last_free = InvalidBlockNumber;+ }++ /* maintain bistate->current_buf */+ IncrBufferRefCount(buffer);+ bistate->current_buf = buffer;+ }++ return buffer;+#undef MAX_BUFFERS_TO_EXTEND_BY
}
+
/*
* RelationGetBufferForTuple
*
@@ -350,10 +437,12 @@ RelationGetBufferForTuple(Relation relation, Size len,
targetFreeSpace = 0;
BlockNumber targetBlock,
otherBlock;
- bool needLock;
len = MAXALIGN(len); /* be conservative */
+ if (num_pages <= 0)+ num_pages = 1;+
/* Bulk insert is not supported for updates, only inserts. */
Assert(otherBuffer == InvalidBuffer || !bistate);
@@ -558,83 +647,50 @@ loop:
ReleaseBuffer(buffer);
}
- /* Without FSM, always fall out of the loop and extend */- if (!use_fsm)- break;-- /*- * Update FSM as to condition of this page, and ask for another page- * to try.- */- targetBlock = RecordAndGetPageWithFreeSpace(relation,- targetBlock,- pageFreeSpace,- targetFreeSpace);- }-- /*- * Have to extend the relation.- *- * We have to use a lock to ensure no one else is extending the rel at the- * same time, else we will both try to initialize the same new page. We- * can skip locking for new or temp relations, however, since no one else- * could be accessing them.- */- needLock = !RELATION_IS_LOCAL(relation);-- /*- * If we need the lock but are not able to acquire it immediately, we'll- * consider extending the relation by multiple blocks at a time to manage- * contention on the relation extension lock. However, this only makes- * sense if we're using the FSM; otherwise, there's no point.- */- if (needLock)- {- if (!use_fsm)- LockRelationForExtension(relation, ExclusiveLock);- else if (!ConditionalLockRelationForExtension(relation, ExclusiveLock))+ if (bistate+ && bistate->next_free != InvalidBlockNumber+ && bistate->next_free <= bistate->last_free)
{
- /* Couldn't get the lock immediately; wait for it. */- LockRelationForExtension(relation, ExclusiveLock);-
/*
- * Check if some other backend has extended a block for us while- * we were waiting on the lock.+ * We bulk extended the relation before, and there are still some+ * unused pages from that extension, so we don't need to look in+ * the FSM for a new page. But do record the free space from the+ * last page, somebody might insert narrower tuples later.
*/
- targetBlock = GetPageWithFreeSpace(relation, targetFreeSpace);+ if (use_fsm)+ RecordPageWithFreeSpace(relation, targetBlock, pageFreeSpace);- /*- * If some other waiter has already extended the relation, we- * don't need to do so; just use the existing freespace.- */- if (targetBlock != InvalidBlockNumber)+ Assert(bistate->last_free != InvalidBlockNumber &&+ bistate->next_free <= bistate->last_free);+ targetBlock = bistate->next_free;+ if (bistate->next_free >= bistate->last_free)
{
- UnlockRelationForExtension(relation, ExclusiveLock);- goto loop;+ bistate->next_free = InvalidBlockNumber;+ bistate->last_free = InvalidBlockNumber;
}
-- /* Time to bulk-extend. */- RelationAddExtraBlocks(relation, bistate);+ else+ bistate->next_free++;+ }+ else if (!use_fsm)+ {+ /* Without FSM, always fall out of the loop and extend */+ break;+ }+ else+ {+ /*+ * Update FSM as to condition of this page, and ask for another+ * page to try.+ */+ targetBlock = RecordAndGetPageWithFreeSpace(relation,+ targetBlock,+ pageFreeSpace,+ targetFreeSpace);
}
}
- /*- * In addition to whatever extension we performed above, we always add at- * least one block to satisfy our own request.- *- * XXX This does an lseek - rather expensive - but at the moment it is the- * only way to accurately determine how many blocks are in a relation. Is- * it worth keeping an accurate file length in shared memory someplace,- * rather than relying on the kernel to do it for us?- */- buffer = ReadBufferBI(relation, P_NEW, RBM_ZERO_AND_LOCK, bistate);-- /*- * Release the file-extension lock; it's now OK for someone else to extend- * the relation some more.- */- if (needLock)- UnlockRelationForExtension(relation, ExclusiveLock);+ /* Have to extend the relation */+ buffer = RelationAddBlocks(relation, bistate, num_pages, use_fsm);
/*
* We need to initialize the empty new page. Double-check that it really
--
2.38.0
[text/x-diff] v5-0012-WIP-Don-t-initialize-page-in-vm-fsm-_extend-not-n.patch (2.1K, ../20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de/13-v5-0012-WIP-Don-t-initialize-page-in-vm-fsm-_extend-not-n.patch)
download | inline diff:
From 751d65ff1f8394f0513e556dd7bf331c4a1c4943 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 28 Feb 2023 20:56:32 -0800
Subject: [PATCH v5 12/14] WIP: Don't initialize page in {vm,fsm}_extend(), not
needed
---
src/backend/access/heap/visibilitymap.c | 6 +-----
src/backend/storage/freespace/freespace.c | 5 +----
2 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.cindex 74ff01bb172..709213d0d97 100644--- a/src/backend/access/heap/visibilitymap.c+++ b/src/backend/access/heap/visibilitymap.c@@ -623,11 +623,9 @@ static void
vm_extend(Relation rel, BlockNumber vm_nblocks)
{
BlockNumber vm_nblocks_now;
- PGAlignedBlock pg;+ PGAlignedBlock pg = {0};
SMgrRelation reln;
- PageInit((Page) pg.data, BLCKSZ, 0);-
/*
* We use the relation extension lock to lock out other backends trying to
* extend the visibility map at the same time. It also locks out extension
@@ -663,8 +661,6 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
/* Now extend the file */
while (vm_nblocks_now < vm_nblocks)
{
- PageSetChecksumInplace((Page) pg.data, vm_nblocks_now);-
smgrextend(reln, VISIBILITYMAP_FORKNUM, vm_nblocks_now, pg.data, false);
vm_nblocks_now++;
}
diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.cindex 3e9693b293b..90c529958e7 100644--- a/src/backend/storage/freespace/freespace.c+++ b/src/backend/storage/freespace/freespace.c@@ -608,10 +608,9 @@ static void
fsm_extend(Relation rel, BlockNumber fsm_nblocks)
{
BlockNumber fsm_nblocks_now;
- PGAlignedBlock pg;+ PGAlignedBlock pg = {0};
SMgrRelation reln;
- PageInit((Page) pg.data, BLCKSZ, 0);
/*
* We use the relation extension lock to lock out other backends trying to
@@ -649,8 +648,6 @@ fsm_extend(Relation rel, BlockNumber fsm_nblocks)
/* Extend as needed. */
while (fsm_nblocks_now < fsm_nblocks)
{
- PageSetChecksumInplace((Page) pg.data, fsm_nblocks_now);-
smgrextend(reln, FSM_FORKNUM, fsm_nblocks_now,
pg.data, false);
fsm_nblocks_now++;
--
2.38.0
[text/x-diff] v5-0013-Convert-a-few-places-to-ExtendBufferedRelTo.patch (11.6K, ../20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de/14-v5-0013-Convert-a-few-places-to-ExtendBufferedRelTo.patch)
download | inline diff:
From 6d60144e1deab40782ba5df4e53622e33fd4ed23 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 1 Mar 2023 13:08:17 -0800
Subject: [PATCH v5 13/14] Convert a few places to ExtendBufferedRelTo
---
src/backend/access/heap/visibilitymap.c | 80 +++++++---------------
src/backend/access/transam/xlogutils.c | 29 ++------
src/backend/storage/freespace/freespace.c | 83 +++++++----------------
3 files changed, 55 insertions(+), 137 deletions(-)
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.cindex 709213d0d97..60b96f5df80 100644--- a/src/backend/access/heap/visibilitymap.c+++ b/src/backend/access/heap/visibilitymap.c@@ -126,7 +126,7 @@
/* prototypes for internal routines */
static Buffer vm_readbuf(Relation rel, BlockNumber blkno, bool extend);
-static void vm_extend(Relation rel, BlockNumber vm_nblocks);+static Buffer vm_extend(Relation rel, BlockNumber vm_nblocks);
/*
@@ -575,22 +575,29 @@ vm_readbuf(Relation rel, BlockNumber blkno, bool extend)
reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = 0;
}
- /* Handle requests beyond EOF */+ /*+ * For reading we use ZERO_ON_ERROR mode, and initialize the page if+ * necessary. It's always safe to clear bits, so it's better to clear+ * corrupt pages than error out.+ *+ * We use the same path below to initialize pages when extending the+ * relation, as a concurrent extension can end up with vm_extend()+ * returning an already-initialized page.+ */
if (blkno >= reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM])
{
if (extend)
- vm_extend(rel, blkno + 1);+ buf = vm_extend(rel, blkno + 1);
else
return InvalidBuffer;
}
+ else+ buf = ReadBufferExtended(rel, VISIBILITYMAP_FORKNUM, blkno,+ RBM_ZERO_ON_ERROR, NULL);
/*
- * Use ZERO_ON_ERROR mode, and initialize the page if necessary. It's- * always safe to clear bits, so it's better to clear corrupt pages than- * error out.- *- * The initialize-the-page part is trickier than it looks, because of the- * possibility of multiple backends doing this concurrently, and our+ * Initializing the page when needed is trickier than it looks, because of+ * the possibility of multiple backends doing this concurrently, and our
* desire to not uselessly take the buffer lock in the normal path where
* the page is OK. We must take the lock to initialize the page, so
* recheck page newness after we have the lock, in case someone else
@@ -603,8 +610,6 @@ vm_readbuf(Relation rel, BlockNumber blkno, bool extend)
* long as it doesn't depend on the page header having correct contents.
* Current usage is safe because PageGetContents() does not require that.
*/
- buf = ReadBufferExtended(rel, VISIBILITYMAP_FORKNUM, blkno,- RBM_ZERO_ON_ERROR, NULL);
if (PageIsNew(BufferGetPage(buf)))
{
LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
@@ -619,51 +624,16 @@ vm_readbuf(Relation rel, BlockNumber blkno, bool extend)
* Ensure that the visibility map fork is at least vm_nblocks long, extending
* it if necessary with zeroed pages.
*/
-static void+static Buffer
vm_extend(Relation rel, BlockNumber vm_nblocks)
{
- BlockNumber vm_nblocks_now;- PGAlignedBlock pg = {0};- SMgrRelation reln;+ Buffer buf;- /*- * We use the relation extension lock to lock out other backends trying to- * extend the visibility map at the same time. It also locks out extension- * of the main fork, unnecessarily, but extending the visibility map- * happens seldom enough that it doesn't seem worthwhile to have a- * separate lock tag type for it.- *- * Note that another backend might have extended or created the relation- * by the time we get the lock.- */- LockRelationForExtension(rel, ExclusiveLock);-- /*- * Caution: re-using this smgr pointer could fail if the relcache entry- * gets closed. It's safe as long as we only do smgr-level operations- * between here and the last use of the pointer.- */- reln = RelationGetSmgr(rel);-- /*- * Create the file first if it doesn't exist. If smgr_vm_nblocks is- * positive then it must exist, no need for an smgrexists call.- */- if ((reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == 0 ||- reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == InvalidBlockNumber) &&- !smgrexists(reln, VISIBILITYMAP_FORKNUM))- smgrcreate(reln, VISIBILITYMAP_FORKNUM, false);-- /* Invalidate cache so that smgrnblocks() asks the kernel. */- reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = InvalidBlockNumber;- vm_nblocks_now = smgrnblocks(reln, VISIBILITYMAP_FORKNUM);-- /* Now extend the file */- while (vm_nblocks_now < vm_nblocks)- {- smgrextend(reln, VISIBILITYMAP_FORKNUM, vm_nblocks_now, pg.data, false);- vm_nblocks_now++;- }+ buf = ExtendBufferedRelTo(EB_REL(rel), VISIBILITYMAP_FORKNUM, NULL,+ EB_CREATE_FORK_IF_NEEDED |+ EB_CLEAR_SIZE_CACHE,+ vm_nblocks,+ RBM_ZERO_ON_ERROR);
/*
* Send a shared-inval message to force other backends to close any smgr
@@ -672,7 +642,7 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
* to keep checking for creation or extension of the file, which happens
* infrequently.
*/
- CacheInvalidateSmgr(reln->smgr_rlocator);+ CacheInvalidateSmgr(RelationGetSmgr(rel)->smgr_rlocator);- UnlockRelationForExtension(rel, ExclusiveLock);+ return buf;
}
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.cindex 2c28956b1aa..e174a2a8919 100644--- a/src/backend/access/transam/xlogutils.c+++ b/src/backend/access/transam/xlogutils.c@@ -524,28 +524,13 @@ XLogReadBufferExtended(RelFileLocator rlocator, ForkNumber forknum,
/* OK to extend the file */
/* we do this in recovery only - no rel-extension lock needed */
Assert(InRecovery);
- buffer = InvalidBuffer;- do- {- if (buffer != InvalidBuffer)- {- if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK)- LockBuffer(buffer, BUFFER_LOCK_UNLOCK);- ReleaseBuffer(buffer);- }- buffer = ReadBufferWithoutRelcache(rlocator, forknum,- P_NEW, mode, NULL, true);- }- while (BufferGetBlockNumber(buffer) < blkno);- /* Handle the corner case that P_NEW returns non-consecutive pages */- if (BufferGetBlockNumber(buffer) != blkno)- {- if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK)- LockBuffer(buffer, BUFFER_LOCK_UNLOCK);- ReleaseBuffer(buffer);- buffer = ReadBufferWithoutRelcache(rlocator, forknum, blkno,- mode, NULL, true);- }+ buffer = ExtendBufferedRelTo(EB_SMGR(smgr, RELPERSISTENCE_PERMANENT),+ forknum,+ NULL,+ EB_PERFORMING_RECOVERY |+ EB_SKIP_EXTENSION_LOCK,+ blkno + 1,+ mode);
}
recent_buffer_fast_path:
diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.cindex 90c529958e7..2face615d07 100644--- a/src/backend/storage/freespace/freespace.c+++ b/src/backend/storage/freespace/freespace.c@@ -98,7 +98,7 @@ static BlockNumber fsm_get_heap_blk(FSMAddress addr, uint16 slot);
static BlockNumber fsm_logical_to_physical(FSMAddress addr);
static Buffer fsm_readbuf(Relation rel, FSMAddress addr, bool extend);
-static void fsm_extend(Relation rel, BlockNumber fsm_nblocks);+static Buffer fsm_extend(Relation rel, BlockNumber fsm_nblocks);
/* functions to convert amount of free space to a FSM category */
static uint8 fsm_space_avail_to_cat(Size avail);
@@ -558,24 +558,30 @@ fsm_readbuf(Relation rel, FSMAddress addr, bool extend)
reln->smgr_cached_nblocks[FSM_FORKNUM] = 0;
}
- /* Handle requests beyond EOF */+ /*+ * For reading we use ZERO_ON_ERROR mode, and initialize the page if+ * necessary. The FSM information is not accurate anyway, so it's better+ * to clear corrupt pages than error out. Since the FSM changes are not+ * WAL-logged, the so-called torn page problem on crash can lead to pages+ * with corrupt headers, for example.+ *+ * We use the same path below to initialize pages when extending the+ * relation, as a concurrent extension can end up with vm_extend()+ * returning an already-initialized page.+ */
if (blkno >= reln->smgr_cached_nblocks[FSM_FORKNUM])
{
if (extend)
- fsm_extend(rel, blkno + 1);+ buf = fsm_extend(rel, blkno + 1);
else
return InvalidBuffer;
}
+ else+ buf = ReadBufferExtended(rel, FSM_FORKNUM, blkno, RBM_ZERO_ON_ERROR, NULL);
/*
- * Use ZERO_ON_ERROR mode, and initialize the page if necessary. The FSM- * information is not accurate anyway, so it's better to clear corrupt- * pages than error out. Since the FSM changes are not WAL-logged, the- * so-called torn page problem on crash can lead to pages with corrupt- * headers, for example.- *- * The initialize-the-page part is trickier than it looks, because of the- * possibility of multiple backends doing this concurrently, and our+ * Initializing the page when needed is trickier than it looks, because of+ * the possibility of multiple backends doing this concurrently, and our
* desire to not uselessly take the buffer lock in the normal path where
* the page is OK. We must take the lock to initialize the page, so
* recheck page newness after we have the lock, in case someone else
@@ -588,7 +594,6 @@ fsm_readbuf(Relation rel, FSMAddress addr, bool extend)
* long as it doesn't depend on the page header having correct contents.
* Current usage is safe because PageGetContents() does not require that.
*/
- buf = ReadBufferExtended(rel, FSM_FORKNUM, blkno, RBM_ZERO_ON_ERROR, NULL);
if (PageIsNew(BufferGetPage(buf)))
{
LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
@@ -604,56 +609,14 @@ fsm_readbuf(Relation rel, FSMAddress addr, bool extend)
* it if necessary with empty pages. And by empty, I mean pages filled
* with zeros, meaning there's no free space.
*/
-static void+static Buffer
fsm_extend(Relation rel, BlockNumber fsm_nblocks)
{
- BlockNumber fsm_nblocks_now;- PGAlignedBlock pg = {0};- SMgrRelation reln;--- /*- * We use the relation extension lock to lock out other backends trying to- * extend the FSM at the same time. It also locks out extension of the- * main fork, unnecessarily, but extending the FSM happens seldom enough- * that it doesn't seem worthwhile to have a separate lock tag type for- * it.- *- * Note that another backend might have extended or created the relation- * by the time we get the lock.- */- LockRelationForExtension(rel, ExclusiveLock);-- /*- * Caution: re-using this smgr pointer could fail if the relcache entry- * gets closed. It's safe as long as we only do smgr-level operations- * between here and the last use of the pointer.- */- reln = RelationGetSmgr(rel);-- /*- * Create the FSM file first if it doesn't exist. If- * smgr_cached_nblocks[FSM_FORKNUM] is positive then it must exist, no- * need for an smgrexists call.- */- if ((reln->smgr_cached_nblocks[FSM_FORKNUM] == 0 ||- reln->smgr_cached_nblocks[FSM_FORKNUM] == InvalidBlockNumber) &&- !smgrexists(reln, FSM_FORKNUM))- smgrcreate(reln, FSM_FORKNUM, false);-- /* Invalidate cache so that smgrnblocks() asks the kernel. */- reln->smgr_cached_nblocks[FSM_FORKNUM] = InvalidBlockNumber;- fsm_nblocks_now = smgrnblocks(reln, FSM_FORKNUM);-- /* Extend as needed. */- while (fsm_nblocks_now < fsm_nblocks)- {- smgrextend(reln, FSM_FORKNUM, fsm_nblocks_now,- pg.data, false);- fsm_nblocks_now++;- }-- UnlockRelationForExtension(rel, ExclusiveLock);+ return ExtendBufferedRelTo(EB_REL(rel), FSM_FORKNUM, NULL,+ EB_CREATE_FORK_IF_NEEDED |+ EB_CLEAR_SIZE_CACHE,+ fsm_nblocks,+ RBM_ZERO_ON_ERROR);
}
/*
--
2.38.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: andres@anarazel.de, hlinnaka@iki.fi, alvherre@alvh.no-ip.org, vignesh21@gmail.com, thomas.munro@gmail.com, melanieplageman@gmail.com, y.sokolov@postgrespro.ru, robertmhaas@gmail.com
Subject: Re: refactoring relation extension and BufferAlloc(), faster COPY
In-Reply-To: <20230326192659.hmmbizchu4zy7kix@awork3.anarazel.de>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox