Re: Checks in RegisterBackgroundWorker.()

On Thu, Sep 28, 2023 at 9:46 AM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> Here's a new version of these patches. I fixed one comment and ran
> pgindent, no other changes.

> Subject: [PATCH v2 1/3] Clarify the checks in RegisterBackgroundWorker.

LGTM. I see it passes on CI, and I also tested locally with
EXEC_BACKEND, with shared_preload_libraries=path/to/pg_prewarm.so
which works fine.

> Subject: [PATCH v2 2/3] Allocate Backend structs in PostmasterContext.

LGTM. I checked that you preserve the behaviour on OOM (LOG), and you
converted free() to pfree() in code that runs in the postmaster, but
dropped it in the code that runs in the child because all children
should delete PostmasterContext, making per-object pfree redundant.
Good.

> Subject: [PATCH v2 3/3] Fix misleading comment on StartBackgroundWorker().

LGTM. Hmm, maybe I would have called that function
"BackgroundWorkerMain()" like several other similar things, but that's
not important.

This doesn't quite fix the problem I was complaining about earlier,
but it de-confuses things. (Namely that if BackgroundWorkerList
weren't a global variable, RegisterWorkerMain() wouldn't be able to
find it, and if it took some kind of context pointer as an argument,
_PG_init() functions wouldn't be able to provide it, unless we changed
_PG_init() to take an argument, which we can't really do. Oh well.)