Thread: Set appropriate processing mode for auxiliary processes.
Hi hackers, After several refactoring iterations, auxiliary processes are no longer initialized from the bootstrapper. I think using the InitProcessing mode for initializing auxiliary processes is more appropriate. Best Regards, Xing
Attachment
On 09/05/2024 16:12, Xing Guo wrote: > Hi hackers, > > After several refactoring iterations, auxiliary processes are no > longer initialized from the bootstrapper. I think using the > InitProcessing mode for initializing auxiliary processes is more > appropriate. At first I was sure this was introduced by my refactorings in v17, but in fact it's been like this forever. I agree that InitProcessing makes much more sense. The ProcessingMode variable is initialized to InitProcessing, so I think we can simply remove that line from AuxiliaryProcessMainCommon(). There are existing "SetProcessingMode(InitProcessing)" calls in other Main functions too (AutoVacLauncherMain, BackgroundWorkerMain, etc.), and I think those can also be removed. -- Heikki Linnakangas Neon (https://neon.tech)
On Thu, May 9, 2024 at 10:13 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > On 09/05/2024 16:12, Xing Guo wrote: > > Hi hackers, > > > > After several refactoring iterations, auxiliary processes are no > > longer initialized from the bootstrapper. I think using the > > InitProcessing mode for initializing auxiliary processes is more > > appropriate. > > At first I was sure this was introduced by my refactorings in v17, but > in fact it's been like this forever. I agree that InitProcessing makes > much more sense. The ProcessingMode variable is initialized to > InitProcessing, so I think we can simply remove that line from > AuxiliaryProcessMainCommon(). There are existing > "SetProcessingMode(InitProcessing)" calls in other Main functions too > (AutoVacLauncherMain, BackgroundWorkerMain, etc.), and I think those can > also be removed. Good catch! I agree with you. Best Regards, Xing.
Attachment
Sorry, forget to add an assertion to guard our codes in my previous patch.
Attachment
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> At first I was sure this was introduced by my refactorings in v17, but
> in fact it's been like this forever. I agree that InitProcessing makes
> much more sense. The ProcessingMode variable is initialized to
> InitProcessing, so I think we can simply remove that line from
> AuxiliaryProcessMainCommon(). There are existing
> "SetProcessingMode(InitProcessing)" calls in other Main functions too
> (AutoVacLauncherMain, BackgroundWorkerMain, etc.), and I think those can
> also be removed.
This only works if the postmaster can be trusted never to change the
variable; else children could inherit some other value via fork().
In that connection, it seems a bit scary that postmaster.c contains a
couple of calls "SetProcessingMode(NormalProcessing)". It looks like
they are in functions that should only be executed by child processes,
but should we try to move them somewhere else? Another idea could be
to add an Assert to SetProcessingMode that insists that it can't be
executed by the postmaster.
regards, tom lane
On Thu, May 9, 2024 at 11:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Heikki Linnakangas <hlinnaka@iki.fi> writes: > > At first I was sure this was introduced by my refactorings in v17, but > > in fact it's been like this forever. I agree that InitProcessing makes > > much more sense. The ProcessingMode variable is initialized to > > InitProcessing, so I think we can simply remove that line from > > AuxiliaryProcessMainCommon(). There are existing > > "SetProcessingMode(InitProcessing)" calls in other Main functions too > > (AutoVacLauncherMain, BackgroundWorkerMain, etc.), and I think those can > > also be removed. > > This only works if the postmaster can be trusted never to change the > variable; else children could inherit some other value via fork(). > In that connection, it seems a bit scary that postmaster.c contains a > couple of calls "SetProcessingMode(NormalProcessing)". It looks like > they are in functions that should only be executed by child processes, > but should we try to move them somewhere else? After checking calls to "SetProcessingMode(NormalProcessing)" in the postmaster.c, they are used in background worker specific functions (BackgroundWorkerInitializeConnectionByOid and BackgroundWorkerInitializeConnection). So I think it's a good idea to move these functions to bgworker.c. Then, we can get rid of calling "SetProcessingMode(NormalProcessing)" in postmaster.c. I also noticed that there's an unnecessary call to "BackgroundWorkerInitializeConnection" in worker_spi.c (The worker_spi launcher has set the dboid correctly). Best Regards, Xing.
Attachment
On 10/05/2024 05:58, Xing Guo wrote: > On Thu, May 9, 2024 at 11:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >> Heikki Linnakangas <hlinnaka@iki.fi> writes: >>> At first I was sure this was introduced by my refactorings in v17, but >>> in fact it's been like this forever. I agree that InitProcessing makes >>> much more sense. The ProcessingMode variable is initialized to >>> InitProcessing, so I think we can simply remove that line from >>> AuxiliaryProcessMainCommon(). There are existing >>> "SetProcessingMode(InitProcessing)" calls in other Main functions too >>> (AutoVacLauncherMain, BackgroundWorkerMain, etc.), and I think those can >>> also be removed. >> >> This only works if the postmaster can be trusted never to change the >> variable; else children could inherit some other value via fork(). >> In that connection, it seems a bit scary that postmaster.c contains a >> couple of calls "SetProcessingMode(NormalProcessing)". It looks like >> they are in functions that should only be executed by child processes, >> but should we try to move them somewhere else? > > After checking calls to "SetProcessingMode(NormalProcessing)" in the > postmaster.c, they are used in background worker specific functions > (BackgroundWorkerInitializeConnectionByOid and > BackgroundWorkerInitializeConnection). So I think it's a good idea to > move these functions to bgworker.c. Then, we can get rid of calling > "SetProcessingMode(NormalProcessing)" in postmaster.c. Committed these first two patches. Thank you! > I also noticed that there's an unnecessary call to > "BackgroundWorkerInitializeConnection" in worker_spi.c (The worker_spi > launcher has set the dboid correctly). No, you can call the launcher function with "dboid=0", and it's also 0 in the "static" registration at end of _PG_init(). This causes regression tests to fail too because of that. So I left out that patch. -- Heikki Linnakangas Neon (https://neon.tech)