Thread: 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
Attachment
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
Attachment
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