Обсуждение: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Bharath Rupireddy
Дата:
Hi, It looks like auxiliary processes will not have a valid MyBackendId as they don't call InitPostgres() and SharedInvalBackendInit() unlike backends. But the startup process (which is an auxiliary process) in hot standby mode seems to be different in the way that it does have a valid MyBackendId as SharedInvalBackendInit() gets called from InitRecoveryTransactionEnvironment(). The SharedInvalBackendInit() usually stores the MyBackendId in the caller's PGPROC structure i.e. MyProc->backendId. The auxiliary processes (including the startup process) usually register themselves in procsignal array with ProcSignalInit(MaxBackends + MyAuxProcType + 1) unlike the backends with ProcSignalInit(MyBackendId) after SharedInvalBackendInit() in InitPostgres(). The problem comes when a postgres process wants to send a multiplexed SIGUSR1 signal (probably using SendProcSignal()) to the startup process after receiving its ProcSignal->psh_slot[] with its backendId from the PGPROC (the postgres process can get the startup process PGPROC structure from AuxiliaryPidGetProc()). Remember the startup process has registered in the procsignal array with ProcSignalInit(MaxBackends + MyAuxProcType + 1), not with the ProcSignalInit(MyBackendId) like the backends did. So, the postgres process, wanting to send SIGUSR1 to the startup process, refers to the wrong ProcSignal->psh_slot[] and may not send the signal. Is this inconsistency of MyBackendId for a startup process a problem at all? Thoughts? These are the following ways I think we can fix it, if at all some other hacker agrees that it is actually an issue: 1) Fix the startup process code, probably by unregistering the procsignal array entry that was made with ProcSignalInit(MaxBackends + MyAuxProcType + 1) in AuxiliaryProcessMain() and register with ProcSignalInit(MyBackendId) immediately after SharedInvalBackendInit() calculates the MyBackendId in with SharedInvalBackendInit() in InitRecoveryTransactionEnvironment(). This seems risky to me as unregistering and registering ProcSignal array involves some barriers and during the registering and unregistering window, the startup process may miss the SIGUSR1. 2) Ensure that the process, that wants to send the startup process SIGUSR1 signal, doesn't use the backendId from the startup process PGPROC, in which case it has to loop over all the entries of ProcSignal->psh_slot[] array to find the entry with the startup process PID. It seems easier and less riskier but only caveat is that the sending process shouldn't look at the backendId from auxiliary process PGPROC, instead it should just traverse the entire proc signal array to find the right slot. 3) Add a comment around AuxiliaryPidGetProc() that says "auxiliary processes don't have valid backend ids, so don't use the backendId from the returned PGPROC". (2) and (3) seem reasonable to me. Thoughts? Regards, Bharath Rupireddy.
On 2021/10/09 22:22, Bharath Rupireddy wrote: > Hi, > > It looks like auxiliary processes will not have a valid MyBackendId as > they don't call InitPostgres() and SharedInvalBackendInit() unlike > backends. But the startup process (which is an auxiliary process) in > hot standby mode seems to be different in the way that it does have a > valid MyBackendId as SharedInvalBackendInit() gets called from > InitRecoveryTransactionEnvironment(). The SharedInvalBackendInit() > usually stores the MyBackendId in the caller's PGPROC structure i.e. > MyProc->backendId. The auxiliary processes (including the startup > process) usually register themselves in procsignal array with > ProcSignalInit(MaxBackends + MyAuxProcType + 1) unlike the backends > with ProcSignalInit(MyBackendId) after SharedInvalBackendInit() in > InitPostgres(). > > The problem comes when a postgres process wants to send a multiplexed > SIGUSR1 signal (probably using SendProcSignal()) to the startup > process after receiving its ProcSignal->psh_slot[] with its backendId > from the PGPROC (the postgres process can get the startup process > PGPROC structure from AuxiliaryPidGetProc()). Remember the startup > process has registered in the procsignal array with > ProcSignalInit(MaxBackends + MyAuxProcType + 1), not with the > ProcSignalInit(MyBackendId) like the backends did. So, the postgres > process, wanting to send SIGUSR1 to the startup process, refers to the > wrong ProcSignal->psh_slot[] and may not send the signal. > > Is this inconsistency of MyBackendId for a startup process a problem > at all? Thoughts? > > These are the following ways I think we can fix it, if at all some > other hacker agrees that it is actually an issue: > > 1) Fix the startup process code, probably by unregistering the > procsignal array entry that was made with ProcSignalInit(MaxBackends + > MyAuxProcType + 1) in AuxiliaryProcessMain() and register with > ProcSignalInit(MyBackendId) immediately after SharedInvalBackendInit() > calculates the MyBackendId in with SharedInvalBackendInit() in > InitRecoveryTransactionEnvironment(). This seems risky to me as > unregistering and registering ProcSignal array involves some barriers > and during the registering and unregistering window, the startup > process may miss the SIGUSR1. > > 2) Ensure that the process, that wants to send the startup process > SIGUSR1 signal, doesn't use the backendId from the startup process > PGPROC, in which case it has to loop over all the entries of > ProcSignal->psh_slot[] array to find the entry with the startup > process PID. It seems easier and less riskier but only caveat is that > the sending process shouldn't look at the backendId from auxiliary > process PGPROC, instead it should just traverse the entire proc signal > array to find the right slot. > > 3) Add a comment around AuxiliaryPidGetProc() that says "auxiliary > processes don't have valid backend ids, so don't use the backendId > from the returned PGPROC". > > (2) and (3) seem reasonable to me. Thoughts? How about modifying SharedInvalBackendInit() so that it accepts BackendId as an argument and allocates the ProcState entry of the specified BackendId? That is, the startup process determines that its BackendId is "MaxBackends + MyAuxProcType (=StartupProcess) + 1" in AuxiliaryProcessMain(), and then it passes that BackendId to SharedInvalBackendInit() in InitRecoveryTransactionEnvironment(). Maybe you need to enlarge ProcState array so that it also handles auxiliary processes if it does not for now. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Kyotaro Horiguchi
Дата:
At Mon, 11 Oct 2021 15:24:46 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in > > > On 2021/10/09 22:22, Bharath Rupireddy wrote: > > Hi, > > It looks like auxiliary processes will not have a valid MyBackendId as > > they don't call InitPostgres() and SharedInvalBackendInit() unlike > > backends. But the startup process (which is an auxiliary process) in > > hot standby mode seems to be different in the way that it does have a > > valid MyBackendId as SharedInvalBackendInit() gets called from > > InitRecoveryTransactionEnvironment(). The SharedInvalBackendInit() > > usually stores the MyBackendId in the caller's PGPROC structure i.e. > > MyProc->backendId. The auxiliary processes (including the startup > > process) usually register themselves in procsignal array with > > ProcSignalInit(MaxBackends + MyAuxProcType + 1) unlike the backends > > with ProcSignalInit(MyBackendId) after SharedInvalBackendInit() in > > InitPostgres(). > > The problem comes when a postgres process wants to send a multiplexed > > SIGUSR1 signal (probably using SendProcSignal()) to the startup > > process after receiving its ProcSignal->psh_slot[] with its backendId > > from the PGPROC (the postgres process can get the startup process > > PGPROC structure from AuxiliaryPidGetProc()). Remember the startup > > process has registered in the procsignal array with > > ProcSignalInit(MaxBackends + MyAuxProcType + 1), not with the > > ProcSignalInit(MyBackendId) like the backends did. So, the postgres > > process, wanting to send SIGUSR1 to the startup process, refers to the > > wrong ProcSignal->psh_slot[] and may not send the signal. > > Is this inconsistency of MyBackendId for a startup process a problem > > at all? Thoughts? > > These are the following ways I think we can fix it, if at all some > > other hacker agrees that it is actually an issue: > > 1) Fix the startup process code, probably by unregistering the > > procsignal array entry that was made with ProcSignalInit(MaxBackends + > > MyAuxProcType + 1) in AuxiliaryProcessMain() and register with > > ProcSignalInit(MyBackendId) immediately after SharedInvalBackendInit() > > calculates the MyBackendId in with SharedInvalBackendInit() in > > InitRecoveryTransactionEnvironment(). This seems risky to me as > > unregistering and registering ProcSignal array involves some barriers > > and during the registering and unregistering window, the startup > > process may miss the SIGUSR1. > > 2) Ensure that the process, that wants to send the startup process > > SIGUSR1 signal, doesn't use the backendId from the startup process > > PGPROC, in which case it has to loop over all the entries of > > ProcSignal->psh_slot[] array to find the entry with the startup > > process PID. It seems easier and less riskier but only caveat is that > > the sending process shouldn't look at the backendId from auxiliary > > process PGPROC, instead it should just traverse the entire proc signal > > array to find the right slot. > > 3) Add a comment around AuxiliaryPidGetProc() that says "auxiliary > > processes don't have valid backend ids, so don't use the backendId > > from the returned PGPROC". > > (2) and (3) seem reasonable to me. Thoughts? (I'm not sure how the trouble happens.) 2 and 3 looks like fixing inconsistency by another inconsistency. I'm not sure 1 is acceptable. > How about modifying SharedInvalBackendInit() so that it accepts > BackendId as an argument and allocates the ProcState entry of > the specified BackendId? That is, the startup process determines > that its BackendId is "MaxBackends + MyAuxProcType (=StartupProcess) + > 1" > in AuxiliaryProcessMain(), and then it passes that BackendId to > SharedInvalBackendInit() in InitRecoveryTransactionEnvironment(). > > Maybe you need to enlarge ProcState array so that it also handles > auxiliary processes if it does not for now. It seems to me that the backendId on startup process is used only for vxid generation. Actually "make check-world" doesn't fail by skipping SharedInvalBackendinit() (and disabling an assertion). I thought that we could decouple vxid from backend ID (or sinval code) by using pgprocno for vxid generation instead of backend ID. "make check-world" doesn't fail with that change, too. (I don't think "doesn't fail" ncecessarily mean that that change is correct, though), but vxid gets somewhat odd after the change.. =# select distinct virtualxid from pg_locks; virtualxid ------------ 116/1 # startup 99/48 # backend 1 ... regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Bharath Rupireddy
Дата:
On Mon, Oct 11, 2021 at 12:41 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> > > These are the following ways I think we can fix it, if at all some
> > > other hacker agrees that it is actually an issue:
> > > 1) Fix the startup process code, probably by unregistering the
> > > procsignal array entry that was made with ProcSignalInit(MaxBackends +
> > > MyAuxProcType + 1) in AuxiliaryProcessMain() and register with
> > > ProcSignalInit(MyBackendId) immediately after SharedInvalBackendInit()
> > > calculates the MyBackendId in with SharedInvalBackendInit() in
> > > InitRecoveryTransactionEnvironment(). This seems risky to me as
> > > unregistering and registering ProcSignal array involves some barriers
> > > and during the registering and unregistering window, the startup
> > > process may miss the SIGUSR1.
> > > 2) Ensure that the process, that wants to send the startup process
> > > SIGUSR1 signal, doesn't use the backendId from the startup process
> > > PGPROC, in which case it has to loop over all the entries of
> > > ProcSignal->psh_slot[] array to find the entry with the startup
> > > process PID. It seems easier and less riskier but only caveat is that
> > > the sending process shouldn't look at the backendId from auxiliary
> > > process PGPROC, instead it should just traverse the entire proc signal
> > > array to find the right slot.
> > > 3) Add a comment around AuxiliaryPidGetProc() that says "auxiliary
> > > processes don't have valid backend ids, so don't use the backendId
> > > from the returned PGPROC".
> > > (2) and (3) seem reasonable to me. Thoughts?
>
> (I'm not sure how the trouble happens.)
The order in which SharedInvalBackendInit and ProcSignalInit should be
used to keep procState and ProcSignal array entries for backend and
auxiliary processes is as follows:
call SharedInvalBackendInit() and let it calculate the MyBackendId
call ProcSignalInit(MyBackendId);
But for the startup process it does the opposite way, so the procState
and ProcSignal array entries are not in sync.
call ProcSignalInit(MaxBackends + MyAuxProcType + 1);
call SharedInvalBackendInit() and let it calculate the MyBackendId
If some process wants to send the startup process SIGUSR1 with the
PGPROC->backendId and SendProcSignal(PGPROC->pid, XXXXX,
PGPROC->backendId) after getting the PGPROC entry from
AuxiliaryPidGetProc(), then the signal isn't sent. To understand this
issue, please use a sample patch at [1], have a standby setup, call
pg_log_backend_memory_contexts with startup process pid on the
standby, the error "could not send signal to process" is shown, see
[2].
[1]
diff --git a/src/backend/utils/adt/mcxtfuncs.c
b/src/backend/utils/adt/mcxtfuncs.c
index 0d52613bc3..2739591edc 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -185,6 +185,10 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
proc = BackendPidGetProc(pid);
+ /* see if the given process is an auxiliary process. */
+ if (proc == NULL)
+ proc = AuxiliaryPidGetProc(pid);
+
/*
* BackendPidGetProc returns NULL if the pid isn't valid; but
by the time
* we reach kill(), a process for which we get a valid proc here might
[2]
postgres=# select pg_log_backend_memory_contexts(726901);
WARNING: could not send signal to process 726901: No such process
pg_log_backend_memory_contexts
--------------------------------
f
(1 row)
> > How about modifying SharedInvalBackendInit() so that it accepts
> > BackendId as an argument and allocates the ProcState entry of
> > the specified BackendId? That is, the startup process determines
> > that its BackendId is "MaxBackends + MyAuxProcType (=StartupProcess) +
> > 1"
> > in AuxiliaryProcessMain(), and then it passes that BackendId to
> > SharedInvalBackendInit() in InitRecoveryTransactionEnvironment().
> >
> > Maybe you need to enlarge ProcState array so that it also handles
> > auxiliary processes if it does not for now.
>
> It seems to me that the backendId on startup process is used only for
> vxid generation. Actually "make check-world" doesn't fail by skipping
> SharedInvalBackendinit() (and disabling an assertion).
>
> I thought that we could decouple vxid from backend ID (or sinval code)
> by using pgprocno for vxid generation instead of backend ID. "make
> check-world" doesn't fail with that change, too. (I don't think
> "doesn't fail" ncecessarily mean that that change is correct, though),
> but vxid gets somewhat odd after the change..
>
> =# select distinct virtualxid from pg_locks;
> virtualxid
> ------------
>
> 116/1 # startup
> 99/48 # backend 1
I'm not sure we go that path. Others may have better thoughts.
Regards,
Bharath Rupireddy.
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Bharath Rupireddy
Дата:
On Mon, Oct 11, 2021 at 11:54 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > How about modifying SharedInvalBackendInit() so that it accepts > BackendId as an argument and allocates the ProcState entry of > the specified BackendId? That is, the startup process determines > that its BackendId is "MaxBackends + MyAuxProcType (=StartupProcess) + 1" > in AuxiliaryProcessMain(), and then it passes that BackendId to > SharedInvalBackendInit() in InitRecoveryTransactionEnvironment(). 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; > Maybe you need to enlarge ProcState array so that it also handles > auxiliary processes if it does not for now. It looks like we need to increase the size of the ProcState array by 1 at least (for the startup process). Currently the ProcState array doesn't have entries for auxiliary processes, it does have entries for MaxBackends. The startup process is eating up one slot from MaxBackends. Since we need only an extra ProcState array slot for the startup process I think we could just extend its size by 1. Instead of modifying the MaxBackends definition, we can just add 1 (and a comment saying this 1 is for startup process) to shmInvalBuffer->maxBackends in SInvalShmemSize, CreateSharedInvalidationState. IMO, this has to go in a separate patch and probably in a separate thread. Thoughts? Regards, Bharath Rupireddy.
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? > It looks like we need to increase the size of the ProcState array by 1 > at least (for the startup process). Currently the ProcState array > doesn't have entries for auxiliary processes, it does have entries for > MaxBackends. The startup process is eating up one slot from > MaxBackends. Since we need only an extra ProcState array slot for the > startup process I think we could just extend its size by 1. Instead of > modifying the MaxBackends definition, we can just add 1 (and a comment > saying this 1 is for startup process) to shmInvalBuffer->maxBackends > in SInvalShmemSize, CreateSharedInvalidationState. IMO, this has to go > in a separate patch and probably in a separate thread. Thoughts? Agreed. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Вложения
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Bharath Rupireddy
Дата:
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? I will take a look at it a bit later. > > It looks like we need to increase the size of the ProcState array by 1 > > at least (for the startup process). Currently the ProcState array > > doesn't have entries for auxiliary processes, it does have entries for > > MaxBackends. The startup process is eating up one slot from > > MaxBackends. Since we need only an extra ProcState array slot for the > > startup process I think we could just extend its size by 1. Instead of > > modifying the MaxBackends definition, we can just add 1 (and a comment > > saying this 1 is for startup process) to shmInvalBuffer->maxBackends > > in SInvalShmemSize, CreateSharedInvalidationState. IMO, this has to go > > in a separate patch and probably in a separate thread. Thoughts? > > Agreed. Posted a patch in a separate thread, please review it. https://www.postgresql.org/message-id/CALj2ACXZ_o7rcOb7-Rs96P0d%3DEi%2Bnvf_WZ-Meky7Vv%2BnQNFYjQ%40mail.gmail.com Regards, Bharath Rupireddy.
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Bharath Rupireddy
Дата:
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.
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
Regards,
Bharath Rupireddy.
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Kyotaro Horiguchi
Дата:
At Mon, 11 Oct 2021 16:03:57 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in > On Mon, Oct 11, 2021 at 12:41 PM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: > > (I'm not sure how the trouble happens.) ... > If some process wants to send the startup process SIGUSR1 with the > PGPROC->backendId and SendProcSignal(PGPROC->pid, XXXXX, > PGPROC->backendId) after getting the PGPROC entry from > AuxiliaryPidGetProc(), then the signal isn't sent. To understand this > issue, please use a sample patch at [1], have a standby setup, call > pg_log_backend_memory_contexts with startup process pid on the > standby, the error "could not send signal to process" is shown, see > [2]. Thanks, I understand that this doesn't happen on vanilla PG. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Kyotaro Horiguchi
Дата:
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
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Bharath Rupireddy
Дата:
On Tue, Oct 12, 2021 at 2:03 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > [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; I don't think it is a good idea to use macro StartupProcess (because the macro might get changed to a different value later). What we essentially need to do for procState array is to extend its size by 1 (for startup process) which is being handled separately in [1]. Once the patch at [1] gets in, the patch proposed here will not have the above change at all. [1] - https://www.postgresql.org/message-id/CALj2ACXZ_o7rcOb7-Rs96P0d%3DEi%2Bnvf_WZ-Meky7Vv%2BnQNFYjQ%40mail.gmail.com Regards, Bharath Rupireddy.
On 2021/10/12 16:39, Bharath Rupireddy wrote: > 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; Yes, this is an option. But, at [1], you're proposing to enhance pg_log_backend_memory_contexts() so that it can send the request to even auxiliary processes. If we need to assign a backend ID to an auxiliary process other than the startup process and use it to send the signal promptly to those auxiliary processes, this design might not be good. Since those auxiliary processes don't call SharedInvalBackendInit(), backend IDs for them might need to be assigned outside SharedInvalBackendInit(). Thought? [1] https://postgr.es/m/CALj2ACU1nBzpacOK2q=a65S_4+Oaz_rLTsU1Ri0gf7YUmnmhfQ@mail.gmail.com Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Hi, On 2021-10-11 15:24:46 +0900, Fujii Masao wrote: > How about modifying SharedInvalBackendInit() so that it accepts > BackendId as an argument and allocates the ProcState entry of > the specified BackendId? That is, the startup process determines > that its BackendId is "MaxBackends + MyAuxProcType (=StartupProcess) + 1" > in AuxiliaryProcessMain(), and then it passes that BackendId to > SharedInvalBackendInit() in InitRecoveryTransactionEnvironment(). If I understand correctly what you're proposing, I think that's going in the wrong direction. We should work towards getting rid of BackendIds instead. This whole complication vanishes if we make sinvaladt use pgprocno. See https://postgr.es/m/20210802171255.k4yv5cfqaqbuuy6f%40alap3.anarazel.de for some discussion of this. Greetings, Andres Freund
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Kyotaro Horiguchi
Дата:
At Tue, 12 Oct 2021 14:57:58 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in > On Tue, Oct 12, 2021 at 2:03 PM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: > > > [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; > > I don't think it is a good idea to use macro StartupProcess (because > the macro might get changed to a different value later). What we If wo, we shouldn't use MyAuxProcType at the "case StartupProcess". > essentially need to do for procState array is to extend its size by 1 > (for startup process) which is being handled separately in [1]. Once > the patch at [1] gets in, the patch proposed here will not have the > above change at all. > > [1] - https://www.postgresql.org/message-id/CALj2ACXZ_o7rcOb7-Rs96P0d%3DEi%2Bnvf_WZ-Meky7Vv%2BnQNFYjQ%40mail.gmail.com -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Bharath Rupireddy
Дата:
On Wed, Oct 13, 2021 at 4:25 AM Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2021-10-11 15:24:46 +0900, Fujii Masao wrote: > > How about modifying SharedInvalBackendInit() so that it accepts > > BackendId as an argument and allocates the ProcState entry of > > the specified BackendId? That is, the startup process determines > > that its BackendId is "MaxBackends + MyAuxProcType (=StartupProcess) + 1" > > in AuxiliaryProcessMain(), and then it passes that BackendId to > > SharedInvalBackendInit() in InitRecoveryTransactionEnvironment(). > > If I understand correctly what you're proposing, I think that's going in the > wrong direction. We should work towards getting rid of BackendIds > instead. This whole complication vanishes if we make sinvaladt use pgprocno. > > See https://postgr.es/m/20210802171255.k4yv5cfqaqbuuy6f%40alap3.anarazel.de > for some discussion of this. Will any of the backends get pgprocno greater than MaxBackends? The pgprocno can range from 0 to ((MaxBackends + NUM_AUXILIARY_PROCS + max_prepared_xacts) - 1) and the ProcState array size is MaxBackends. How do we put a backend with pgprocno > MaxBackends, into the ProcState array? Is it that we also increase ProcState array size to (MaxBackends + NUM_AUXILIARY_PROCS + max_prepared_xacts)? Probably this is the dumbest thing as some slots are unused (NUM_AUXILIARY_PROCS - 1 + max_prepared_xacts slots. -1 because the startup process needs a ProcState slot) and the shared memory is wasted. Regards, Bharath Rupireddy.
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Kyotaro Horiguchi
Дата:
At Wed, 13 Oct 2021 13:39:24 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in > On Wed, Oct 13, 2021 at 4:25 AM Andres Freund <andres@anarazel.de> wrote: > > > > Hi, > > > > On 2021-10-11 15:24:46 +0900, Fujii Masao wrote: > > > How about modifying SharedInvalBackendInit() so that it accepts > > > BackendId as an argument and allocates the ProcState entry of > > > the specified BackendId? That is, the startup process determines > > > that its BackendId is "MaxBackends + MyAuxProcType (=StartupProcess) + 1" > > > in AuxiliaryProcessMain(), and then it passes that BackendId to > > > SharedInvalBackendInit() in InitRecoveryTransactionEnvironment(). > > > > If I understand correctly what you're proposing, I think that's going in the > > wrong direction. We should work towards getting rid of BackendIds > > instead. This whole complication vanishes if we make sinvaladt use pgprocno. > > > > See https://postgr.es/m/20210802171255.k4yv5cfqaqbuuy6f%40alap3.anarazel.de > > for some discussion of this. I feel this is the right direction. I understand sinvaladt needs the backend id but I think it's wrong that the backend id is used widely. And, in the current direction, procState array gets very sparse and performence of sinval gets degraded. > Will any of the backends get pgprocno greater than MaxBackends? The > pgprocno can range from 0 to ((MaxBackends + NUM_AUXILIARY_PROCS + > max_prepared_xacts) - 1) and the ProcState array size is MaxBackends. > How do we put a backend with pgprocno > MaxBackends, into the > ProcState array? Is it that we also increase ProcState array size to > (MaxBackends + NUM_AUXILIARY_PROCS + max_prepared_xacts)? Probably > this is the dumbest thing as some slots are unused > (NUM_AUXILIARY_PROCS - 1 + max_prepared_xacts slots. -1 because the > startup process needs a ProcState slot) and the shared memory is > wasted. The elements for max_prepared_xacts can be placed at the last part of pgprocno and we can actually omit allocating memory for them. I don't think NUM_AUXILIARY_PROC is that large. It's only 5 and won't increase much in future. Since we know that startup is the only user of procsig, we can save the space for four of them. In short, no extra memory is required by using pgprocno for now. The actual problem is if we simply use pgprocno in sinvaladt, we will see preformance degradation caused by the super sparse procState array. As Andres mentioned in the URL above, it can be avoided if we can get rid of looping over the array. Although needing a bit of care for the difference of invalid values for both though, BackendId can be easily replaced with pgprocno almost mechanically except sinvaladt. Therefore, we can confine the current backend ID within sinvaladt isolating from other part. The ids dedicated for sinvaladt can be packed to small range and perfomance won't be damaged. In the future, if we can get rid of looping over the procState array, sinvaladt - the last user of the current backend ID - can move to pgprocno and we will say good-bye to the current backend ID. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Kyotaro Horiguchi
Дата:
At Wed, 13 Oct 2021 19:52:52 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> Although needing a bit of care for the difference of invalid values
> for both though, BackendId can be easily replaced with pgprocno almost
> mechanically except sinvaladt. Therefore, we can confine the current
> backend ID within sinvaladt isolating from other part. The ids
> dedicated for sinvaladt can be packed to small range and perfomance
> won't be damaged.
Since I said it "mechanically doable", did I that. FWIW the attached is that. All behavioral differences come from the
difrenceof the valid range nad the invaild value between old BackendId and pgprpcno.
- Only sinvaladt uses the packed old backendid internally.
So procsignal can be sent to auxiliary processes.
- All other part uses pgprocno but it is named as backendid so as to
reduce the diff size.
- vxid's backendid part start from 0 not 1, and invalid backendid
becomes -1 to PG_INT32_MAX. Since it is mere a cosmetic issue, we
can replace PG_INT32_MAX as -1 on printing.
- The name of an exported snapshot changes. The first part start from
0, not 1.
- Prepared transactions' vxid is changed so that it's backendid part
has a valid value. Previously it was invalid id (-1). I'm not sure
it doesn't harm but I faced no trouble with make check-world.
(MarkAsPreparingGuts)
- With only 0002, backendid starts from 99 (when max_connection is
100) then decremented to 0, which is quite odd. 0001 reverses the
order of freelist.
> In the future, if we can get rid of looping over the procState array,
> sinvaladt - the last user of the current backend ID - can move to
> pgprocno and we will say good-bye to the current backend ID.
The attached files are named as *.txt so that bots don't recognize
them as a patch.
--
Kyotaro Horiguchi
NTT Open Source Software Center
From dd8187954eb72edc3fbe0a807ce531b438412030 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Wed, 13 Oct 2021 10:31:26 +0900
Subject: [PATCH 1/2] procfreelist in ascending order
---
src/backend/storage/lmgr/proc.c | 62 +++++++++++++++++++--------------
1 file changed, 36 insertions(+), 26 deletions(-)
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index b7d9da0aa9..78e05976a4 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -163,6 +163,9 @@ InitProcGlobal(void)
j;
bool found;
uint32 TotalProcs = MaxBackends + NUM_AUXILIARY_PROCS + max_prepared_xacts;
+ PGPROC **freelist;
+ int switchpoint;
+ int arraykind;
/* Create the ProcGlobal shared structure */
ProcGlobal = (PROC_HDR *)
@@ -214,6 +217,8 @@ InitProcGlobal(void)
ProcGlobal->statusFlags = (uint8 *) ShmemAlloc(TotalProcs * sizeof(*ProcGlobal->statusFlags));
MemSet(ProcGlobal->statusFlags, 0, TotalProcs * sizeof(*ProcGlobal->statusFlags));
+ switchpoint = 0;
+ arraykind = 0;
for (i = 0; i < TotalProcs; i++)
{
/* Common initialization for all PGPROCs, regardless of type. */
@@ -239,33 +244,38 @@ InitProcGlobal(void)
* linear search. PGPROCs for prepared transactions are added to a
* free list by TwoPhaseShmemInit().
*/
- if (i < MaxConnections)
+ if (i < MaxBackends)
{
- /* PGPROC for normal backend, add to freeProcs list */
- procs[i].links.next = (SHM_QUEUE *) ProcGlobal->freeProcs;
- ProcGlobal->freeProcs = &procs[i];
- procs[i].procgloballist = &ProcGlobal->freeProcs;
- }
- else if (i < MaxConnections + autovacuum_max_workers + 1)
- {
- /* PGPROC for AV launcher/worker, add to autovacFreeProcs list */
- procs[i].links.next = (SHM_QUEUE *) ProcGlobal->autovacFreeProcs;
- ProcGlobal->autovacFreeProcs = &procs[i];
- procs[i].procgloballist = &ProcGlobal->autovacFreeProcs;
- }
- else if (i < MaxConnections + autovacuum_max_workers + 1 + max_worker_processes)
- {
- /* PGPROC for bgworker, add to bgworkerFreeProcs list */
- procs[i].links.next = (SHM_QUEUE *) ProcGlobal->bgworkerFreeProcs;
- ProcGlobal->bgworkerFreeProcs = &procs[i];
- procs[i].procgloballist = &ProcGlobal->bgworkerFreeProcs;
- }
- else if (i < MaxBackends)
- {
- /* PGPROC for walsender, add to walsenderFreeProcs list */
- procs[i].links.next = (SHM_QUEUE *) ProcGlobal->walsenderFreeProcs;
- ProcGlobal->walsenderFreeProcs = &procs[i];
- procs[i].procgloballist = &ProcGlobal->walsenderFreeProcs;
+ if (i == switchpoint)
+ {
+ switch (arraykind++)
+ {
+ case 0:
+ freelist = &ProcGlobal->freeProcs;
+ switchpoint += MaxConnections;
+ break;
+
+ case 1:
+ freelist = &ProcGlobal->autovacFreeProcs;
+ switchpoint += autovacuum_max_workers + 1;
+ break;
+
+ case 2:
+ freelist = &ProcGlobal->bgworkerFreeProcs;
+ switchpoint += max_worker_processes;
+ break;
+
+ case 3:
+ freelist = &ProcGlobal->walsenderFreeProcs;
+ }
+
+ /* link the element to the just-switched freelist */
+ *freelist = &procs[i];
+ }
+ else
+ procs[i - 1].links.next = (SHM_QUEUE *) &procs[i];
+
+ procs[i].procgloballist = freelist;
}
/* Initialize myProcLocks[] shared memory queues. */
--
2.27.0
From 24d58b74691c5011fd26a9d430fe79e6f66079ea Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 14 Oct 2021 17:05:24 +0900
Subject: [PATCH 2/2] Remove BackendId
BackendId was generated by sinvaladt.c and widely used as ids packed
in narrow range. However, the characteristics of narrow-packed is not
required other than sinvaladt.c and the use of the id in other places
rather harm. Get rid of the backend id in most of the tree and use
pgprocno as new BackendId.
However, sinvaladt still needs such packed id so the old backend id is
confined to the module.
---
src/backend/access/transam/multixact.c | 23 +++++-----
src/backend/access/transam/twophase.c | 4 +-
src/backend/access/transam/xact.c | 2 +-
src/backend/catalog/namespace.c | 2 +-
src/backend/commands/async.c | 21 +++++----
src/backend/commands/indexcmds.c | 3 +-
src/backend/postmaster/auxprocess.c | 2 +-
src/backend/postmaster/pgstat.c | 2 +-
src/backend/storage/ipc/procarray.c | 9 ++--
src/backend/storage/ipc/procsignal.c | 25 ++++-------
src/backend/storage/ipc/sinvaladt.c | 49 ++++-----------------
src/backend/storage/ipc/standby.c | 2 +-
src/backend/storage/lmgr/lmgr.c | 3 +-
src/backend/storage/lmgr/lock.c | 20 ++++-----
src/backend/storage/lmgr/proc.c | 29 +++++++++---
src/backend/utils/activity/backend_status.c | 22 +--------
src/backend/utils/adt/dbsize.c | 5 ++-
src/backend/utils/adt/mcxtfuncs.c | 2 +-
src/backend/utils/cache/relcache.c | 1 +
src/backend/utils/error/elog.c | 10 ++---
src/backend/utils/init/globals.c | 2 -
src/backend/utils/init/postinit.c | 7 +--
src/backend/utils/time/snapmgr.c | 4 +-
src/include/storage/backendid.h | 3 +-
src/include/storage/lock.h | 6 +--
src/include/storage/proc.h | 11 +++--
src/include/storage/procsignal.h | 4 +-
src/include/storage/smgr.h | 2 +-
src/include/utils/rel.h | 2 +-
29 files changed, 117 insertions(+), 160 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index e6c70ed0bc..585dc50858 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -238,9 +238,9 @@ typedef struct MultiXactStateData
* immediately following the MultiXactStateData struct. Each is indexed by
* BackendId.
*
- * In both arrays, there's a slot for all normal backends (1..MaxBackends)
- * followed by a slot for max_prepared_xacts prepared transactions. Valid
- * BackendIds start from 1; element zero of each array is never used.
+ * In both arrays, there's a slot for all normal backends
+ * (0..MaxBackends-1) followed by a slot for max_prepared_xacts prepared
+ * transactions.
*
* OldestMemberMXactId[k] is the oldest MultiXactId each backend's current
* transaction(s) could possibly be a member of, or InvalidMultiXactId
@@ -283,9 +283,9 @@ typedef struct MultiXactStateData
/*
* Last element of OldestMemberMXactId and OldestVisibleMXactId arrays.
- * Valid elements are (1..MaxOldestSlot); element 0 is never used.
+ * Valid elements are (0..MaxOldestSlot).
*/
-#define MaxOldestSlot (MaxBackends + max_prepared_xacts)
+#define MaxOldestSlot (MaxBackends + max_prepared_xacts - 1)
/* Pointers to the state data in shared memory */
static MultiXactStateData *MultiXactState;
@@ -697,7 +697,7 @@ MultiXactIdSetOldestVisible(void)
if (oldestMXact < FirstMultiXactId)
oldestMXact = FirstMultiXactId;
- for (i = 1; i <= MaxOldestSlot; i++)
+ for (i = 0 ; i <= MaxOldestSlot; i++)
{
MultiXactId thisoldest = OldestMemberMXactId[i];
@@ -1828,10 +1828,10 @@ MultiXactShmemSize(void)
{
Size size;
- /* We need 2*MaxOldestSlot + 1 perBackendXactIds[] entries */
+ /* We need 2*(MaxOldestSlot + 1) + 1 perBackendXactIds[] entries */
#define SHARED_MULTIXACT_STATE_SIZE \
add_size(offsetof(MultiXactStateData, perBackendXactIds) + sizeof(MultiXactId), \
- mul_size(sizeof(MultiXactId) * 2, MaxOldestSlot))
+ mul_size(sizeof(MultiXactId) * 2, MaxOldestSlot + 1))
size = SHARED_MULTIXACT_STATE_SIZE;
size = add_size(size, SimpleLruShmemSize(NUM_MULTIXACTOFFSET_BUFFERS, 0));
@@ -1878,11 +1878,10 @@ MultiXactShmemInit(void)
Assert(found);
/*
- * Set up array pointers. Note that perBackendXactIds[0] is wasted space
- * since we only use indexes 1..MaxOldestSlot in each array.
+ * Set up array pointers.
*/
OldestMemberMXactId = MultiXactState->perBackendXactIds;
- OldestVisibleMXactId = OldestMemberMXactId + MaxOldestSlot;
+ OldestVisibleMXactId = OldestMemberMXactId + MaxOldestSlot + 1;
}
/*
@@ -2525,7 +2524,7 @@ GetOldestMultiXactId(void)
nextMXact = FirstMultiXactId;
oldestMXact = nextMXact;
- for (i = 1; i <= MaxOldestSlot; i++)
+ for (i = 0; i <= MaxOldestSlot; i++)
{
MultiXactId thisoldest;
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 2156de187c..76d5bb55d6 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -293,7 +293,7 @@ TwoPhaseShmemInit(void)
* prepared transaction. Currently multixact.c uses that
* technique.
*/
- gxacts[i].dummyBackendId = MaxBackends + 1 + i;
+ gxacts[i].dummyBackendId = MaxBackends + i;
}
}
else
@@ -459,14 +459,12 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
proc->pgprocno = gxact->pgprocno;
SHMQueueElemInit(&(proc->links));
proc->waitStatus = PROC_WAIT_STATUS_OK;
- /* We set up the gxact's VXID as InvalidBackendId/XID */
proc->lxid = (LocalTransactionId) xid;
proc->xid = xid;
Assert(proc->xmin == InvalidTransactionId);
proc->delayChkpt = false;
proc->statusFlags = 0;
proc->pid = 0;
- proc->backendId = InvalidBackendId;
proc->databaseId = databaseid;
proc->roleId = owner;
proc->tempNamespaceId = InvalidOid;
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 4cc38f0d85..9a8b0686bd 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2020,7 +2020,7 @@ StartTransaction(void)
* Advertise it in the proc array. We assume assignment of
* localTransactionId is atomic, and the backendId should be set already.
*/
- Assert(MyProc->backendId == vxid.backendId);
+ Assert(MyProc->pgprocno == vxid.backendId);
MyProc->lxid = vxid.localTransactionId;
TRACE_POSTGRESQL_TRANSACTION_START(vxid.localTransactionId);
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 4de8400fd0..f450c46f4d 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -3293,7 +3293,7 @@ checkTempNamespaceStatus(Oid namespaceId)
return TEMP_NAMESPACE_NOT_TEMP;
/* Is the backend alive? */
- proc = BackendIdGetProc(backendId);
+ proc = GetProcIfAlive(backendId);
if (proc == NULL)
return TEMP_NAMESPACE_IDLE;
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 8557008545..e007c17f51 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -272,8 +272,8 @@ typedef struct QueueBackendStatus
* NotifyQueueTailLock, then NotifyQueueLock, and lastly NotifySLRULock.
*
* Each backend uses the backend[] array entry with index equal to its
- * BackendId (which can range from 1 to MaxBackends). We rely on this to make
- * SendProcSignal fast.
+ * BackendId (which can range from 0 to MaxBackends - 1). We rely on this to
+ * make SendProcSignal fast.
*
* The backend[] array entries for actively-listening backends are threaded
* together using firstListener and the nextListener links, so that we can
@@ -1122,7 +1122,8 @@ Exec_ListenPreCommit(void)
head = QUEUE_HEAD;
max = QUEUE_TAIL;
prevListener = InvalidBackendId;
- for (BackendId i = QUEUE_FIRST_LISTENER; i > 0; i = QUEUE_NEXT_LISTENER(i))
+ for (BackendId i = QUEUE_FIRST_LISTENER; i != InvalidBackendId ;
+ i = QUEUE_NEXT_LISTENER(i))
{
if (QUEUE_BACKEND_DBOID(i) == MyDatabaseId)
max = QUEUE_POS_MAX(max, QUEUE_BACKEND_POS(i));
@@ -1134,7 +1135,7 @@ Exec_ListenPreCommit(void)
QUEUE_BACKEND_PID(MyBackendId) = MyProcPid;
QUEUE_BACKEND_DBOID(MyBackendId) = MyDatabaseId;
/* Insert backend into list of listeners at correct position */
- if (prevListener > 0)
+ if (prevListener != InvalidBackendId)
{
QUEUE_NEXT_LISTENER(MyBackendId) = QUEUE_NEXT_LISTENER(prevListener);
QUEUE_NEXT_LISTENER(prevListener) = MyBackendId;
@@ -1281,7 +1282,8 @@ asyncQueueUnregister(void)
QUEUE_FIRST_LISTENER = QUEUE_NEXT_LISTENER(MyBackendId);
else
{
- for (BackendId i = QUEUE_FIRST_LISTENER; i > 0; i = QUEUE_NEXT_LISTENER(i))
+ for (BackendId i = QUEUE_FIRST_LISTENER; i != InvalidBackendId ;
+ i = QUEUE_NEXT_LISTENER(i))
{
if (QUEUE_NEXT_LISTENER(i) == MyBackendId)
{
@@ -1590,7 +1592,8 @@ asyncQueueFillWarning(void)
QueuePosition min = QUEUE_HEAD;
int32 minPid = InvalidPid;
- for (BackendId i = QUEUE_FIRST_LISTENER; i > 0; i = QUEUE_NEXT_LISTENER(i))
+ for (BackendId i = QUEUE_FIRST_LISTENER; i != InvalidBackendId;
+ i = QUEUE_NEXT_LISTENER(i))
{
Assert(QUEUE_BACKEND_PID(i) != InvalidPid);
min = QUEUE_POS_MIN(min, QUEUE_BACKEND_POS(i));
@@ -1646,7 +1649,8 @@ SignalBackends(void)
count = 0;
LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE);
- for (BackendId i = QUEUE_FIRST_LISTENER; i > 0; i = QUEUE_NEXT_LISTENER(i))
+ for (BackendId i = QUEUE_FIRST_LISTENER; i != InvalidBackendId ;
+ i = QUEUE_NEXT_LISTENER(i))
{
int32 pid = QUEUE_BACKEND_PID(i);
QueuePosition pos;
@@ -2183,7 +2187,8 @@ asyncQueueAdvanceTail(void)
*/
LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE);
min = QUEUE_HEAD;
- for (BackendId i = QUEUE_FIRST_LISTENER; i > 0; i = QUEUE_NEXT_LISTENER(i))
+ for (int i = QUEUE_FIRST_LISTENER; i != InvalidBackendId ;
+ i = QUEUE_NEXT_LISTENER(i))
{
Assert(QUEUE_BACKEND_PID(i) != InvalidPid);
min = QUEUE_POS_MIN(min, QUEUE_BACKEND_POS(i));
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index c14ca27c5e..fa70eed559 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -463,7 +463,8 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
/* If requested, publish who we're going to wait for. */
if (progress)
{
- PGPROC *holder = BackendIdGetProc(old_snapshots[i].backendId);
+ PGPROC *holder =
+ &ProcGlobal->allProcs[old_snapshots[i].backendId];
if (holder)
pgstat_progress_update_param(PROGRESS_WAITFOR_CURRENT_PID,
diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index 7452f908b2..4c7784b036 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -116,7 +116,7 @@ AuxiliaryProcessMain(AuxProcType auxtype)
* This will need rethinking if we ever want more than one of a particular
* auxiliary process type.
*/
- ProcSignalInit(MaxBackends + MyAuxProcType + 1);
+ ProcSignalInit();
/*
* Auxiliary processes don't run transactions, but they may need a
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index b7d0fbaefd..b1cd23f661 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -54,7 +54,7 @@
#include "postmaster/postmaster.h"
#include "replication/slot.h"
#include "replication/walsender.h"
-#include "storage/backendid.h"
+//#include "storage/backendid.h"
#include "storage/dsm.h"
#include "storage/fd.h"
#include "storage/ipc.h"
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index bd3c7a47fe..100c0dae8c 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -2592,7 +2592,7 @@ ProcArrayInstallImportedXmin(TransactionId xmin,
continue;
/* We are only interested in the specific virtual transaction. */
- if (proc->backendId != sourcevxid->backendId)
+ if (proc->pgprocno != sourcevxid->backendId)
continue;
if (proc->lxid != sourcevxid->localTransactionId)
continue;
@@ -3454,7 +3454,7 @@ SignalVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode,
* Kill the pid if it's still here. If not, that's what we
* wanted so ignore any errors.
*/
- (void) SendProcSignal(pid, sigmode, vxid.backendId);
+ (void) SendProcSignal(pid, sigmode, proc->pgprocno);
}
break;
}
@@ -3604,11 +3604,8 @@ CancelDBBackends(Oid databaseid, ProcSignalReason sigmode, bool conflictPending)
if (databaseid == InvalidOid || proc->databaseId == databaseid)
{
- VirtualTransactionId procvxid;
pid_t pid;
- GET_VXID_FROM_PGPROC(procvxid, *proc);
-
proc->recoveryConflictPending = conflictPending;
pid = proc->pid;
if (pid != 0)
@@ -3617,7 +3614,7 @@ CancelDBBackends(Oid databaseid, ProcSignalReason sigmode, bool conflictPending)
* Kill the pid if it's still here. If not, that's what we
* wanted so ignore any errors.
*/
- (void) SendProcSignal(pid, sigmode, procvxid.backendId);
+ (void) SendProcSignal(pid, sigmode, proc->pgprocno);
}
}
}
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index defb75aa26..ffe6939780 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -81,9 +81,8 @@ typedef struct
} ProcSignalHeader;
/*
- * We reserve a slot for each possible BackendId, plus one for each
- * possible auxiliary process type. (This scheme assumes there is not
- * more than one of any auxiliary process type at a time.)
+ * We reserve a slot for PGPROCs for backends and auxiliary processes. Not all
+ * of auxiliary processes use this but allocate for them for safety.
*/
#define NumProcSignalSlots (MaxBackends + NUM_AUXPROCTYPES)
@@ -153,24 +152,19 @@ ProcSignalShmemInit(void)
/*
* ProcSignalInit
* Register the current process in the procsignal array
- *
- * The passed index should be my BackendId if the process has one,
- * or MaxBackends + aux process type if not.
*/
void
-ProcSignalInit(int pss_idx)
+ProcSignalInit(void)
{
ProcSignalSlot *slot;
uint64 barrier_generation;
- Assert(pss_idx >= 1 && pss_idx <= NumProcSignalSlots);
-
- slot = &ProcSignal->psh_slot[pss_idx - 1];
+ slot = &ProcSignal->psh_slot[MyBackendId];
/* sanity check */
if (slot->pss_pid != 0)
elog(LOG, "process %d taking over ProcSignal slot %d, but it's not empty",
- MyProcPid, pss_idx);
+ MyProcPid, MyBackendId);
/* Clear out any leftover signal reasons */
MemSet(slot->pss_signalFlags, 0, NUM_PROCSIGNALS * sizeof(sig_atomic_t));
@@ -199,7 +193,7 @@ ProcSignalInit(int pss_idx)
MyProcSignalSlot = slot;
/* Set up to release the slot on process exit */
- on_shmem_exit(CleanupProcSignalState, Int32GetDatum(pss_idx));
+ on_shmem_exit(CleanupProcSignalState, (Datum) 0);
}
/*
@@ -211,10 +205,9 @@ ProcSignalInit(int pss_idx)
static void
CleanupProcSignalState(int status, Datum arg)
{
- int pss_idx = DatumGetInt32(arg);
ProcSignalSlot *slot;
- slot = &ProcSignal->psh_slot[pss_idx - 1];
+ slot = &ProcSignal->psh_slot[MyBackendId];
Assert(slot == MyProcSignalSlot);
/*
@@ -232,7 +225,7 @@ CleanupProcSignalState(int status, Datum arg)
* infinite loop trying to exit
*/
elog(LOG, "process %d releasing ProcSignal slot %d, but it contains %d",
- MyProcPid, pss_idx, (int) slot->pss_pid);
+ MyProcPid, MyProcPid, (int) slot->pss_pid);
return; /* XXX better to zero the slot anyway? */
}
@@ -264,7 +257,7 @@ SendProcSignal(pid_t pid, ProcSignalReason reason, BackendId backendId)
if (backendId != InvalidBackendId)
{
- slot = &ProcSignal->psh_slot[backendId - 1];
+ slot = &ProcSignal->psh_slot[backendId];
/*
* Note: Since there's no locking, it's possible that the target
diff --git a/src/backend/storage/ipc/sinvaladt.c b/src/backend/storage/ipc/sinvaladt.c
index 946bd8e3cb..e5cfba5f96 100644
--- a/src/backend/storage/ipc/sinvaladt.c
+++ b/src/backend/storage/ipc/sinvaladt.c
@@ -19,7 +19,6 @@
#include "access/transam.h"
#include "miscadmin.h"
-#include "storage/backendid.h"
#include "storage/ipc.h"
#include "storage/proc.h"
#include "storage/procsignal.h"
@@ -157,7 +156,7 @@ typedef struct ProcState
/*
* Next LocalTransactionId to use for each idle backend slot. We keep
* this here because it is indexed by BackendId and it is convenient to
- * copy the value to and from local memory when MyBackendId is set. It's
+ * copy the value to and from local memory when MybackendId is set. It's
* meaningless in an active ProcState entry.
*/
LocalTransactionId nextLXID;
@@ -195,6 +194,7 @@ static LocalTransactionId nextLocalTransactionId;
static void CleanupInvalidationState(int status, Datum arg);
+static int siindex;
/*
* SInvalShmemSize --- return shared-memory space needed
@@ -290,7 +290,7 @@ SharedInvalBackendInit(bool sendOnly)
/*
* out of procState slots: MaxBackends exceeded -- report normally
*/
- MyBackendId = InvalidBackendId;
+ siindex = -1;
LWLockRelease(SInvalWriteLock);
ereport(FATAL,
(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
@@ -298,10 +298,7 @@ SharedInvalBackendInit(bool sendOnly)
}
}
- MyBackendId = (stateP - &segP->procState[0]) + 1;
-
- /* Advertise assigned backend ID in MyProc */
- MyProc->backendId = MyBackendId;
+ siindex = (stateP - &segP->procState[0]);
/* Fetch next local transaction ID into local memory */
nextLocalTransactionId = stateP->nextLXID;
@@ -320,7 +317,7 @@ SharedInvalBackendInit(bool sendOnly)
/* register exit routine to mark my entry inactive at exit */
on_shmem_exit(CleanupInvalidationState, PointerGetDatum(segP));
- elog(DEBUG4, "my backend ID is %d", MyBackendId);
+ elog(DEBUG4, "my SI slot index is %d", siindex);
}
/*
@@ -342,7 +339,7 @@ CleanupInvalidationState(int status, Datum arg)
LWLockAcquire(SInvalWriteLock, LW_EXCLUSIVE);
- stateP = &segP->procState[MyBackendId - 1];
+ stateP = &segP->procState[siindex];
/* Update next local transaction ID for next holder of this backendID */
stateP->nextLXID = nextLocalTransactionId;
@@ -365,34 +362,6 @@ CleanupInvalidationState(int status, Datum arg)
LWLockRelease(SInvalWriteLock);
}
-/*
- * BackendIdGetProc
- * Get the PGPROC structure for a backend, given the backend ID.
- * The result may be out of date arbitrarily quickly, so the caller
- * must be careful about how this information is used. NULL is
- * returned if the backend is not active.
- */
-PGPROC *
-BackendIdGetProc(int backendID)
-{
- PGPROC *result = NULL;
- SISeg *segP = shmInvalBuffer;
-
- /* Need to lock out additions/removals of backends */
- LWLockAcquire(SInvalWriteLock, LW_SHARED);
-
- if (backendID > 0 && backendID <= segP->lastBackend)
- {
- ProcState *stateP = &segP->procState[backendID - 1];
-
- result = stateP->proc;
- }
-
- LWLockRelease(SInvalWriteLock);
-
- return result;
-}
-
/*
* BackendIdGetTransactionIds
* Get the xid and xmin of the backend. The result may be out of date
@@ -541,7 +510,7 @@ SIGetDataEntries(SharedInvalidationMessage *data, int datasize)
int n;
segP = shmInvalBuffer;
- stateP = &segP->procState[MyBackendId - 1];
+ stateP = &segP->procState[siindex];
/*
* Before starting to take locks, do a quick, unlocked test to see whether
@@ -730,13 +699,13 @@ SICleanupQueue(bool callerHasWriteLock, int minFree)
if (needSig)
{
pid_t his_pid = needSig->procPid;
- BackendId his_backendId = (needSig - &segP->procState[0]) + 1;
+ int his_pgprocno = needSig->proc->pgprocno;
needSig->signaled = true;
LWLockRelease(SInvalReadLock);
LWLockRelease(SInvalWriteLock);
elog(DEBUG4, "sending sinval catchup signal to PID %d", (int) his_pid);
- SendProcSignal(his_pid, PROCSIG_CATCHUP_INTERRUPT, his_backendId);
+ SendProcSignal(his_pid, PROCSIG_CATCHUP_INTERRUPT, his_pgprocno);
if (callerHasWriteLock)
LWLockAcquire(SInvalWriteLock, LW_EXCLUSIVE);
}
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index b17326bc20..032f0428aa 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -274,7 +274,7 @@ LogRecoveryConflict(ProcSignalReason reason, TimestampTz wait_start,
vxids = wait_list;
while (VirtualTransactionIdIsValid(*vxids))
{
- PGPROC *proc = BackendIdGetProc(vxids->backendId);
+ PGPROC *proc = &ProcGlobal->allProcs[vxids->backendId];
/* proc can be NULL if the target backend is not active */
if (proc)
diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c
index cdf2266d6d..f0208531e0 100644
--- a/src/backend/storage/lmgr/lmgr.c
+++ b/src/backend/storage/lmgr/lmgr.c
@@ -918,7 +918,8 @@ WaitForLockersMultiple(List *locktags, LOCKMODE lockmode, bool progress)
/* If requested, publish who we're going to wait for. */
if (progress)
{
- PGPROC *holder = BackendIdGetProc(lockholders->backendId);
+ PGPROC *holder =
+ &ProcGlobal->allProcs[lockholders->backendId];
if (holder)
pgstat_progress_update_param(PROGRESS_WAITFOR_CURRENT_PID,
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 364654e106..3edb7d6fd5 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -3695,7 +3695,7 @@ GetLockStatusData(void)
proc->fpRelId[f]);
instance->holdMask = lockbits << FAST_PATH_LOCKNUMBER_OFFSET;
instance->waitLockMode = NoLock;
- instance->backend = proc->backendId;
+ instance->backend = proc->pgprocno;
instance->lxid = proc->lxid;
instance->pid = proc->pid;
instance->leaderPid = proc->pid;
@@ -3722,14 +3722,14 @@ GetLockStatusData(void)
repalloc(data->locks, sizeof(LockInstanceData) * els);
}
- vxid.backendId = proc->backendId;
+ vxid.backendId = proc->pgprocno;
vxid.localTransactionId = proc->fpLocalTransactionId;
instance = &data->locks[el];
SET_LOCKTAG_VIRTUALTRANSACTION(instance->locktag, vxid);
instance->holdMask = LOCKBIT_ON(ExclusiveLock);
instance->waitLockMode = NoLock;
- instance->backend = proc->backendId;
+ instance->backend = proc->pgprocno;
instance->lxid = proc->lxid;
instance->pid = proc->pid;
instance->leaderPid = proc->pid;
@@ -3782,7 +3782,7 @@ GetLockStatusData(void)
instance->waitLockMode = proc->waitLockMode;
else
instance->waitLockMode = NoLock;
- instance->backend = proc->backendId;
+ instance->backend = proc->pgprocno;
instance->lxid = proc->lxid;
instance->pid = proc->pid;
instance->leaderPid = proclock->groupLeader->pid;
@@ -3961,7 +3961,7 @@ GetSingleProcBlockerStatusData(PGPROC *blocked_proc, BlockedProcsData *data)
instance->waitLockMode = proc->waitLockMode;
else
instance->waitLockMode = NoLock;
- instance->backend = proc->backendId;
+ instance->backend = proc->pgprocno;
instance->lxid = proc->lxid;
instance->pid = proc->pid;
instance->leaderPid = proclock->groupLeader->pid;
@@ -4475,7 +4475,7 @@ VirtualXactLockTableInsert(VirtualTransactionId vxid)
LWLockAcquire(&MyProc->fpInfoLock, LW_EXCLUSIVE);
- Assert(MyProc->backendId == vxid.backendId);
+ Assert(MyProc->pgprocno == vxid.backendId);
Assert(MyProc->fpLocalTransactionId == InvalidLocalTransactionId);
Assert(MyProc->fpVXIDLock == false);
@@ -4497,8 +4497,6 @@ VirtualXactLockTableCleanup(void)
bool fastpath;
LocalTransactionId lxid;
- Assert(MyProc->backendId != InvalidBackendId);
-
/*
* Clean up shared memory state.
*/
@@ -4520,7 +4518,7 @@ VirtualXactLockTableCleanup(void)
VirtualTransactionId vxid;
LOCKTAG locktag;
- vxid.backendId = MyBackendId;
+ vxid.backendId = MyProc->pgprocno;
vxid.localTransactionId = lxid;
SET_LOCKTAG_VIRTUALTRANSACTION(locktag, vxid);
@@ -4571,7 +4569,7 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
* relevant lxid is no longer running here, that's enough to prove that
* it's no longer running anywhere.
*/
- proc = BackendIdGetProc(vxid.backendId);
+ proc = &ProcGlobal->allProcs[vxid.backendId];
if (proc == NULL)
return true;
@@ -4583,7 +4581,7 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
LWLockAcquire(&proc->fpInfoLock, LW_EXCLUSIVE);
/* If the transaction has ended, our work here is done. */
- if (proc->backendId != vxid.backendId
+ if (proc->pgprocno != vxid.backendId
|| proc->fpLocalTransactionId != vxid.localTransactionId)
{
LWLockRelease(&proc->fpInfoLock);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 78e05976a4..0ee6627e20 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -386,6 +386,8 @@ InitProcess(void)
if (IsUnderPostmaster && !IsAutoVacuumLauncherProcess())
MarkPostmasterChildActive();
+ MyBackendId = MyProc->pgprocno;
+
/*
* Initialize all fields of MyProc, except for those previously
* initialized by InitProcGlobal.
@@ -398,14 +400,13 @@ InitProcess(void)
MyProc->xid = InvalidTransactionId;
MyProc->xmin = InvalidTransactionId;
MyProc->pid = MyProcPid;
- /* backendId, databaseId and roleId will be filled in later */
- MyProc->backendId = InvalidBackendId;
+ /* databaseId and roleId will be filled in later */
MyProc->databaseId = InvalidOid;
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
MyProc->delayChkpt = false;
- MyProc->statusFlags = 0;
+ MyProc->statusFlags = PROC_IS_ACTIVE;
/* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */
if (IsAutoVacuumWorkerProcess())
MyProc->statusFlags |= PROC_IS_AUTOVACUUM;
@@ -570,6 +571,7 @@ InitAuxiliaryProcess(void)
((volatile PGPROC *) auxproc)->pid = MyProcPid;
MyProc = auxproc;
+ MyBackendId = MyProc->pgprocno;
SpinLockRelease(ProcStructLock);
@@ -584,13 +586,12 @@ InitAuxiliaryProcess(void)
MyProc->fpLocalTransactionId = InvalidLocalTransactionId;
MyProc->xid = InvalidTransactionId;
MyProc->xmin = InvalidTransactionId;
- MyProc->backendId = InvalidBackendId;
MyProc->databaseId = InvalidOid;
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
MyProc->delayChkpt = false;
- MyProc->statusFlags = 0;
+ MyProc->statusFlags = PROC_IS_ACTIVE;
MyProc->lwWaiting = false;
MyProc->lwWaitMode = 0;
MyProc->waitLock = NULL;
@@ -913,6 +914,9 @@ ProcKill(int code, Datum arg)
MyProc = NULL;
DisownLatch(&proc->procLatch);
+ /* mark this process dead */
+ proc->statusFlags &= ~PROC_IS_ACTIVE;
+
procgloballist = proc->procgloballist;
SpinLockAcquire(ProcStructLock);
@@ -985,6 +989,7 @@ AuxiliaryProcKill(int code, Datum arg)
/* Mark auxiliary proc no longer in use */
proc->pid = 0;
+ proc->statusFlags &= ~PROC_IS_ACTIVE;
/* Update shared estimate of spins_per_delay */
ProcGlobal->spins_per_delay = update_spins_per_delay(ProcGlobal->spins_per_delay);
@@ -2020,3 +2025,17 @@ BecomeLockGroupMember(PGPROC *leader, int pid)
return ok;
}
+
+/*
+ * Return PGPROC if it is alive.
+ */
+PGPROC *
+GetProcIfAlive(BackendId backend)
+{
+ PGPROC *proc = &ProcGlobal->allProcs[backend];
+
+ if (proc->statusFlags & PROC_IS_ACTIVE)
+ return proc;
+
+ return NULL;
+}
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index 7229598822..0490a3a8b2 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -249,26 +249,8 @@ void
pgstat_beinit(void)
{
/* Initialize MyBEEntry */
- if (MyBackendId != InvalidBackendId)
- {
- Assert(MyBackendId >= 1 && MyBackendId <= MaxBackends);
- MyBEEntry = &BackendStatusArray[MyBackendId - 1];
- }
- else
- {
- /* Must be an auxiliary process */
- Assert(MyAuxProcType != NotAnAuxProcess);
-
- /*
- * Assign the MyBEEntry for an auxiliary process. Since it doesn't
- * have a BackendId, the slot is statically allocated based on the
- * auxiliary process type (MyAuxProcType). Backends use slots indexed
- * in the range from 1 to MaxBackends (inclusive), so we use
- * MaxBackends + AuxBackendType + 1 as the index of the slot for an
- * auxiliary process.
- */
- MyBEEntry = &BackendStatusArray[MaxBackends + MyAuxProcType];
- }
+ Assert(MyBackendId >= 0 && MyBackendId < NumBackendStatSlots);
+ MyBEEntry = &BackendStatusArray[MyBackendId];
/* Set up a process-exit hook to clean up */
on_shmem_exit(pgstat_beshutdown_hook, 0);
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index d5a7fb13f3..16acb0c335 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -22,6 +22,7 @@
#include "commands/dbcommands.h"
#include "commands/tablespace.h"
#include "miscadmin.h"
+#include "storage/backendid.h"
#include "storage/fd.h"
#include "utils/acl.h"
#include "utils/builtins.h"
@@ -292,7 +293,7 @@ pg_tablespace_size_name(PG_FUNCTION_ARGS)
* is no check here or at the call sites for that.
*/
static int64
-calculate_relation_size(RelFileNode *rfn, BackendId backend, ForkNumber forknum)
+calculate_relation_size(RelFileNode *rfn, int backend, ForkNumber forknum)
{
int64 totalsize = 0;
char *relationpath;
@@ -925,7 +926,7 @@ pg_relation_filepath(PG_FUNCTION_ARGS)
HeapTuple tuple;
Form_pg_class relform;
RelFileNode rnode;
- BackendId backend;
+ int backend;
char *path;
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 0d52613bc3..fca87448cf 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -205,7 +205,7 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(false);
}
- if (SendProcSignal(pid, PROCSIG_LOG_MEMORY_CONTEXT, proc->backendId) < 0)
+ if (SendProcSignal(pid, PROCSIG_LOG_MEMORY_CONTEXT, proc->pgprocno) < 0)
{
/* Again, just a warning to allow loops */
ereport(WARNING,
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 13d9994af3..00036ec9c0 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -73,6 +73,7 @@
#include "optimizer/optimizer.h"
#include "rewrite/rewriteDefine.h"
#include "rewrite/rowsecurity.h"
+#include "storage/backendid.h"
#include "storage/lmgr.h"
#include "storage/smgr.h"
#include "utils/array.h"
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index f33729513a..0f927751d7 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2685,18 +2685,18 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
break;
case 'v':
/* keep VXID format in sync with lockfuncs.c */
- if (MyProc != NULL && MyProc->backendId != InvalidBackendId)
+ if (MyProc != NULL && MyProc->pgprocno != InvalidBackendId)
{
if (padding != 0)
{
char strfbuf[128];
snprintf(strfbuf, sizeof(strfbuf) - 1, "%d/%u",
- MyProc->backendId, MyProc->lxid);
+ MyProc->pgprocno, MyProc->lxid);
appendStringInfo(buf, "%*s", padding, strfbuf);
}
else
- appendStringInfo(buf, "%d/%u", MyProc->backendId, MyProc->lxid);
+ appendStringInfo(buf, "%d/%u", MyProc->pgprocno, MyProc->lxid);
}
else if (padding != 0)
appendStringInfoSpaces(buf,
@@ -2860,8 +2860,8 @@ write_csvlog(ErrorData *edata)
/* Virtual transaction id */
/* keep VXID format in sync with lockfuncs.c */
- if (MyProc != NULL && MyProc->backendId != InvalidBackendId)
- appendStringInfo(&buf, "%d/%u", MyProc->backendId, MyProc->lxid);
+ if (MyProc != NULL && MyProc->pgprocno != InvalidBackendId)
+ appendStringInfo(&buf, "%d/%u", MyProc->pgprocno, MyProc->lxid);
appendStringInfoChar(&buf, ',');
/* Transaction id */
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 381d9e548d..4ba1914472 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -81,8 +81,6 @@ char postgres_exec_path[MAXPGPATH]; /* full path to backend */
/* note: currently this is not valid in backend processes */
#endif
-BackendId MyBackendId = InvalidBackendId;
-
BackendId ParallelLeaderBackendId = InvalidBackendId;
Oid MyDatabaseId = InvalidOid;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 78bc64671e..7f5b8e12ee 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -592,15 +592,10 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
*
* Sets up MyBackendId, a unique backend identifier.
*/
- MyBackendId = InvalidBackendId;
-
SharedInvalBackendInit(false);
- if (MyBackendId > MaxBackends || MyBackendId <= 0)
- elog(FATAL, "bad backend ID: %d", MyBackendId);
-
/* Now that we have a BackendId, we can participate in ProcSignal */
- ProcSignalInit(MyBackendId);
+ ProcSignalInit();
/*
* Also set up timeout handlers needed for backend operation. We need
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 5001efdf7a..df8476d177 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1173,7 +1173,7 @@ ExportSnapshot(Snapshot snapshot)
* inside the transaction from 1.
*/
snprintf(path, sizeof(path), SNAPSHOT_EXPORT_DIR "/%08X-%08X-%d",
- MyProc->backendId, MyProc->lxid, list_length(exportedSnapshots) + 1);
+ MyProc->pgprocno, MyProc->lxid, list_length(exportedSnapshots) + 1);
/*
* Copy the snapshot into TopTransactionContext, add it to the
@@ -1200,7 +1200,7 @@ ExportSnapshot(Snapshot snapshot)
*/
initStringInfo(&buf);
- appendStringInfo(&buf, "vxid:%d/%u\n", MyProc->backendId, MyProc->lxid);
+ appendStringInfo(&buf, "vxid:%d/%u\n", MyProc->pgprocno, MyProc->lxid);
appendStringInfo(&buf, "pid:%d\n", MyProcPid);
appendStringInfo(&buf, "dbid:%u\n", MyDatabaseId);
appendStringInfo(&buf, "iso:%d\n", XactIsoLevel);
diff --git a/src/include/storage/backendid.h b/src/include/storage/backendid.h
index 7aa3936899..3772e2b4a2 100644
--- a/src/include/storage/backendid.h
+++ b/src/include/storage/backendid.h
@@ -20,7 +20,8 @@
*/
typedef int BackendId; /* unique currently active backend identifier */
-#define InvalidBackendId (-1)
+#define INVALID_PGPROCNO PG_INT32_MAX
+#define InvalidBackendId INVALID_PGPROCNO
extern PGDLLIMPORT BackendId MyBackendId; /* backend id of this backend */
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index 9b2a421c32..13b9352704 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -62,7 +62,7 @@ extern bool Debug_deadlocks;
*/
typedef struct
{
- BackendId backendId; /* backendId from PGPROC */
+ BackendId backendId; /* pgprocno from PGPROC */
LocalTransactionId localTransactionId; /* lxid from PGPROC */
} VirtualTransactionId;
@@ -79,7 +79,7 @@ typedef struct
((vxid).backendId = InvalidBackendId, \
(vxid).localTransactionId = InvalidLocalTransactionId)
#define GET_VXID_FROM_PGPROC(vxid, proc) \
- ((vxid).backendId = (proc).backendId, \
+ ((vxid).backendId = (proc).pgprocno, \
(vxid).localTransactionId = (proc).lxid)
/* MAX_LOCKMODES cannot be larger than the # of bits in LOCKMASK */
@@ -445,7 +445,7 @@ typedef struct LockInstanceData
LOCKTAG locktag; /* tag for locked object */
LOCKMASK holdMask; /* locks held by this PGPROC */
LOCKMODE waitLockMode; /* lock awaited by this PGPROC, if any */
- BackendId backend; /* backend ID of this PGPROC */
+ BackendId backend; /* pgprocno of this PGPROC */
LocalTransactionId lxid; /* local transaction ID of this PGPROC */
TimestampTz waitStart; /* time at which this PGPROC started waiting
* for lock */
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index be67d8a861..2f3d9cfb17 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -17,6 +17,7 @@
#include "access/clog.h"
#include "access/xlogdefs.h"
#include "lib/ilist.h"
+#include "storage/backendid.h"
#include "storage/latch.h"
#include "storage/lock.h"
#include "storage/pg_sema.h"
@@ -60,6 +61,7 @@ struct XidCache
#define PROC_VACUUM_FOR_WRAPAROUND 0x08 /* set by autovac only */
#define PROC_IN_LOGICAL_DECODING 0x10 /* currently doing logical
* decoding outside xact */
+#define PROC_IS_ACTIVE 0x20 /* This process is active */
/* flags reset at EOXact */
#define PROC_VACUUM_STATE_MASK \
@@ -73,11 +75,7 @@ struct XidCache
*/
#define FP_LOCK_SLOTS_PER_BACKEND 16
-/*
- * An invalid pgprocno. Must be larger than the maximum number of PGPROC
- * structures we could possibly have. See comments for MAX_BACKENDS.
- */
-#define INVALID_PGPROCNO PG_INT32_MAX
+BackendId MyBackendId;
typedef enum
{
@@ -150,7 +148,6 @@ struct PGPROC
int pgprocno;
/* These fields are zero while a backend is still starting up: */
- BackendId backendId; /* This backend's backend ID (if assigned) */
Oid databaseId; /* OID of database this backend is using */
Oid roleId; /* OID of role using this backend */
@@ -418,4 +415,6 @@ extern PGPROC *AuxiliaryPidGetProc(int pid);
extern void BecomeLockGroupLeader(void);
extern bool BecomeLockGroupMember(PGPROC *leader, int pid);
+extern PGPROC *GetProcIfAlive(BackendId backend);
+
#endif /* _PROC_H_ */
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index eec186be2e..34a7fff271 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -63,9 +63,9 @@ typedef enum
extern Size ProcSignalShmemSize(void);
extern void ProcSignalShmemInit(void);
-extern void ProcSignalInit(int pss_idx);
+extern void ProcSignalInit(void);
extern int SendProcSignal(pid_t pid, ProcSignalReason reason,
- BackendId backendId);
+ int pgprocno);
extern uint64 EmitProcSignalBarrier(ProcSignalBarrierType type);
extern void WaitForProcSignalBarrier(uint64 generation);
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index a6fbf7b6a6..7c1063f9f9 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -78,7 +78,7 @@ typedef SMgrRelationData *SMgrRelation;
RelFileNodeBackendIsTemp((smgr)->smgr_rnode)
extern void smgrinit(void);
-extern SMgrRelation smgropen(RelFileNode rnode, BackendId backend);
+extern SMgrRelation smgropen(RelFileNode rnode, int backend);
extern bool smgrexists(SMgrRelation reln, ForkNumber forknum);
extern void smgrsetowner(SMgrRelation *owner, SMgrRelation reln);
extern void smgrclearowner(SMgrRelation *owner, SMgrRelation reln);
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index b4faa1c123..1ef0571201 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -56,7 +56,7 @@ typedef struct RelationData
RelFileNode rd_node; /* relation physical identifier */
SMgrRelation rd_smgr; /* cached file handle, or NULL */
int rd_refcnt; /* reference count */
- BackendId rd_backend; /* owning backend id, if temporary relation */
+ int rd_backend; /* owning backend id, if temporary relation */
bool rd_islocaltemp; /* rel is a temp rel of this session */
bool rd_isnailed; /* rel is nailed in cache */
bool rd_isvalid; /* relcache entry is valid */
--
2.27.0
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Kyotaro Horiguchi
Дата:
At Thu, 14 Oct 2021 10:53:06 -0700, Andres Freund <andres@anarazel.de> wrote in > Hi, > > On 2021-10-14 17:28:34 +0900, Kyotaro Horiguchi wrote: > > At Wed, 13 Oct 2021 19:52:52 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > > > Although needing a bit of care for the difference of invalid values > > > for both though, BackendId can be easily replaced with pgprocno almost > > > mechanically except sinvaladt. Therefore, we can confine the current > > > backend ID within sinvaladt isolating from other part. The ids > > > dedicated for sinvaladt can be packed to small range and perfomance > > > won't be damaged. > > FWIW, I don't actually think there's necessarily that strong a need for > density in sinvaladt. With a few relatively changes we can get rid of the O(n) > work in the most crucial paths. Right. So I left it for the "future:p > In https://www.postgresql.org/message-id/20210802171255.k4yv5cfqaqbuuy6f%40alap3.anarazel.de > I wrote: > > Another approach to deal with this could be to simply not do the O(n) work in > > SIInsertDataEntries(). It's not obvious that ->hasMessages is actually > > necessary - we could atomically read maxMsgNum without acquiring a lock > > instead of needing the per-backend ->hasMessages. I don't the density would > > be a relevant factor in SICleanupQueue(). > > This'd get rid of the need of density *and* make SIInsertDataEntries() > cheaper. Yes. So.. I tried that. The only part where memory-flush timing is crucial seems to be between writing messages and setting maxMsgNum. By placing memory barrier between them it seems *to me* we can read maxMsgNum safely without locks. I reread that thread and found we can get rid of O(N) behavior from two places, SIgnalVirtualTransaction and GetVirtualXIDsDelayingChkpt. Finally, I got rid of the siindex (the old BackendId) from sinvaladt.c at all. Still CleanupInvalidationState and SICleanupQueue has O(N) behavior but they are executed rarely or ends in a short time in the most cases. 0001: Reverses the proc freelist so that the backendid is assigned in the sane order. 0002: Replaces the current BackendId - that is generated by sinvaladt.c intending to pack the ids to a narrow range - with pgprocno in most of the tree. The old BackendID is now used only in sinvaladt.c 0003: Removes O(N) behavior from SIInsertDataEntries. I'm not sure it is correctly revised, though.. 0004: Gets rid of O(N), or reduce O(N^2) to O(N) of HaveVirtualXIDsDelayingChkpt and SignalVirtualTransaction. 0005: Gets rid of the old BackendID at all from sinvaladt.c. regards. -- Kyotaro Horiguchi NTT Open Source Software Center From 2b28c29d68eb2c37137aef66c7634b65aa640520 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Date: Wed, 13 Oct 2021 10:31:26 +0900 Subject: [PATCH v2 1/5] procfreelist in ascending order --- src/backend/storage/lmgr/proc.c | 62 +++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index b7d9da0aa9..78e05976a4 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -163,6 +163,9 @@ InitProcGlobal(void) j; bool found; uint32 TotalProcs = MaxBackends + NUM_AUXILIARY_PROCS + max_prepared_xacts; + PGPROC **freelist; + int switchpoint; + int arraykind; /* Create the ProcGlobal shared structure */ ProcGlobal = (PROC_HDR *) @@ -214,6 +217,8 @@ InitProcGlobal(void) ProcGlobal->statusFlags = (uint8 *) ShmemAlloc(TotalProcs * sizeof(*ProcGlobal->statusFlags)); MemSet(ProcGlobal->statusFlags, 0, TotalProcs * sizeof(*ProcGlobal->statusFlags)); + switchpoint = 0; + arraykind = 0; for (i = 0; i < TotalProcs; i++) { /* Common initialization for all PGPROCs, regardless of type. */ @@ -239,33 +244,38 @@ InitProcGlobal(void) * linear search. PGPROCs for prepared transactions are added to a * free list by TwoPhaseShmemInit(). */ - if (i < MaxConnections) + if (i < MaxBackends) { - /* PGPROC for normal backend, add to freeProcs list */ - procs[i].links.next = (SHM_QUEUE *) ProcGlobal->freeProcs; - ProcGlobal->freeProcs = &procs[i]; - procs[i].procgloballist = &ProcGlobal->freeProcs; - } - else if (i < MaxConnections + autovacuum_max_workers + 1) - { - /* PGPROC for AV launcher/worker, add to autovacFreeProcs list */ - procs[i].links.next = (SHM_QUEUE *) ProcGlobal->autovacFreeProcs; - ProcGlobal->autovacFreeProcs = &procs[i]; - procs[i].procgloballist = &ProcGlobal->autovacFreeProcs; - } - else if (i < MaxConnections + autovacuum_max_workers + 1 + max_worker_processes) - { - /* PGPROC for bgworker, add to bgworkerFreeProcs list */ - procs[i].links.next = (SHM_QUEUE *) ProcGlobal->bgworkerFreeProcs; - ProcGlobal->bgworkerFreeProcs = &procs[i]; - procs[i].procgloballist = &ProcGlobal->bgworkerFreeProcs; - } - else if (i < MaxBackends) - { - /* PGPROC for walsender, add to walsenderFreeProcs list */ - procs[i].links.next = (SHM_QUEUE *) ProcGlobal->walsenderFreeProcs; - ProcGlobal->walsenderFreeProcs = &procs[i]; - procs[i].procgloballist = &ProcGlobal->walsenderFreeProcs; + if (i == switchpoint) + { + switch (arraykind++) + { + case 0: + freelist = &ProcGlobal->freeProcs; + switchpoint += MaxConnections; + break; + + case 1: + freelist = &ProcGlobal->autovacFreeProcs; + switchpoint += autovacuum_max_workers + 1; + break; + + case 2: + freelist = &ProcGlobal->bgworkerFreeProcs; + switchpoint += max_worker_processes; + break; + + case 3: + freelist = &ProcGlobal->walsenderFreeProcs; + } + + /* link the element to the just-switched freelist */ + *freelist = &procs[i]; + } + else + procs[i - 1].links.next = (SHM_QUEUE *) &procs[i]; + + procs[i].procgloballist = freelist; } /* Initialize myProcLocks[] shared memory queues. */ -- 2.27.0 From d7cc2b6b95dd90683b47e5692b2acbc370223ada Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Date: Thu, 14 Oct 2021 17:05:24 +0900 Subject: [PATCH v2 2/5] Remove BackendId BackendId was generated by sinvaladt.c and widely used as ids packed in narrow range. However, the characteristics of narrow-packed is not required other than sinvaladt.c and the use of the id in other places rather harm. Get rid of the backend id in most of the tree and use pgprocno as new BackendId. However, sinvaladt still needs such packed id so the old backend id is confined to the module. --- src/backend/access/transam/multixact.c | 23 +++++----- src/backend/access/transam/twophase.c | 4 +- src/backend/access/transam/xact.c | 2 +- src/backend/catalog/namespace.c | 2 +- src/backend/commands/async.c | 21 +++++---- src/backend/commands/indexcmds.c | 3 +- src/backend/postmaster/auxprocess.c | 2 +- src/backend/postmaster/pgstat.c | 2 +- src/backend/storage/ipc/procarray.c | 9 ++-- src/backend/storage/ipc/procsignal.c | 25 ++++------- src/backend/storage/ipc/sinvaladt.c | 47 ++++----------------- src/backend/storage/ipc/standby.c | 2 +- src/backend/storage/lmgr/lmgr.c | 3 +- src/backend/storage/lmgr/lock.c | 20 ++++----- src/backend/storage/lmgr/proc.c | 29 ++++++++++--- src/backend/utils/activity/backend_status.c | 22 +--------- src/backend/utils/adt/dbsize.c | 5 ++- src/backend/utils/adt/mcxtfuncs.c | 2 +- src/backend/utils/cache/relcache.c | 1 + src/backend/utils/error/elog.c | 10 ++--- src/backend/utils/init/globals.c | 2 - src/backend/utils/init/postinit.c | 7 +-- src/backend/utils/time/snapmgr.c | 4 +- src/include/storage/backendid.h | 3 +- src/include/storage/lock.h | 6 +-- src/include/storage/proc.h | 11 +++-- src/include/storage/procsignal.h | 4 +- src/include/storage/smgr.h | 2 +- src/include/utils/rel.h | 2 +- 29 files changed, 116 insertions(+), 159 deletions(-) diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index e6c70ed0bc..585dc50858 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -238,9 +238,9 @@ typedef struct MultiXactStateData * immediately following the MultiXactStateData struct. Each is indexed by * BackendId. * - * In both arrays, there's a slot for all normal backends (1..MaxBackends) - * followed by a slot for max_prepared_xacts prepared transactions. Valid - * BackendIds start from 1; element zero of each array is never used. + * In both arrays, there's a slot for all normal backends + * (0..MaxBackends-1) followed by a slot for max_prepared_xacts prepared + * transactions. * * OldestMemberMXactId[k] is the oldest MultiXactId each backend's current * transaction(s) could possibly be a member of, or InvalidMultiXactId @@ -283,9 +283,9 @@ typedef struct MultiXactStateData /* * Last element of OldestMemberMXactId and OldestVisibleMXactId arrays. - * Valid elements are (1..MaxOldestSlot); element 0 is never used. + * Valid elements are (0..MaxOldestSlot). */ -#define MaxOldestSlot (MaxBackends + max_prepared_xacts) +#define MaxOldestSlot (MaxBackends + max_prepared_xacts - 1) /* Pointers to the state data in shared memory */ static MultiXactStateData *MultiXactState; @@ -697,7 +697,7 @@ MultiXactIdSetOldestVisible(void) if (oldestMXact < FirstMultiXactId) oldestMXact = FirstMultiXactId; - for (i = 1; i <= MaxOldestSlot; i++) + for (i = 0 ; i <= MaxOldestSlot; i++) { MultiXactId thisoldest = OldestMemberMXactId[i]; @@ -1828,10 +1828,10 @@ MultiXactShmemSize(void) { Size size; - /* We need 2*MaxOldestSlot + 1 perBackendXactIds[] entries */ + /* We need 2*(MaxOldestSlot + 1) + 1 perBackendXactIds[] entries */ #define SHARED_MULTIXACT_STATE_SIZE \ add_size(offsetof(MultiXactStateData, perBackendXactIds) + sizeof(MultiXactId), \ - mul_size(sizeof(MultiXactId) * 2, MaxOldestSlot)) + mul_size(sizeof(MultiXactId) * 2, MaxOldestSlot + 1)) size = SHARED_MULTIXACT_STATE_SIZE; size = add_size(size, SimpleLruShmemSize(NUM_MULTIXACTOFFSET_BUFFERS, 0)); @@ -1878,11 +1878,10 @@ MultiXactShmemInit(void) Assert(found); /* - * Set up array pointers. Note that perBackendXactIds[0] is wasted space - * since we only use indexes 1..MaxOldestSlot in each array. + * Set up array pointers. */ OldestMemberMXactId = MultiXactState->perBackendXactIds; - OldestVisibleMXactId = OldestMemberMXactId + MaxOldestSlot; + OldestVisibleMXactId = OldestMemberMXactId + MaxOldestSlot + 1; } /* @@ -2525,7 +2524,7 @@ GetOldestMultiXactId(void) nextMXact = FirstMultiXactId; oldestMXact = nextMXact; - for (i = 1; i <= MaxOldestSlot; i++) + for (i = 0; i <= MaxOldestSlot; i++) { MultiXactId thisoldest; diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index 2156de187c..76d5bb55d6 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -293,7 +293,7 @@ TwoPhaseShmemInit(void) * prepared transaction. Currently multixact.c uses that * technique. */ - gxacts[i].dummyBackendId = MaxBackends + 1 + i; + gxacts[i].dummyBackendId = MaxBackends + i; } } else @@ -459,14 +459,12 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid, proc->pgprocno = gxact->pgprocno; SHMQueueElemInit(&(proc->links)); proc->waitStatus = PROC_WAIT_STATUS_OK; - /* We set up the gxact's VXID as InvalidBackendId/XID */ proc->lxid = (LocalTransactionId) xid; proc->xid = xid; Assert(proc->xmin == InvalidTransactionId); proc->delayChkpt = false; proc->statusFlags = 0; proc->pid = 0; - proc->backendId = InvalidBackendId; proc->databaseId = databaseid; proc->roleId = owner; proc->tempNamespaceId = InvalidOid; diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 4cc38f0d85..9a8b0686bd 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -2020,7 +2020,7 @@ StartTransaction(void) * Advertise it in the proc array. We assume assignment of * localTransactionId is atomic, and the backendId should be set already. */ - Assert(MyProc->backendId == vxid.backendId); + Assert(MyProc->pgprocno == vxid.backendId); MyProc->lxid = vxid.localTransactionId; TRACE_POSTGRESQL_TRANSACTION_START(vxid.localTransactionId); diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c index 4de8400fd0..f450c46f4d 100644 --- a/src/backend/catalog/namespace.c +++ b/src/backend/catalog/namespace.c @@ -3293,7 +3293,7 @@ checkTempNamespaceStatus(Oid namespaceId) return TEMP_NAMESPACE_NOT_TEMP; /* Is the backend alive? */ - proc = BackendIdGetProc(backendId); + proc = GetProcIfAlive(backendId); if (proc == NULL) return TEMP_NAMESPACE_IDLE; diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c index 8557008545..e007c17f51 100644 --- a/src/backend/commands/async.c +++ b/src/backend/commands/async.c @@ -272,8 +272,8 @@ typedef struct QueueBackendStatus * NotifyQueueTailLock, then NotifyQueueLock, and lastly NotifySLRULock. * * Each backend uses the backend[] array entry with index equal to its - * BackendId (which can range from 1 to MaxBackends). We rely on this to make - * SendProcSignal fast. + * BackendId (which can range from 0 to MaxBackends - 1). We rely on this to + * make SendProcSignal fast. * * The backend[] array entries for actively-listening backends are threaded * together using firstListener and the nextListener links, so that we can @@ -1122,7 +1122,8 @@ Exec_ListenPreCommit(void) head = QUEUE_HEAD; max = QUEUE_TAIL; prevListener = InvalidBackendId; - for (BackendId i = QUEUE_FIRST_LISTENER; i > 0; i = QUEUE_NEXT_LISTENER(i)) + for (BackendId i = QUEUE_FIRST_LISTENER; i != InvalidBackendId ; + i = QUEUE_NEXT_LISTENER(i)) { if (QUEUE_BACKEND_DBOID(i) == MyDatabaseId) max = QUEUE_POS_MAX(max, QUEUE_BACKEND_POS(i)); @@ -1134,7 +1135,7 @@ Exec_ListenPreCommit(void) QUEUE_BACKEND_PID(MyBackendId) = MyProcPid; QUEUE_BACKEND_DBOID(MyBackendId) = MyDatabaseId; /* Insert backend into list of listeners at correct position */ - if (prevListener > 0) + if (prevListener != InvalidBackendId) { QUEUE_NEXT_LISTENER(MyBackendId) = QUEUE_NEXT_LISTENER(prevListener); QUEUE_NEXT_LISTENER(prevListener) = MyBackendId; @@ -1281,7 +1282,8 @@ asyncQueueUnregister(void) QUEUE_FIRST_LISTENER = QUEUE_NEXT_LISTENER(MyBackendId); else { - for (BackendId i = QUEUE_FIRST_LISTENER; i > 0; i = QUEUE_NEXT_LISTENER(i)) + for (BackendId i = QUEUE_FIRST_LISTENER; i != InvalidBackendId ; + i = QUEUE_NEXT_LISTENER(i)) { if (QUEUE_NEXT_LISTENER(i) == MyBackendId) { @@ -1590,7 +1592,8 @@ asyncQueueFillWarning(void) QueuePosition min = QUEUE_HEAD; int32 minPid = InvalidPid; - for (BackendId i = QUEUE_FIRST_LISTENER; i > 0; i = QUEUE_NEXT_LISTENER(i)) + for (BackendId i = QUEUE_FIRST_LISTENER; i != InvalidBackendId; + i = QUEUE_NEXT_LISTENER(i)) { Assert(QUEUE_BACKEND_PID(i) != InvalidPid); min = QUEUE_POS_MIN(min, QUEUE_BACKEND_POS(i)); @@ -1646,7 +1649,8 @@ SignalBackends(void) count = 0; LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE); - for (BackendId i = QUEUE_FIRST_LISTENER; i > 0; i = QUEUE_NEXT_LISTENER(i)) + for (BackendId i = QUEUE_FIRST_LISTENER; i != InvalidBackendId ; + i = QUEUE_NEXT_LISTENER(i)) { int32 pid = QUEUE_BACKEND_PID(i); QueuePosition pos; @@ -2183,7 +2187,8 @@ asyncQueueAdvanceTail(void) */ LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE); min = QUEUE_HEAD; - for (BackendId i = QUEUE_FIRST_LISTENER; i > 0; i = QUEUE_NEXT_LISTENER(i)) + for (int i = QUEUE_FIRST_LISTENER; i != InvalidBackendId ; + i = QUEUE_NEXT_LISTENER(i)) { Assert(QUEUE_BACKEND_PID(i) != InvalidPid); min = QUEUE_POS_MIN(min, QUEUE_BACKEND_POS(i)); diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index c14ca27c5e..fa70eed559 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -463,7 +463,8 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress) /* If requested, publish who we're going to wait for. */ if (progress) { - PGPROC *holder = BackendIdGetProc(old_snapshots[i].backendId); + PGPROC *holder = + &ProcGlobal->allProcs[old_snapshots[i].backendId]; if (holder) pgstat_progress_update_param(PROGRESS_WAITFOR_CURRENT_PID, diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c index 7452f908b2..4c7784b036 100644 --- a/src/backend/postmaster/auxprocess.c +++ b/src/backend/postmaster/auxprocess.c @@ -116,7 +116,7 @@ AuxiliaryProcessMain(AuxProcType auxtype) * This will need rethinking if we ever want more than one of a particular * auxiliary process type. */ - ProcSignalInit(MaxBackends + MyAuxProcType + 1); + ProcSignalInit(); /* * Auxiliary processes don't run transactions, but they may need a diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index b7d0fbaefd..b1cd23f661 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -54,7 +54,7 @@ #include "postmaster/postmaster.h" #include "replication/slot.h" #include "replication/walsender.h" -#include "storage/backendid.h" +//#include "storage/backendid.h" #include "storage/dsm.h" #include "storage/fd.h" #include "storage/ipc.h" diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index bd3c7a47fe..100c0dae8c 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -2592,7 +2592,7 @@ ProcArrayInstallImportedXmin(TransactionId xmin, continue; /* We are only interested in the specific virtual transaction. */ - if (proc->backendId != sourcevxid->backendId) + if (proc->pgprocno != sourcevxid->backendId) continue; if (proc->lxid != sourcevxid->localTransactionId) continue; @@ -3454,7 +3454,7 @@ SignalVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode, * Kill the pid if it's still here. If not, that's what we * wanted so ignore any errors. */ - (void) SendProcSignal(pid, sigmode, vxid.backendId); + (void) SendProcSignal(pid, sigmode, proc->pgprocno); } break; } @@ -3604,11 +3604,8 @@ CancelDBBackends(Oid databaseid, ProcSignalReason sigmode, bool conflictPending) if (databaseid == InvalidOid || proc->databaseId == databaseid) { - VirtualTransactionId procvxid; pid_t pid; - GET_VXID_FROM_PGPROC(procvxid, *proc); - proc->recoveryConflictPending = conflictPending; pid = proc->pid; if (pid != 0) @@ -3617,7 +3614,7 @@ CancelDBBackends(Oid databaseid, ProcSignalReason sigmode, bool conflictPending) * Kill the pid if it's still here. If not, that's what we * wanted so ignore any errors. */ - (void) SendProcSignal(pid, sigmode, procvxid.backendId); + (void) SendProcSignal(pid, sigmode, proc->pgprocno); } } } diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c index defb75aa26..ffe6939780 100644 --- a/src/backend/storage/ipc/procsignal.c +++ b/src/backend/storage/ipc/procsignal.c @@ -81,9 +81,8 @@ typedef struct } ProcSignalHeader; /* - * We reserve a slot for each possible BackendId, plus one for each - * possible auxiliary process type. (This scheme assumes there is not - * more than one of any auxiliary process type at a time.) + * We reserve a slot for PGPROCs for backends and auxiliary processes. Not all + * of auxiliary processes use this but allocate for them for safety. */ #define NumProcSignalSlots (MaxBackends + NUM_AUXPROCTYPES) @@ -153,24 +152,19 @@ ProcSignalShmemInit(void) /* * ProcSignalInit * Register the current process in the procsignal array - * - * The passed index should be my BackendId if the process has one, - * or MaxBackends + aux process type if not. */ void -ProcSignalInit(int pss_idx) +ProcSignalInit(void) { ProcSignalSlot *slot; uint64 barrier_generation; - Assert(pss_idx >= 1 && pss_idx <= NumProcSignalSlots); - - slot = &ProcSignal->psh_slot[pss_idx - 1]; + slot = &ProcSignal->psh_slot[MyBackendId]; /* sanity check */ if (slot->pss_pid != 0) elog(LOG, "process %d taking over ProcSignal slot %d, but it's not empty", - MyProcPid, pss_idx); + MyProcPid, MyBackendId); /* Clear out any leftover signal reasons */ MemSet(slot->pss_signalFlags, 0, NUM_PROCSIGNALS * sizeof(sig_atomic_t)); @@ -199,7 +193,7 @@ ProcSignalInit(int pss_idx) MyProcSignalSlot = slot; /* Set up to release the slot on process exit */ - on_shmem_exit(CleanupProcSignalState, Int32GetDatum(pss_idx)); + on_shmem_exit(CleanupProcSignalState, (Datum) 0); } /* @@ -211,10 +205,9 @@ ProcSignalInit(int pss_idx) static void CleanupProcSignalState(int status, Datum arg) { - int pss_idx = DatumGetInt32(arg); ProcSignalSlot *slot; - slot = &ProcSignal->psh_slot[pss_idx - 1]; + slot = &ProcSignal->psh_slot[MyBackendId]; Assert(slot == MyProcSignalSlot); /* @@ -232,7 +225,7 @@ CleanupProcSignalState(int status, Datum arg) * infinite loop trying to exit */ elog(LOG, "process %d releasing ProcSignal slot %d, but it contains %d", - MyProcPid, pss_idx, (int) slot->pss_pid); + MyProcPid, MyProcPid, (int) slot->pss_pid); return; /* XXX better to zero the slot anyway? */ } @@ -264,7 +257,7 @@ SendProcSignal(pid_t pid, ProcSignalReason reason, BackendId backendId) if (backendId != InvalidBackendId) { - slot = &ProcSignal->psh_slot[backendId - 1]; + slot = &ProcSignal->psh_slot[backendId]; /* * Note: Since there's no locking, it's possible that the target diff --git a/src/backend/storage/ipc/sinvaladt.c b/src/backend/storage/ipc/sinvaladt.c index 946bd8e3cb..a90e9920e7 100644 --- a/src/backend/storage/ipc/sinvaladt.c +++ b/src/backend/storage/ipc/sinvaladt.c @@ -19,7 +19,6 @@ #include "access/transam.h" #include "miscadmin.h" -#include "storage/backendid.h" #include "storage/ipc.h" #include "storage/proc.h" #include "storage/procsignal.h" @@ -157,7 +156,7 @@ typedef struct ProcState /* * Next LocalTransactionId to use for each idle backend slot. We keep * this here because it is indexed by BackendId and it is convenient to - * copy the value to and from local memory when MyBackendId is set. It's + * copy the value to and from local memory when MybackendId is set. It's * meaningless in an active ProcState entry. */ LocalTransactionId nextLXID; @@ -195,6 +194,7 @@ static LocalTransactionId nextLocalTransactionId; static void CleanupInvalidationState(int status, Datum arg); +static int siindex; /* * SInvalShmemSize --- return shared-memory space needed @@ -290,7 +290,7 @@ SharedInvalBackendInit(bool sendOnly) /* * out of procState slots: MaxBackends exceeded -- report normally */ - MyBackendId = InvalidBackendId; + siindex = -1; LWLockRelease(SInvalWriteLock); ereport(FATAL, (errcode(ERRCODE_TOO_MANY_CONNECTIONS), @@ -298,10 +298,7 @@ SharedInvalBackendInit(bool sendOnly) } } - MyBackendId = (stateP - &segP->procState[0]) + 1; - - /* Advertise assigned backend ID in MyProc */ - MyProc->backendId = MyBackendId; + siindex = (stateP - &segP->procState[0]); /* Fetch next local transaction ID into local memory */ nextLocalTransactionId = stateP->nextLXID; @@ -320,7 +317,7 @@ SharedInvalBackendInit(bool sendOnly) /* register exit routine to mark my entry inactive at exit */ on_shmem_exit(CleanupInvalidationState, PointerGetDatum(segP)); - elog(DEBUG4, "my backend ID is %d", MyBackendId); + elog(DEBUG4, "my SI slot index is %d", siindex); } /* @@ -342,7 +339,7 @@ CleanupInvalidationState(int status, Datum arg) LWLockAcquire(SInvalWriteLock, LW_EXCLUSIVE); - stateP = &segP->procState[MyBackendId - 1]; + stateP = &segP->procState[siindex]; /* Update next local transaction ID for next holder of this backendID */ stateP->nextLXID = nextLocalTransactionId; @@ -365,34 +362,6 @@ CleanupInvalidationState(int status, Datum arg) LWLockRelease(SInvalWriteLock); } -/* - * BackendIdGetProc - * Get the PGPROC structure for a backend, given the backend ID. - * The result may be out of date arbitrarily quickly, so the caller - * must be careful about how this information is used. NULL is - * returned if the backend is not active. - */ -PGPROC * -BackendIdGetProc(int backendID) -{ - PGPROC *result = NULL; - SISeg *segP = shmInvalBuffer; - - /* Need to lock out additions/removals of backends */ - LWLockAcquire(SInvalWriteLock, LW_SHARED); - - if (backendID > 0 && backendID <= segP->lastBackend) - { - ProcState *stateP = &segP->procState[backendID - 1]; - - result = stateP->proc; - } - - LWLockRelease(SInvalWriteLock); - - return result; -} - /* * BackendIdGetTransactionIds * Get the xid and xmin of the backend. The result may be out of date @@ -541,7 +510,7 @@ SIGetDataEntries(SharedInvalidationMessage *data, int datasize) int n; segP = shmInvalBuffer; - stateP = &segP->procState[MyBackendId - 1]; + stateP = &segP->procState[siindex]; /* * Before starting to take locks, do a quick, unlocked test to see whether @@ -730,7 +699,7 @@ SICleanupQueue(bool callerHasWriteLock, int minFree) if (needSig) { pid_t his_pid = needSig->procPid; - BackendId his_backendId = (needSig - &segP->procState[0]) + 1; + int his_backendId = needSig->proc->pgprocno; needSig->signaled = true; LWLockRelease(SInvalReadLock); diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c index b17326bc20..032f0428aa 100644 --- a/src/backend/storage/ipc/standby.c +++ b/src/backend/storage/ipc/standby.c @@ -274,7 +274,7 @@ LogRecoveryConflict(ProcSignalReason reason, TimestampTz wait_start, vxids = wait_list; while (VirtualTransactionIdIsValid(*vxids)) { - PGPROC *proc = BackendIdGetProc(vxids->backendId); + PGPROC *proc = &ProcGlobal->allProcs[vxids->backendId]; /* proc can be NULL if the target backend is not active */ if (proc) diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c index cdf2266d6d..f0208531e0 100644 --- a/src/backend/storage/lmgr/lmgr.c +++ b/src/backend/storage/lmgr/lmgr.c @@ -918,7 +918,8 @@ WaitForLockersMultiple(List *locktags, LOCKMODE lockmode, bool progress) /* If requested, publish who we're going to wait for. */ if (progress) { - PGPROC *holder = BackendIdGetProc(lockholders->backendId); + PGPROC *holder = + &ProcGlobal->allProcs[lockholders->backendId]; if (holder) pgstat_progress_update_param(PROGRESS_WAITFOR_CURRENT_PID, diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index 364654e106..3edb7d6fd5 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -3695,7 +3695,7 @@ GetLockStatusData(void) proc->fpRelId[f]); instance->holdMask = lockbits << FAST_PATH_LOCKNUMBER_OFFSET; instance->waitLockMode = NoLock; - instance->backend = proc->backendId; + instance->backend = proc->pgprocno; instance->lxid = proc->lxid; instance->pid = proc->pid; instance->leaderPid = proc->pid; @@ -3722,14 +3722,14 @@ GetLockStatusData(void) repalloc(data->locks, sizeof(LockInstanceData) * els); } - vxid.backendId = proc->backendId; + vxid.backendId = proc->pgprocno; vxid.localTransactionId = proc->fpLocalTransactionId; instance = &data->locks[el]; SET_LOCKTAG_VIRTUALTRANSACTION(instance->locktag, vxid); instance->holdMask = LOCKBIT_ON(ExclusiveLock); instance->waitLockMode = NoLock; - instance->backend = proc->backendId; + instance->backend = proc->pgprocno; instance->lxid = proc->lxid; instance->pid = proc->pid; instance->leaderPid = proc->pid; @@ -3782,7 +3782,7 @@ GetLockStatusData(void) instance->waitLockMode = proc->waitLockMode; else instance->waitLockMode = NoLock; - instance->backend = proc->backendId; + instance->backend = proc->pgprocno; instance->lxid = proc->lxid; instance->pid = proc->pid; instance->leaderPid = proclock->groupLeader->pid; @@ -3961,7 +3961,7 @@ GetSingleProcBlockerStatusData(PGPROC *blocked_proc, BlockedProcsData *data) instance->waitLockMode = proc->waitLockMode; else instance->waitLockMode = NoLock; - instance->backend = proc->backendId; + instance->backend = proc->pgprocno; instance->lxid = proc->lxid; instance->pid = proc->pid; instance->leaderPid = proclock->groupLeader->pid; @@ -4475,7 +4475,7 @@ VirtualXactLockTableInsert(VirtualTransactionId vxid) LWLockAcquire(&MyProc->fpInfoLock, LW_EXCLUSIVE); - Assert(MyProc->backendId == vxid.backendId); + Assert(MyProc->pgprocno == vxid.backendId); Assert(MyProc->fpLocalTransactionId == InvalidLocalTransactionId); Assert(MyProc->fpVXIDLock == false); @@ -4497,8 +4497,6 @@ VirtualXactLockTableCleanup(void) bool fastpath; LocalTransactionId lxid; - Assert(MyProc->backendId != InvalidBackendId); - /* * Clean up shared memory state. */ @@ -4520,7 +4518,7 @@ VirtualXactLockTableCleanup(void) VirtualTransactionId vxid; LOCKTAG locktag; - vxid.backendId = MyBackendId; + vxid.backendId = MyProc->pgprocno; vxid.localTransactionId = lxid; SET_LOCKTAG_VIRTUALTRANSACTION(locktag, vxid); @@ -4571,7 +4569,7 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait) * relevant lxid is no longer running here, that's enough to prove that * it's no longer running anywhere. */ - proc = BackendIdGetProc(vxid.backendId); + proc = &ProcGlobal->allProcs[vxid.backendId]; if (proc == NULL) return true; @@ -4583,7 +4581,7 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait) LWLockAcquire(&proc->fpInfoLock, LW_EXCLUSIVE); /* If the transaction has ended, our work here is done. */ - if (proc->backendId != vxid.backendId + if (proc->pgprocno != vxid.backendId || proc->fpLocalTransactionId != vxid.localTransactionId) { LWLockRelease(&proc->fpInfoLock); diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 78e05976a4..0ee6627e20 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -386,6 +386,8 @@ InitProcess(void) if (IsUnderPostmaster && !IsAutoVacuumLauncherProcess()) MarkPostmasterChildActive(); + MyBackendId = MyProc->pgprocno; + /* * Initialize all fields of MyProc, except for those previously * initialized by InitProcGlobal. @@ -398,14 +400,13 @@ InitProcess(void) MyProc->xid = InvalidTransactionId; MyProc->xmin = InvalidTransactionId; MyProc->pid = MyProcPid; - /* backendId, databaseId and roleId will be filled in later */ - MyProc->backendId = InvalidBackendId; + /* databaseId and roleId will be filled in later */ MyProc->databaseId = InvalidOid; MyProc->roleId = InvalidOid; MyProc->tempNamespaceId = InvalidOid; MyProc->isBackgroundWorker = IsBackgroundWorker; MyProc->delayChkpt = false; - MyProc->statusFlags = 0; + MyProc->statusFlags = PROC_IS_ACTIVE; /* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */ if (IsAutoVacuumWorkerProcess()) MyProc->statusFlags |= PROC_IS_AUTOVACUUM; @@ -570,6 +571,7 @@ InitAuxiliaryProcess(void) ((volatile PGPROC *) auxproc)->pid = MyProcPid; MyProc = auxproc; + MyBackendId = MyProc->pgprocno; SpinLockRelease(ProcStructLock); @@ -584,13 +586,12 @@ InitAuxiliaryProcess(void) MyProc->fpLocalTransactionId = InvalidLocalTransactionId; MyProc->xid = InvalidTransactionId; MyProc->xmin = InvalidTransactionId; - MyProc->backendId = InvalidBackendId; MyProc->databaseId = InvalidOid; MyProc->roleId = InvalidOid; MyProc->tempNamespaceId = InvalidOid; MyProc->isBackgroundWorker = IsBackgroundWorker; MyProc->delayChkpt = false; - MyProc->statusFlags = 0; + MyProc->statusFlags = PROC_IS_ACTIVE; MyProc->lwWaiting = false; MyProc->lwWaitMode = 0; MyProc->waitLock = NULL; @@ -913,6 +914,9 @@ ProcKill(int code, Datum arg) MyProc = NULL; DisownLatch(&proc->procLatch); + /* mark this process dead */ + proc->statusFlags &= ~PROC_IS_ACTIVE; + procgloballist = proc->procgloballist; SpinLockAcquire(ProcStructLock); @@ -985,6 +989,7 @@ AuxiliaryProcKill(int code, Datum arg) /* Mark auxiliary proc no longer in use */ proc->pid = 0; + proc->statusFlags &= ~PROC_IS_ACTIVE; /* Update shared estimate of spins_per_delay */ ProcGlobal->spins_per_delay = update_spins_per_delay(ProcGlobal->spins_per_delay); @@ -2020,3 +2025,17 @@ BecomeLockGroupMember(PGPROC *leader, int pid) return ok; } + +/* + * Return PGPROC if it is alive. + */ +PGPROC * +GetProcIfAlive(BackendId backend) +{ + PGPROC *proc = &ProcGlobal->allProcs[backend]; + + if (proc->statusFlags & PROC_IS_ACTIVE) + return proc; + + return NULL; +} diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c index 7229598822..0490a3a8b2 100644 --- a/src/backend/utils/activity/backend_status.c +++ b/src/backend/utils/activity/backend_status.c @@ -249,26 +249,8 @@ void pgstat_beinit(void) { /* Initialize MyBEEntry */ - if (MyBackendId != InvalidBackendId) - { - Assert(MyBackendId >= 1 && MyBackendId <= MaxBackends); - MyBEEntry = &BackendStatusArray[MyBackendId - 1]; - } - else - { - /* Must be an auxiliary process */ - Assert(MyAuxProcType != NotAnAuxProcess); - - /* - * Assign the MyBEEntry for an auxiliary process. Since it doesn't - * have a BackendId, the slot is statically allocated based on the - * auxiliary process type (MyAuxProcType). Backends use slots indexed - * in the range from 1 to MaxBackends (inclusive), so we use - * MaxBackends + AuxBackendType + 1 as the index of the slot for an - * auxiliary process. - */ - MyBEEntry = &BackendStatusArray[MaxBackends + MyAuxProcType]; - } + Assert(MyBackendId >= 0 && MyBackendId < NumBackendStatSlots); + MyBEEntry = &BackendStatusArray[MyBackendId]; /* Set up a process-exit hook to clean up */ on_shmem_exit(pgstat_beshutdown_hook, 0); diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c index d5a7fb13f3..16acb0c335 100644 --- a/src/backend/utils/adt/dbsize.c +++ b/src/backend/utils/adt/dbsize.c @@ -22,6 +22,7 @@ #include "commands/dbcommands.h" #include "commands/tablespace.h" #include "miscadmin.h" +#include "storage/backendid.h" #include "storage/fd.h" #include "utils/acl.h" #include "utils/builtins.h" @@ -292,7 +293,7 @@ pg_tablespace_size_name(PG_FUNCTION_ARGS) * is no check here or at the call sites for that. */ static int64 -calculate_relation_size(RelFileNode *rfn, BackendId backend, ForkNumber forknum) +calculate_relation_size(RelFileNode *rfn, int backend, ForkNumber forknum) { int64 totalsize = 0; char *relationpath; @@ -925,7 +926,7 @@ pg_relation_filepath(PG_FUNCTION_ARGS) HeapTuple tuple; Form_pg_class relform; RelFileNode rnode; - BackendId backend; + int backend; char *path; tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c index 0d52613bc3..fca87448cf 100644 --- a/src/backend/utils/adt/mcxtfuncs.c +++ b/src/backend/utils/adt/mcxtfuncs.c @@ -205,7 +205,7 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS) PG_RETURN_BOOL(false); } - if (SendProcSignal(pid, PROCSIG_LOG_MEMORY_CONTEXT, proc->backendId) < 0) + if (SendProcSignal(pid, PROCSIG_LOG_MEMORY_CONTEXT, proc->pgprocno) < 0) { /* Again, just a warning to allow loops */ ereport(WARNING, diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 13d9994af3..00036ec9c0 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -73,6 +73,7 @@ #include "optimizer/optimizer.h" #include "rewrite/rewriteDefine.h" #include "rewrite/rowsecurity.h" +#include "storage/backendid.h" #include "storage/lmgr.h" #include "storage/smgr.h" #include "utils/array.h" diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index f33729513a..0f927751d7 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -2685,18 +2685,18 @@ log_line_prefix(StringInfo buf, ErrorData *edata) break; case 'v': /* keep VXID format in sync with lockfuncs.c */ - if (MyProc != NULL && MyProc->backendId != InvalidBackendId) + if (MyProc != NULL && MyProc->pgprocno != InvalidBackendId) { if (padding != 0) { char strfbuf[128]; snprintf(strfbuf, sizeof(strfbuf) - 1, "%d/%u", - MyProc->backendId, MyProc->lxid); + MyProc->pgprocno, MyProc->lxid); appendStringInfo(buf, "%*s", padding, strfbuf); } else - appendStringInfo(buf, "%d/%u", MyProc->backendId, MyProc->lxid); + appendStringInfo(buf, "%d/%u", MyProc->pgprocno, MyProc->lxid); } else if (padding != 0) appendStringInfoSpaces(buf, @@ -2860,8 +2860,8 @@ write_csvlog(ErrorData *edata) /* Virtual transaction id */ /* keep VXID format in sync with lockfuncs.c */ - if (MyProc != NULL && MyProc->backendId != InvalidBackendId) - appendStringInfo(&buf, "%d/%u", MyProc->backendId, MyProc->lxid); + if (MyProc != NULL && MyProc->pgprocno != InvalidBackendId) + appendStringInfo(&buf, "%d/%u", MyProc->pgprocno, MyProc->lxid); appendStringInfoChar(&buf, ','); /* Transaction id */ diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index 381d9e548d..4ba1914472 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -81,8 +81,6 @@ char postgres_exec_path[MAXPGPATH]; /* full path to backend */ /* note: currently this is not valid in backend processes */ #endif -BackendId MyBackendId = InvalidBackendId; - BackendId ParallelLeaderBackendId = InvalidBackendId; Oid MyDatabaseId = InvalidOid; diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 78bc64671e..7f5b8e12ee 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -592,15 +592,10 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username, * * Sets up MyBackendId, a unique backend identifier. */ - MyBackendId = InvalidBackendId; - SharedInvalBackendInit(false); - if (MyBackendId > MaxBackends || MyBackendId <= 0) - elog(FATAL, "bad backend ID: %d", MyBackendId); - /* Now that we have a BackendId, we can participate in ProcSignal */ - ProcSignalInit(MyBackendId); + ProcSignalInit(); /* * Also set up timeout handlers needed for backend operation. We need diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index 5001efdf7a..df8476d177 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -1173,7 +1173,7 @@ ExportSnapshot(Snapshot snapshot) * inside the transaction from 1. */ snprintf(path, sizeof(path), SNAPSHOT_EXPORT_DIR "/%08X-%08X-%d", - MyProc->backendId, MyProc->lxid, list_length(exportedSnapshots) + 1); + MyProc->pgprocno, MyProc->lxid, list_length(exportedSnapshots) + 1); /* * Copy the snapshot into TopTransactionContext, add it to the @@ -1200,7 +1200,7 @@ ExportSnapshot(Snapshot snapshot) */ initStringInfo(&buf); - appendStringInfo(&buf, "vxid:%d/%u\n", MyProc->backendId, MyProc->lxid); + appendStringInfo(&buf, "vxid:%d/%u\n", MyProc->pgprocno, MyProc->lxid); appendStringInfo(&buf, "pid:%d\n", MyProcPid); appendStringInfo(&buf, "dbid:%u\n", MyDatabaseId); appendStringInfo(&buf, "iso:%d\n", XactIsoLevel); diff --git a/src/include/storage/backendid.h b/src/include/storage/backendid.h index 7aa3936899..3772e2b4a2 100644 --- a/src/include/storage/backendid.h +++ b/src/include/storage/backendid.h @@ -20,7 +20,8 @@ */ typedef int BackendId; /* unique currently active backend identifier */ -#define InvalidBackendId (-1) +#define INVALID_PGPROCNO PG_INT32_MAX +#define InvalidBackendId INVALID_PGPROCNO extern PGDLLIMPORT BackendId MyBackendId; /* backend id of this backend */ diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h index 9b2a421c32..13b9352704 100644 --- a/src/include/storage/lock.h +++ b/src/include/storage/lock.h @@ -62,7 +62,7 @@ extern bool Debug_deadlocks; */ typedef struct { - BackendId backendId; /* backendId from PGPROC */ + BackendId backendId; /* pgprocno from PGPROC */ LocalTransactionId localTransactionId; /* lxid from PGPROC */ } VirtualTransactionId; @@ -79,7 +79,7 @@ typedef struct ((vxid).backendId = InvalidBackendId, \ (vxid).localTransactionId = InvalidLocalTransactionId) #define GET_VXID_FROM_PGPROC(vxid, proc) \ - ((vxid).backendId = (proc).backendId, \ + ((vxid).backendId = (proc).pgprocno, \ (vxid).localTransactionId = (proc).lxid) /* MAX_LOCKMODES cannot be larger than the # of bits in LOCKMASK */ @@ -445,7 +445,7 @@ typedef struct LockInstanceData LOCKTAG locktag; /* tag for locked object */ LOCKMASK holdMask; /* locks held by this PGPROC */ LOCKMODE waitLockMode; /* lock awaited by this PGPROC, if any */ - BackendId backend; /* backend ID of this PGPROC */ + BackendId backend; /* pgprocno of this PGPROC */ LocalTransactionId lxid; /* local transaction ID of this PGPROC */ TimestampTz waitStart; /* time at which this PGPROC started waiting * for lock */ diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index be67d8a861..2f3d9cfb17 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -17,6 +17,7 @@ #include "access/clog.h" #include "access/xlogdefs.h" #include "lib/ilist.h" +#include "storage/backendid.h" #include "storage/latch.h" #include "storage/lock.h" #include "storage/pg_sema.h" @@ -60,6 +61,7 @@ struct XidCache #define PROC_VACUUM_FOR_WRAPAROUND 0x08 /* set by autovac only */ #define PROC_IN_LOGICAL_DECODING 0x10 /* currently doing logical * decoding outside xact */ +#define PROC_IS_ACTIVE 0x20 /* This process is active */ /* flags reset at EOXact */ #define PROC_VACUUM_STATE_MASK \ @@ -73,11 +75,7 @@ struct XidCache */ #define FP_LOCK_SLOTS_PER_BACKEND 16 -/* - * An invalid pgprocno. Must be larger than the maximum number of PGPROC - * structures we could possibly have. See comments for MAX_BACKENDS. - */ -#define INVALID_PGPROCNO PG_INT32_MAX +BackendId MyBackendId; typedef enum { @@ -150,7 +148,6 @@ struct PGPROC int pgprocno; /* These fields are zero while a backend is still starting up: */ - BackendId backendId; /* This backend's backend ID (if assigned) */ Oid databaseId; /* OID of database this backend is using */ Oid roleId; /* OID of role using this backend */ @@ -418,4 +415,6 @@ extern PGPROC *AuxiliaryPidGetProc(int pid); extern void BecomeLockGroupLeader(void); extern bool BecomeLockGroupMember(PGPROC *leader, int pid); +extern PGPROC *GetProcIfAlive(BackendId backend); + #endif /* _PROC_H_ */ diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h index eec186be2e..34a7fff271 100644 --- a/src/include/storage/procsignal.h +++ b/src/include/storage/procsignal.h @@ -63,9 +63,9 @@ typedef enum extern Size ProcSignalShmemSize(void); extern void ProcSignalShmemInit(void); -extern void ProcSignalInit(int pss_idx); +extern void ProcSignalInit(void); extern int SendProcSignal(pid_t pid, ProcSignalReason reason, - BackendId backendId); + int pgprocno); extern uint64 EmitProcSignalBarrier(ProcSignalBarrierType type); extern void WaitForProcSignalBarrier(uint64 generation); diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h index a6fbf7b6a6..7c1063f9f9 100644 --- a/src/include/storage/smgr.h +++ b/src/include/storage/smgr.h @@ -78,7 +78,7 @@ typedef SMgrRelationData *SMgrRelation; RelFileNodeBackendIsTemp((smgr)->smgr_rnode) extern void smgrinit(void); -extern SMgrRelation smgropen(RelFileNode rnode, BackendId backend); +extern SMgrRelation smgropen(RelFileNode rnode, int backend); extern bool smgrexists(SMgrRelation reln, ForkNumber forknum); extern void smgrsetowner(SMgrRelation *owner, SMgrRelation reln); extern void smgrclearowner(SMgrRelation *owner, SMgrRelation reln); diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index b4faa1c123..1ef0571201 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -56,7 +56,7 @@ typedef struct RelationData RelFileNode rd_node; /* relation physical identifier */ SMgrRelation rd_smgr; /* cached file handle, or NULL */ int rd_refcnt; /* reference count */ - BackendId rd_backend; /* owning backend id, if temporary relation */ + int rd_backend; /* owning backend id, if temporary relation */ bool rd_islocaltemp; /* rel is a temp rel of this session */ bool rd_isnailed; /* rel is nailed in cache */ bool rd_isvalid; /* relcache entry is valid */ -- 2.27.0 From 905ec70637191636e1e997e9a42e867a3521f768 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Date: Fri, 15 Oct 2021 13:15:13 +0900 Subject: [PATCH v2 3/5] Remove O(n) behavior of SIInsertDataEntries SIInsertDataEntries ran a loop over a certain range of procState array at every insertion. Get rid of the behavior so that we won't be annoyed by performance drag that may happen when procState array gets sparse. This commit removes hasMessage from ProcState. Instead the function uses maxMsgNum to check for new messages. --- src/backend/storage/ipc/sinvaladt.c | 63 +++++++++-------------------- 1 file changed, 20 insertions(+), 43 deletions(-) diff --git a/src/backend/storage/ipc/sinvaladt.c b/src/backend/storage/ipc/sinvaladt.c index a90e9920e7..ee3a3accfd 100644 --- a/src/backend/storage/ipc/sinvaladt.c +++ b/src/backend/storage/ipc/sinvaladt.c @@ -143,7 +143,6 @@ typedef struct ProcState int nextMsgNum; /* next message number to read */ bool resetState; /* backend needs to reset its state */ bool signaled; /* backend has been sent catchup signal */ - bool hasMessages; /* backend has unread messages */ /* * Backend only sends invalidations, never receives them. This only makes @@ -244,7 +243,6 @@ CreateSharedInvalidationState(void) shmInvalBuffer->procState[i].nextMsgNum = 0; /* meaningless */ shmInvalBuffer->procState[i].resetState = false; shmInvalBuffer->procState[i].signaled = false; - shmInvalBuffer->procState[i].hasMessages = false; shmInvalBuffer->procState[i].nextLXID = InvalidLocalTransactionId; } } @@ -309,7 +307,6 @@ SharedInvalBackendInit(bool sendOnly) stateP->nextMsgNum = segP->maxMsgNum; stateP->resetState = false; stateP->signaled = false; - stateP->hasMessages = false; stateP->sendOnly = sendOnly; LWLockRelease(SInvalWriteLock); @@ -417,7 +414,6 @@ SIInsertDataEntries(const SharedInvalidationMessage *data, int n) int nthistime = Min(n, WRITE_QUANTUM); int numMsgs; int max; - int i; n -= nthistime; @@ -450,24 +446,13 @@ SIInsertDataEntries(const SharedInvalidationMessage *data, int n) max++; } - /* Update current value of maxMsgNum using spinlock */ - SpinLockAcquire(&segP->msgnumLock); + /* + * Update current value of maxMsgNum without taking locks. Make sure + * the inserted messages habve been flushed to main memory before the + * update of the shared variable, + */ + pg_memory_barrier(); segP->maxMsgNum = max; - SpinLockRelease(&segP->msgnumLock); - - /* - * Now that the maxMsgNum change is globally visible, we give everyone - * a swift kick to make sure they read the newly added messages. - * Releasing SInvalWriteLock will enforce a full memory barrier, so - * these (unlocked) changes will be committed to memory before we exit - * the function. - */ - for (i = 0; i < segP->lastBackend; i++) - { - ProcState *stateP = &segP->procState[i]; - - stateP->hasMessages = true; - } LWLockRelease(SInvalWriteLock); } @@ -523,27 +508,24 @@ SIGetDataEntries(SharedInvalidationMessage *data, int datasize) * invalidations, any such occurrence is not much different than if the * invalidation had arrived slightly later in the first place. */ - if (!stateP->hasMessages) + max = segP->maxMsgNum; + + if (stateP->nextMsgNum >= max) return 0; LWLockAcquire(SInvalReadLock, LW_SHARED); - /* - * We must reset hasMessages before determining how many messages we're - * going to read. That way, if new messages arrive after we have - * determined how many we're reading, the flag will get reset and we'll - * notice those messages part-way through. - * - * Note that, if we don't end up reading all of the messages, we had - * better be certain to reset this flag before exiting! - */ - stateP->hasMessages = false; - - /* Fetch current value of maxMsgNum using spinlock */ - SpinLockAcquire(&segP->msgnumLock); - max = segP->maxMsgNum; - SpinLockRelease(&segP->msgnumLock); - + if (stateP->nextMsgNum < max) + { + /* + * nextMsgNum has been rewinded before we acquired the lock. Recheck + * with maxMsgNum again. + */ + max = segP->maxMsgNum; + if (stateP->nextMsgNum >= max) + return 0; + } + if (stateP->resetState) { /* @@ -576,14 +558,9 @@ SIGetDataEntries(SharedInvalidationMessage *data, int datasize) /* * If we have caught up completely, reset our "signaled" flag so that * we'll get another signal if we fall behind again. - * - * If we haven't caught up completely, reset the hasMessages flag so that - * we see the remaining messages next time. */ if (stateP->nextMsgNum >= max) stateP->signaled = false; - else - stateP->hasMessages = true; LWLockRelease(SInvalReadLock); return n; -- 2.27.0 From 6338a5397e9103609ba9e842e2390ffab4b9aa5c Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Date: Fri, 15 Oct 2021 14:09:30 +0900 Subject: [PATCH v2 4/5] Get rid of two O(1) behavior in procarray. GetVirtualXIDsDelayingChkpt had O(N^2) behavior and SignalVirtualTransaction had O(N) behavior. Since the old BackendId has gone they can be reduced to O(N) and O(1) respectively. --- src/backend/storage/ipc/procarray.c | 74 ++++++++++------------------- 1 file changed, 26 insertions(+), 48 deletions(-) diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 100c0dae8c..70ce58b3fc 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -3079,41 +3079,24 @@ GetVirtualXIDsDelayingChkpt(int *nvxids) * * This is used with the results of GetVirtualXIDsDelayingChkpt to see if any * of the specified VXIDs are still in critical sections of code. - * - * Note: this is O(N^2) in the number of vxacts that are/were delaying, but - * those numbers should be small enough for it not to be a problem. */ bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids) { bool result = false; - ProcArrayStruct *arrayP = procArray; int index; LWLockAcquire(ProcArrayLock, LW_SHARED); - for (index = 0; index < arrayP->numProcs; index++) + for (index = 0; index < nvxids; index++) { - int pgprocno = arrayP->pgprocnos[index]; - PGPROC *proc = &allProcs[pgprocno]; - VirtualTransactionId vxid; + VirtualTransactionId *vxid = &vxids[index]; + PGPROC *proc = &allProcs[vxid->backendId]; - GET_VXID_FROM_PGPROC(vxid, *proc); - - if (proc->delayChkpt && VirtualTransactionIdIsValid(vxid)) + if (proc->delayChkpt && vxid->localTransactionId == proc->lxid) { - int i; - - for (i = 0; i < nvxids; i++) - { - if (VirtualTransactionIdEquals(vxid, vxids[i])) - { - result = true; - break; - } - } - if (result) - break; + result = true; + break; } } @@ -3429,35 +3412,30 @@ pid_t SignalVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode, bool conflictPending) { - ProcArrayStruct *arrayP = procArray; - int index; - pid_t pid = 0; + PGPROC *proc; + pid_t pid; + VirtualTransactionId procvxid PG_USED_FOR_ASSERTS_ONLY; LWLockAcquire(ProcArrayLock, LW_SHARED); - for (index = 0; index < arrayP->numProcs; index++) + proc = &allProcs[vxid.backendId]; + +#ifdef USE_ASSERT_CHECKING + GET_VXID_FROM_PGPROC(procvxid, *proc); + Assert (procvxid.backendId == vxid.backendId && + procvxid.localTransactionId == vxid.localTransactionId); +#endif + + pid = proc->pid; + proc->recoveryConflictPending = conflictPending; + + if (pid != 0) { - int pgprocno = arrayP->pgprocnos[index]; - PGPROC *proc = &allProcs[pgprocno]; - VirtualTransactionId procvxid; - - GET_VXID_FROM_PGPROC(procvxid, *proc); - - if (procvxid.backendId == vxid.backendId && - procvxid.localTransactionId == vxid.localTransactionId) - { - proc->recoveryConflictPending = conflictPending; - pid = proc->pid; - if (pid != 0) - { - /* - * Kill the pid if it's still here. If not, that's what we - * wanted so ignore any errors. - */ - (void) SendProcSignal(pid, sigmode, proc->pgprocno); - } - break; - } + /* + * Kill the pid if it's still here. If not, that's what we + * wanted so ignore any errors. + */ + (void) SendProcSignal(pid, sigmode, proc->pgprocno); } LWLockRelease(ProcArrayLock); -- 2.27.0 From db5bdd8e1a350cf77b77884ec445b8f0f18f135a Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Date: Fri, 15 Oct 2021 14:23:52 +0900 Subject: [PATCH v2 5/5] Get rid of the old BackendId at all Since we have got rid of the annoyging O(N) behavior in SIInsertDataEntries, sinvaladt.c can say good-by to the old packed Backend ID. Still CleanupInvalidationState and SICleanupQueue has O(N) behavior but they are executed rarely or ends in a short time in the most cases. --- src/backend/storage/ipc/sinvaladt.c | 67 ++++++----------------------- 1 file changed, 14 insertions(+), 53 deletions(-) diff --git a/src/backend/storage/ipc/sinvaladt.c b/src/backend/storage/ipc/sinvaladt.c index ee3a3accfd..d56c77400f 100644 --- a/src/backend/storage/ipc/sinvaladt.c +++ b/src/backend/storage/ipc/sinvaladt.c @@ -193,8 +193,6 @@ static LocalTransactionId nextLocalTransactionId; static void CleanupInvalidationState(int status, Datum arg); -static int siindex; - /* * SInvalShmemSize --- return shared-memory space needed */ @@ -254,7 +252,6 @@ CreateSharedInvalidationState(void) void SharedInvalBackendInit(bool sendOnly) { - int index; ProcState *stateP = NULL; SISeg *segP = shmInvalBuffer; @@ -265,38 +262,15 @@ SharedInvalBackendInit(bool sendOnly) */ LWLockAcquire(SInvalWriteLock, LW_EXCLUSIVE); - /* Look for a free entry in the procState array */ - for (index = 0; index < segP->lastBackend; index++) - { - if (segP->procState[index].procPid == 0) /* inactive slot? */ - { - stateP = &segP->procState[index]; - break; - } - } + Assert (MyBackendId < MaxBackends); + stateP = &segP->procState[MyBackendId]; - if (stateP == NULL) - { - if (segP->lastBackend < segP->maxBackends) - { - stateP = &segP->procState[segP->lastBackend]; - Assert(stateP->procPid == 0); - segP->lastBackend++; - } - else - { - /* - * out of procState slots: MaxBackends exceeded -- report normally - */ - siindex = -1; - LWLockRelease(SInvalWriteLock); - ereport(FATAL, - (errcode(ERRCODE_TOO_MANY_CONNECTIONS), - errmsg("sorry, too many clients already"))); - } - } + /* this entry should be free */ + Assert (stateP->procPid == 0); - siindex = (stateP - &segP->procState[0]); + /* adjust lastBackend if needed */ + if (segP->lastBackend < MyBackendId) + segP->lastBackend = MyBackendId; /* Fetch next local transaction ID into local memory */ nextLocalTransactionId = stateP->nextLXID; @@ -313,8 +287,6 @@ SharedInvalBackendInit(bool sendOnly) /* register exit routine to mark my entry inactive at exit */ on_shmem_exit(CleanupInvalidationState, PointerGetDatum(segP)); - - elog(DEBUG4, "my SI slot index is %d", siindex); } /* @@ -336,7 +308,7 @@ CleanupInvalidationState(int status, Datum arg) LWLockAcquire(SInvalWriteLock, LW_EXCLUSIVE); - stateP = &segP->procState[siindex]; + stateP = &segP->procState[MyBackendId]; /* Update next local transaction ID for next holder of this backendID */ stateP->nextLXID = nextLocalTransactionId; @@ -349,7 +321,7 @@ CleanupInvalidationState(int status, Datum arg) stateP->signaled = false; /* Recompute index of last active backend */ - for (i = segP->lastBackend; i > 0; i--) + for (i = segP->lastBackend; i >= 0; i--) { if (segP->procState[i - 1].procPid != 0) break; @@ -368,27 +340,16 @@ CleanupInvalidationState(int status, Datum arg) void BackendIdGetTransactionIds(int backendID, TransactionId *xid, TransactionId *xmin) { - SISeg *segP = shmInvalBuffer; - *xid = InvalidTransactionId; *xmin = InvalidTransactionId; - /* Need to lock out additions/removals of backends */ - LWLockAcquire(SInvalWriteLock, LW_SHARED); - - if (backendID > 0 && backendID <= segP->lastBackend) + if (backendID >= 0 && backendID < MaxBackends) { - ProcState *stateP = &segP->procState[backendID - 1]; - PGPROC *proc = stateP->proc; + PGPROC *proc = &ProcGlobal->allProcs[backendID]; - if (proc != NULL) - { - *xid = proc->xid; - *xmin = proc->xmin; - } + *xid = proc->xid; + *xmin = proc->xmin; } - - LWLockRelease(SInvalWriteLock); } /* @@ -495,7 +456,7 @@ SIGetDataEntries(SharedInvalidationMessage *data, int datasize) int n; segP = shmInvalBuffer; - stateP = &segP->procState[siindex]; + stateP = &segP->procState[MyBackendId]; /* * Before starting to take locks, do a quick, unlocked test to see whether -- 2.27.0
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Kyotaro Horiguchi
Дата:
(This branch may should leave from this thread..) At Fri, 15 Oct 2021 15:00:57 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > At Thu, 14 Oct 2021 10:53:06 -0700, Andres Freund <andres@anarazel.de> wrote in > > This'd get rid of the need of density *and* make SIInsertDataEntries() > > cheaper. > > Yes. So.. I tried that. The only part where memory-flush timing is > crucial seems to be between writing messages and setting maxMsgNum. > By placing memory barrier between them it seems *to me* we can read > maxMsgNum safely without locks. Maybe we need another memory barrier here and the patch was broken about the rechecking on the members in GetSIGetDataEntries.. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
От
Bharath Rupireddy
Дата:
On Fri, Oct 15, 2021 at 11:31 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Thu, 14 Oct 2021 10:53:06 -0700, Andres Freund <andres@anarazel.de> wrote in > > Hi, > > > > On 2021-10-14 17:28:34 +0900, Kyotaro Horiguchi wrote: > > > At Wed, 13 Oct 2021 19:52:52 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > > > > Although needing a bit of care for the difference of invalid values > > > > for both though, BackendId can be easily replaced with pgprocno almost > > > > mechanically except sinvaladt. Therefore, we can confine the current > > > > backend ID within sinvaladt isolating from other part. The ids > > > > dedicated for sinvaladt can be packed to small range and perfomance > > > > won't be damaged. > > > > FWIW, I don't actually think there's necessarily that strong a need for > > density in sinvaladt. With a few relatively changes we can get rid of the O(n) > > work in the most crucial paths. > > Right. So I left it for the "future:p > > > In https://www.postgresql.org/message-id/20210802171255.k4yv5cfqaqbuuy6f%40alap3.anarazel.de > > I wrote: > > > Another approach to deal with this could be to simply not do the O(n) work in > > > SIInsertDataEntries(). It's not obvious that ->hasMessages is actually > > > necessary - we could atomically read maxMsgNum without acquiring a lock > > > instead of needing the per-backend ->hasMessages. I don't the density would > > > be a relevant factor in SICleanupQueue(). > > > > This'd get rid of the need of density *and* make SIInsertDataEntries() > > cheaper. > > Yes. So.. I tried that. The only part where memory-flush timing is > crucial seems to be between writing messages and setting maxMsgNum. > By placing memory barrier between them it seems *to me* we can read > maxMsgNum safely without locks. > > I reread that thread and found we can get rid of O(N) behavior from > two places, SIgnalVirtualTransaction and GetVirtualXIDsDelayingChkpt. > > Finally, I got rid of the siindex (the old BackendId) from sinvaladt.c > at all. Still CleanupInvalidationState and SICleanupQueue has O(N) > behavior but they are executed rarely or ends in a short time in the > most cases. > > > 0001: Reverses the proc freelist so that the backendid is assigned in > the sane order. > > 0002: Replaces the current BackendId - that is generated by > sinvaladt.c intending to pack the ids to a narrow range - with > pgprocno in most of the tree. The old BackendID is now used only in > sinvaladt.c > > 0003: Removes O(N) behavior from SIInsertDataEntries. I'm not sure it > is correctly revised, though.. > > 0004: Gets rid of O(N), or reduce O(N^2) to O(N) of > HaveVirtualXIDsDelayingChkpt and SignalVirtualTransaction. > > 0005: Gets rid of the old BackendID at all from sinvaladt.c. Hi, I'm not sure if the above approach and the patches are okay that I or someone can start reviewing them. Does anyone have comments on this please? Regards, Bharath Rupireddy.