Set appropriate processing mode for auxiliary processes.

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.

Sorry, forget to add an assertion to guard our codes in my previous patch.

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.

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)