Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit() - Mailing list pgsql-hackers

At Tue, 12 Oct 2021 13:09:47 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in 
> On Mon, Oct 11, 2021 at 8:59 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >
> > On 2021/10/11 19:46, Bharath Rupireddy wrote:
> > > If we do the above, then the problem might arise if somebody calls
> > > SICleanupQueue and wants to signal the startup process, the below code
> > > (from SICleanupQueue) can't get the startup process backend id. So,
> > > the backend id calculation for the startup process can't just be
> > > MaxBackends + MyAuxProcType + 1.
> > > BackendId his_backendId = (needSig - &segP->procState[0]) + 1;
> >
> > Attached POC patch illustrates what I'm in mind. ISTM this change
> > doesn't prevent SICleanupQueue() from getting right backend ID
> > of the startup process. Thought?
> 
> The patch looks good to me unless I'm missing something badly.
> 
> + Assert(MyBackendId == InvalidBackendId ||
> +    MyBackendId <= segP->maxBackends);
> +
> In the above assertion, we can just do MyBackendId ==
> segP->maxBackends, instead of <= as the startup process is the only
> process that calls SharedInvalBackendInit with pre-calculated
> MyBackendId = MaxBackends + MyAuxProcType + 1; and it will always
> occupy the last slot in the procState array.

+1 for not allowing to explicitly specify the "auto-assigned"
backendid range,

> Otherwise, we could discard defining MyBackendId in auxprocess.c and
> define the MyBackendId in the SharedInvalBackendInit itself as this is
> the function that defines the MyBackendId for everyone whoever
> requires it. I prefer this approach over what's done in PoC patch.
> 
> In SharedInvalBackendInit:
> Assert(MyBackendId == InvalidBackendId);
> /*
> * The startup process requires a valid BackendId for the SI message
> * buffer and virtual transaction id, so define it here with the value with
> * which the procsignal array slot was allocated in AuxiliaryProcessMain.
> * All other auxiliary processes don't need it.
> */
> if (MyAuxProcType == StartupProcess)
>     MyBackendId = MaxBackends + MyAuxProcType + 1;
> 
> I think this solution, coupled with the one proposed at [1], should
> solve this startup process's inconsistency in MyBackendId, procState
> and ProcSignal array slot problems.
> 
> [1] -
https://www.postgresql.org/message-id/CALj2ACXZ_o7rcOb7-Rs96P0d%3DEi%2Bnvf_WZ-Meky7Vv%2BnQNFYjQ%40mail.gmail.com

The patch does this:

         case StartupProcess:
+            MyBackendId = MaxBackends + MyAuxProcType + 1;

as well as this:

+    shmInvalBuffer->maxBackends = MaxBackends + 1;

These don't seem to be in the strict correspondence.  I'd prefer
something like the following.

+   /* currently only StartupProcess uses nailed SI slot */
+    shmInvalBuffer->maxBackends = MaxBackends + StartupProcess + 1;

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center