On 08/10/2026 15:52, Ashutosh Bapat wrote:
xlog.c, which has WAL_DEBUG code, is about writing WAL to the disk and
xloginsert.c, which has InitXLogInsert() is about assembling WAL
records. InitWalDebug() does not seem to fit InitXLogInsert's charter.
Maybe the relation between InitXLogInsert() and InitWalDebug() is the
same as that of XLogInsert() and XLogInsertRecord(). So it's ok.
Further it doesn't look good to call two XLog related functions from
BaseInit().
Yeah, InitXLogInsert() isn't perfect. If there was another direct caller
of XLogInsertRecord() that from somewhere else than XLogInsert(), and
that caller didn't call InitXLogInsert(), it'd not work. But
XLogInsertRecord() is not really intended to be used directly like that,
and even if it was, it'd be reasonable to assume that you'd call
InitXLogInsert() in the backend first anyway. So yeah, I also think it's OK.
We don't have a universal mechanism or convention for per-backend
initialization functions. There are many functions like InitXLogInsert()
and InitBufferManagerAccess(), but how they're all called is a little ad
hoc. And no such facility for extensions at all.
On 08/10/2026 21:53, Bharath Rupireddy wrote:
Hi,
On Thu, Oct 8, 2026 at 3:07 AM Heikki Linnakangas <[email protected]> wrote:
Hmm, it's a bit ugly to initialize what is a purely backend-private
thing in the ShmemInit/Attach() functions. It was expedient in the past,
because the ShmemInit() functions happened to run at the right times,
but initializing the memory context was never related to shared memory
in any way. I propose the attached, which calls the InitWalDebug()
function from InitXLogInsert() instead.
Thanks. Yes, this fix is better than mine. Since the wal_debug memory
context is only used while inserting WAL records, moving it to the WAL
insert working buffers initialization code is correct. The patch LGTM.
Ok, committed, thanks!
Searching for similar cases where we do backend-private initialization
that is not related to shared memory in the shmem callbacks, I found
BufferManagerShmemAttach():
static void
BufferManagerShmemAttach(void *arg)
{
/* Initialize per-backend file flush context */
WritebackContextInit(&BackendWritebackContext,
&backend_flush_after);
}
There's no bug here, but I propose that we also move that to
InitBufferManagerAccess(), per the second attached patch.
Moving the per-backend file flush context to the buffer pool access
init function looks good to me. Do we need to keep
BackendWritebackContext as extern? A quick check on
https://sourcegraph.com says no, but just in case any external module
needs it.
I don't think there's any need to keep it as an extern. An extension
might want to use their own WritebackContext, maybe, but I don't think
they should be messing with BackendWritebackContext.
Committed this too.
- Heikki
