Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()

At Tue, 12 Oct 2021 13:09:47 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> On Mon, Oct 11, 2021 at 8:59 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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