introduce bufmgr hooks
Hi hackers,
I'd like to propose some new hooks for the buffer manager. My primary goal
is to allow users to create an additional caching mechanism between the
shared buffers and disk for evicted buffers. For example, some EC2
instance classes have ephemeral disks that are physically attached to the
host machine that might be useful for such a cache. Presumably there are
other uses (e.g., gathering more information about the buffer cache), but
this is the main use-case I have in mind. I am proposing the following new
hooks:
* bufmgr_read_hook: called in place of smgrread() in ReadBuffer_common().
It is expected that such hooks would call smgrread() as necessary.
* bufmgr_write_hook: called before smgrwrite() in FlushBuffer(). The hook
indicateѕ whether the buffer is being evicted. Hook functions must
gracefully handle concurrent hint bit updates to the page.
* bufmgr_invalidate_hook: called within InvalidateBuffer().
The attached patch is a first attempt at introducing these hooks with
acceptable names, placements, arguments, etc.
Thoughts?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Introduce-bufmgr-hooks.patchtext/x-diff; charset=us-asciiDownload
From d1683d73df8930927a464555f256c8c91c7cf24e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Thu, 11 Aug 2022 16:24:26 -0700
Subject: [PATCH v1 1/1] Introduce bufmgr hooks.
These hooks can be used for maintaining a secondary buffer cache
outside of the regular shared buffers. In theory, there are many
other potential uses.
---
src/backend/storage/buffer/bufmgr.c | 54 ++++++++++++++++++++---------
src/include/storage/buf_internals.h | 14 ++++++++
2 files changed, 51 insertions(+), 17 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 9c1bd508d3..365272d139 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -166,6 +166,15 @@ static bool IsForInput;
/* local state for LockBufferForCleanup */
static BufferDesc *PinCountWaitBuf = NULL;
+/* hook for plugins to get control when reading in a page */
+bufmgr_read_hook_type bufmgr_read_hook = NULL;
+
+/* hook for plugins to get control when evicting a page */
+bufmgr_write_hook_type bufmgr_write_hook = NULL;
+
+/* hook for plugins to get control when invalidating a page */
+bufmgr_invalidate_hook_type bufmgr_invalidate_hook = NULL;
+
/*
* Backend-Private refcount management:
*
@@ -482,7 +491,7 @@ static BufferDesc *BufferAlloc(SMgrRelation smgr,
BlockNumber blockNum,
BufferAccessStrategy strategy,
bool *foundPtr);
-static void FlushBuffer(BufferDesc *buf, SMgrRelation reln);
+static void FlushBuffer(BufferDesc *buf, SMgrRelation reln, bool for_eviction);
static void FindAndDropRelationBuffers(RelFileLocator rlocator,
ForkNumber forkNum,
BlockNumber nForkBlock,
@@ -1015,17 +1024,22 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
instr_time io_start,
io_time;
- if (track_io_timing)
- INSTR_TIME_SET_CURRENT(io_start);
+ if (bufmgr_read_hook)
+ (*bufmgr_read_hook) (smgr, forkNum, blockNum, (char *) bufBlock);
+ else
+ {
+ if (track_io_timing)
+ INSTR_TIME_SET_CURRENT(io_start);
- smgrread(smgr, forkNum, blockNum, (char *) bufBlock);
+ smgrread(smgr, forkNum, blockNum, (char *) bufBlock);
- 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);
+ 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 */
@@ -1269,7 +1283,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
smgr->smgr_rlocator.locator.dbOid,
smgr->smgr_rlocator.locator.relNumber);
- FlushBuffer(buf, NULL);
+ FlushBuffer(buf, NULL, true);
LWLockRelease(BufferDescriptorGetContentLock(buf));
ScheduleBufferTagForWriteback(&BackendWritebackContext,
@@ -1544,6 +1558,9 @@ retry:
goto retry;
}
+ if (bufmgr_invalidate_hook)
+ (*bufmgr_invalidate_hook) (buf);
+
/*
* Clear out the buffer's tag and flags. We must do this to ensure that
* linear scans of the buffer array don't think the buffer is valid.
@@ -2573,7 +2590,7 @@ SyncOneBuffer(int buf_id, bool skip_recently_used, WritebackContext *wb_context)
PinBuffer_Locked(bufHdr);
LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED);
- FlushBuffer(bufHdr, NULL);
+ FlushBuffer(bufHdr, NULL, false);
LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
@@ -2822,7 +2839,7 @@ BufferGetTag(Buffer buffer, RelFileLocator *rlocator, ForkNumber *forknum,
* as the second parameter. If not, pass NULL.
*/
static void
-FlushBuffer(BufferDesc *buf, SMgrRelation reln)
+FlushBuffer(BufferDesc *buf, SMgrRelation reln, bool for_eviction)
{
XLogRecPtr recptr;
ErrorContextCallback errcallback;
@@ -2902,6 +2919,9 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln)
*/
bufToWrite = PageSetChecksumCopy((Page) bufBlock, buf->tag.blockNum);
+ if (bufmgr_write_hook)
+ (*bufmgr_write_hook) (reln, buf, bufToWrite, for_eviction);
+
if (track_io_timing)
INSTR_TIME_SET_CURRENT(io_start);
@@ -3584,7 +3604,7 @@ FlushRelationBuffers(Relation rel)
{
PinBuffer_Locked(bufHdr);
LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED);
- FlushBuffer(bufHdr, RelationGetSmgr(rel));
+ FlushBuffer(bufHdr, RelationGetSmgr(rel), false);
LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
UnpinBuffer(bufHdr, true);
}
@@ -3679,7 +3699,7 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels)
{
PinBuffer_Locked(bufHdr);
LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED);
- FlushBuffer(bufHdr, srelent->srel);
+ FlushBuffer(bufHdr, srelent->srel, false);
LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
UnpinBuffer(bufHdr, true);
}
@@ -3880,7 +3900,7 @@ FlushDatabaseBuffers(Oid dbid)
{
PinBuffer_Locked(bufHdr);
LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED);
- FlushBuffer(bufHdr, NULL);
+ FlushBuffer(bufHdr, NULL, false);
LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
UnpinBuffer(bufHdr, true);
}
@@ -3907,7 +3927,7 @@ FlushOneBuffer(Buffer buffer)
Assert(LWLockHeldByMe(BufferDescriptorGetContentLock(bufHdr)));
- FlushBuffer(bufHdr, NULL);
+ FlushBuffer(bufHdr, NULL, false);
}
/*
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 72466551d7..aa0eb13ed7 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -336,6 +336,20 @@ typedef struct CkptSortItem
extern PGDLLIMPORT CkptSortItem *CkptBufferIds;
+/* hook for plugins to get control when reading in a page */
+typedef void (*bufmgr_read_hook_type) (SMgrRelation smgr, ForkNumber forknum,
+ BlockNumber blocknum, char *buffer);
+extern PGDLLIMPORT bufmgr_read_hook_type bufmgr_read_hook;
+
+/* hook for plugins to get control when writing a page */
+typedef void (*bufmgr_write_hook_type) (SMgrRelation smgr, BufferDesc *buf,
+ char *buffer, bool for_eviction);
+extern PGDLLIMPORT bufmgr_write_hook_type bufmgr_write_hook;
+
+/* hook for plugins to get control when invalidating a page */
+typedef void (*bufmgr_invalidate_hook_type) (BufferDesc *buf);
+extern PGDLLIMPORT bufmgr_invalidate_hook_type bufmgr_invalidate_hook;
+
/*
* Internal buffer management routines
*/
--
2.25.1
At Mon, 29 Aug 2022 15:24:49 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
I'd like to propose some new hooks for the buffer manager. My primary goal
is to allow users to create an additional caching mechanism between the
...
The attached patch is a first attempt at introducing these hooks with
acceptable names, placements, arguments, etc.Thoughts?
smgr is an abstract interface originally intended to allow to choose
one implementation among several (though cannot dynamically). Even
though the patch intends to replace specific (but most of all) uses of
the smgrread/write, still it sounds somewhat strange to me to add
hooks to replace smgr functions in that respect. I'm not sure whether
we still regard smgr as just an interface, though..
As for the names, bufmgr_read_hook looks like as if it is additionally
called when the normal operation performed by smgrread completes, or
just before. (planner_hook already doesn't sounds so for me, though:p)
"bufmgr_alt_smgrread" works for me but I'm not sure it is following
the project policy.
I think that the INSTR_* section should enclose the hook call as it is
still an I/O operation in the view of the core.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Thanks for taking a look.
On Tue, Aug 30, 2022 at 01:02:20PM +0900, Kyotaro Horiguchi wrote:
smgr is an abstract interface originally intended to allow to choose
one implementation among several (though cannot dynamically). Even
though the patch intends to replace specific (but most of all) uses of
the smgrread/write, still it sounds somewhat strange to me to add
hooks to replace smgr functions in that respect. I'm not sure whether
we still regard smgr as just an interface, though..
I suspect that it's probably still worthwhile to provide such hooks so that
you don't have to write an entire smgr implementation. But I think you
bring up a good point.
As for the names, bufmgr_read_hook looks like as if it is additionally
called when the normal operation performed by smgrread completes, or
just before. (planner_hook already doesn't sounds so for me, though:p)
"bufmgr_alt_smgrread" works for me but I'm not sure it is following
the project policy.
Yeah, the intent is for this hook to replace the smgrread() call (although
it might end up calling smgrread()). I debated having this hook return
whether smgrread() needs to be called. Would that address your concern?
I think that the INSTR_* section should enclose the hook call as it is
still an I/O operation in the view of the core.
Okay, will do.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
HHi,
On 2022-08-29 15:24:49 -0700, Nathan Bossart wrote:
I'd like to propose some new hooks for the buffer manager. My primary goal
is to allow users to create an additional caching mechanism between the
shared buffers and disk for evicted buffers.
I'm very doubtful this is a good idea. These are quite hot paths. While not a
huge cost, adding an indirection isn't free nonetheless. I also think it'll
make it harder to improve things in this area, which needs quite a bit of
work.
Greetings,
Andres Freund
On Wed, Aug 31, 2022 at 08:29:31AM -0700, Andres Freund wrote:
I'm very doubtful this is a good idea. These are quite hot paths. While not a
huge cost, adding an indirection isn't free nonetheless.
Are you concerned about the NULL check or the potential hook
implementations? I can probably test the former pretty easily, but the
latter seems like a generic problem for many hooks.
I also think it'll
make it harder to improve things in this area, which needs quite a bit of
work.
If you have specific refactoring in mind that you think ought to be a
prerequisite for this change, I'm happy to give it a try.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Aug 30, 2022 at 03:22:43PM -0700, Nathan Bossart wrote:
Okay, will do.
v2 attached.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v2-0001-Introduce-bufmgr-hooks.patchtext/x-diff; charset=us-asciiDownload
From df1556c6da69f8c0aae9f8878f24e21907cf4d89 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Thu, 11 Aug 2022 16:24:26 -0700
Subject: [PATCH v2 1/1] Introduce bufmgr hooks.
These hooks can be used for maintaining a secondary buffer cache
outside of the regular shared buffers. In theory, there are many
other potential uses.
---
src/backend/storage/buffer/bufmgr.c | 35 +++++++++++++++++++++--------
src/include/storage/buf_internals.h | 14 ++++++++++++
2 files changed, 40 insertions(+), 9 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index e898ffad7b..5448716a1d 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -166,6 +166,15 @@ static bool IsForInput;
/* local state for LockBufferForCleanup */
static BufferDesc *PinCountWaitBuf = NULL;
+/* hook for plugins to get control when reading in a page */
+bufmgr_read_hook_type bufmgr_read_hook = NULL;
+
+/* hook for plugins to get control when writing a page */
+bufmgr_write_hook_type bufmgr_write_hook = NULL;
+
+/* hook for plugins to get control when invalidating a page */
+bufmgr_invalidate_hook_type bufmgr_invalidate_hook = NULL;
+
/*
* Backend-Private refcount management:
*
@@ -482,7 +491,7 @@ static BufferDesc *BufferAlloc(SMgrRelation smgr,
BlockNumber blockNum,
BufferAccessStrategy strategy,
bool *foundPtr);
-static void FlushBuffer(BufferDesc *buf, SMgrRelation reln);
+static void FlushBuffer(BufferDesc *buf, SMgrRelation reln, bool for_eviction);
static void FindAndDropRelationBuffers(RelFileLocator rlocator,
ForkNumber forkNum,
BlockNumber nForkBlock,
@@ -1018,7 +1027,9 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
if (track_io_timing)
INSTR_TIME_SET_CURRENT(io_start);
- smgrread(smgr, forkNum, blockNum, (char *) bufBlock);
+ if (!bufmgr_read_hook ||
+ !(*bufmgr_read_hook) (smgr, forkNum, blockNum, (char *) bufBlock))
+ smgrread(smgr, forkNum, blockNum, (char *) bufBlock);
if (track_io_timing)
{
@@ -1269,7 +1280,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
smgr->smgr_rlocator.locator.dbOid,
smgr->smgr_rlocator.locator.relNumber);
- FlushBuffer(buf, NULL);
+ FlushBuffer(buf, NULL, true);
LWLockRelease(BufferDescriptorGetContentLock(buf));
ScheduleBufferTagForWriteback(&BackendWritebackContext,
@@ -1544,6 +1555,9 @@ retry:
goto retry;
}
+ if (bufmgr_invalidate_hook)
+ (*bufmgr_invalidate_hook) (buf);
+
/*
* Clear out the buffer's tag and flags. We must do this to ensure that
* linear scans of the buffer array don't think the buffer is valid.
@@ -2573,7 +2587,7 @@ SyncOneBuffer(int buf_id, bool skip_recently_used, WritebackContext *wb_context)
PinBuffer_Locked(bufHdr);
LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED);
- FlushBuffer(bufHdr, NULL);
+ FlushBuffer(bufHdr, NULL, false);
LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
@@ -2823,7 +2837,7 @@ BufferGetTag(Buffer buffer, RelFileLocator *rlocator, ForkNumber *forknum,
* as the second parameter. If not, pass NULL.
*/
static void
-FlushBuffer(BufferDesc *buf, SMgrRelation reln)
+FlushBuffer(BufferDesc *buf, SMgrRelation reln, bool for_eviction)
{
XLogRecPtr recptr;
ErrorContextCallback errcallback;
@@ -2903,6 +2917,9 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln)
*/
bufToWrite = PageSetChecksumCopy((Page) bufBlock, buf->tag.blockNum);
+ if (bufmgr_write_hook)
+ (*bufmgr_write_hook) (reln, buf, bufToWrite, for_eviction);
+
if (track_io_timing)
INSTR_TIME_SET_CURRENT(io_start);
@@ -3589,7 +3606,7 @@ FlushRelationBuffers(Relation rel)
{
PinBuffer_Locked(bufHdr);
LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED);
- FlushBuffer(bufHdr, RelationGetSmgr(rel));
+ FlushBuffer(bufHdr, RelationGetSmgr(rel), false);
LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
UnpinBuffer(bufHdr, true);
}
@@ -3687,7 +3704,7 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels)
{
PinBuffer_Locked(bufHdr);
LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED);
- FlushBuffer(bufHdr, srelent->srel);
+ FlushBuffer(bufHdr, srelent->srel, false);
LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
UnpinBuffer(bufHdr, true);
}
@@ -3897,7 +3914,7 @@ FlushDatabaseBuffers(Oid dbid)
{
PinBuffer_Locked(bufHdr);
LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED);
- FlushBuffer(bufHdr, NULL);
+ FlushBuffer(bufHdr, NULL, false);
LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
UnpinBuffer(bufHdr, true);
}
@@ -3924,7 +3941,7 @@ FlushOneBuffer(Buffer buffer)
Assert(LWLockHeldByMe(BufferDescriptorGetContentLock(bufHdr)));
- FlushBuffer(bufHdr, NULL);
+ FlushBuffer(bufHdr, NULL, false);
}
/*
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 406db6be78..ef8b683da3 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -382,6 +382,20 @@ typedef struct CkptSortItem
extern PGDLLIMPORT CkptSortItem *CkptBufferIds;
+/* hook for plugins to get control when reading in a page */
+typedef bool (*bufmgr_read_hook_type) (SMgrRelation smgr, ForkNumber forknum,
+ BlockNumber blocknum, char *buffer);
+extern PGDLLIMPORT bufmgr_read_hook_type bufmgr_read_hook;
+
+/* hook for plugins to get control when writing a page */
+typedef void (*bufmgr_write_hook_type) (SMgrRelation smgr, BufferDesc *buf,
+ char *buffer, bool for_eviction);
+extern PGDLLIMPORT bufmgr_write_hook_type bufmgr_write_hook;
+
+/* hook for plugins to get control when invalidating a page */
+typedef void (*bufmgr_invalidate_hook_type) (BufferDesc *buf);
+extern PGDLLIMPORT bufmgr_invalidate_hook_type bufmgr_invalidate_hook;
+
/*
* Internal buffer management routines
*/
--
2.25.1
Hi,
On 2022-09-01 13:11:50 -0700, Nathan Bossart wrote:
On Wed, Aug 31, 2022 at 08:29:31AM -0700, Andres Freund wrote:
I'm very doubtful this is a good idea. These are quite hot paths. While not a
huge cost, adding an indirection isn't free nonetheless.Are you concerned about the NULL check or the potential hook
implementations? I can probably test the former pretty easily, but the
latter seems like a generic problem for many hooks.
Mostly the former. But the latter is also relevant - the lock nesting etc is
very hard to deal with if you don't know what runs inside.
I also think it'll
make it harder to improve things in this area, which needs quite a bit of
work.If you have specific refactoring in mind that you think ought to be a
prerequisite for this change, I'm happy to give it a try.
There's a few semi-active threads (e.g. about not holding multiple buffer
partition locks). One important change is to split the way we acquire buffers
for file extensions - right now we get a victim buffer while holding the
relation extension lock, because there's simply no API to do otherwise. We
need to change that so we get acquire a victim buffer before holding the
extension lock (with the buffer pinned but not [tag] valid), then we need to
get the extension lock, insert it into its new position in the buffer mapping
table.
Greetings,
Andres Freund
On Thu, Sep 01, 2022 at 05:34:03PM -0700, Andres Freund wrote:
On 2022-09-01 13:11:50 -0700, Nathan Bossart wrote:
On Wed, Aug 31, 2022 at 08:29:31AM -0700, Andres Freund wrote:
I also think it'll
make it harder to improve things in this area, which needs quite a bit of
work.If you have specific refactoring in mind that you think ought to be a
prerequisite for this change, I'm happy to give it a try.There's a few semi-active threads (e.g. about not holding multiple buffer
partition locks). One important change is to split the way we acquire buffers
for file extensions - right now we get a victim buffer while holding the
relation extension lock, because there's simply no API to do otherwise. We
need to change that so we get acquire a victim buffer before holding the
extension lock (with the buffer pinned but not [tag] valid), then we need to
get the extension lock, insert it into its new position in the buffer mapping
table.
I see, thanks for clarifying.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com