At Tue, 12 Oct 2021 13:09:47 +0530, Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote in > On Mon, Oct 11, 2021 at 8:59 PM Fujii Masao <masao.fu...@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
