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 <hlinnaka@iki.fi> 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
