Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Remove redundant SetProcessingMode(InitProcessing) calls

  2. Move bgworker specific logic to bgworker.c

  1. Set appropriate processing mode for auxiliary processes.

    Xing Guo <higuoxing@gmail.com> — 2024-05-09T13:12:45Z

    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

  2. Re: Set appropriate processing mode for auxiliary processes.

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-05-09T14:13:14Z

    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)

  3. Re: Set appropriate processing mode for auxiliary processes.

    Xing Guo <higuoxing@gmail.com> — 2024-05-09T14:55:57Z

    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.

  4. Re: Set appropriate processing mode for auxiliary processes.

    Xing Guo <higuoxing@gmail.com> — 2024-05-09T15:10:41Z

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

  5. Re: Set appropriate processing mode for auxiliary processes.

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-05-09T15:19:11Z

    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

  6. Re: Set appropriate processing mode for auxiliary processes.

    Xing Guo <higuoxing@gmail.com> — 2024-05-10T02:58:33Z

    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.

  7. Re: Set appropriate processing mode for auxiliary processes.

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-07-02T17:16:19Z

    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)