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

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

At Mon, 11 Oct 2021 15:24:46 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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

On Mon, Oct 11, 2021 at 12:41 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)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.

On Mon, Oct 11, 2021 at 11:54 AM Fujii Masao
<masao(dot)fujii(at)oss(dot)nttdata(dot)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

On Mon, Oct 11, 2021 at 8:59 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>
> On 2021/10/11 19:46, Bharath Rupireddy wrote:
> > If we do the above, then the problem might arise if somebody calls
> > SICleanupQueue and wants to signal the startup process, the below code
> > (from SICleanupQueue) can't get the startup process backend id. So,
> > the backend id calculation for the startup process can't just be
> > MaxBackends + MyAuxProcType + 1.
> > BackendId his_backendId = (needSig - &segP->procState[0]) + 1;
>
> Attached POC patch illustrates what I'm in mind. ISTM this change
> doesn't prevent SICleanupQueue() from getting right backend ID
> of the startup process. Thought?

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.

On Mon, Oct 11, 2021 at 8:59 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>
> On 2021/10/11 19:46, Bharath Rupireddy wrote:
> > If we do the above, then the problem might arise if somebody calls
> > SICleanupQueue and wants to signal the startup process, the below code
> > (from SICleanupQueue) can't get the startup process backend id. So,
> > the backend id calculation for the startup process can't just be
> > MaxBackends + MyAuxProcType + 1.
> > BackendId his_backendId = (needSig - &segP->procState[0]) + 1;
>
> Attached POC patch illustrates what I'm in mind. ISTM this change
> doesn't prevent SICleanupQueue() from getting right backend ID
> of the startup process. Thought?

The patch looks good to me unless I'm missing something badly.

+ Assert(MyBackendId == InvalidBackendId ||
+ MyBackendId <= segP->maxBackends);
+
In the above assertion, we can just do MyBackendId ==
segP->maxBackends, instead of <= as the startup process is the only
process that calls SharedInvalBackendInit with pre-calculated
MyBackendId = MaxBackends + MyAuxProcType + 1; and it will always
occupy the last slot in the procState array.

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.

At Mon, 11 Oct 2021 16:03:57 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> On Mon, Oct 11, 2021 at 12:41 PM Kyotaro Horiguchi
> <horikyota(dot)ntt(at)gmail(dot)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

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

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

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

The patch does this:

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

as well as this:

+ shmInvalBuffer->maxBackends = MaxBackends + 1;

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

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

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Oct 12, 2021 at 2:03 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)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

At Tue, 12 Oct 2021 14:57:58 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> On Tue, Oct 12, 2021 at 2:03 PM Kyotaro Horiguchi
> <horikyota(dot)ntt(at)gmail(dot)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

On Wed, Oct 13, 2021 at 4:25 AM Andres Freund <andres(at)anarazel(dot)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.

At Wed, 13 Oct 2021 13:39:24 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> On Wed, Oct 13, 2021 at 4:25 AM Andres Freund <andres(at)anarazel(dot)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

At Wed, 13 Oct 2021 19:52:52 +0900 (JST), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)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 difrence of 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

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(dot)ntt(at)gmail(dot)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.

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.

Greetings,

Andres Freund

At Thu, 14 Oct 2021 10:53:06 -0700, Andres Freund <andres(at)anarazel(dot)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(dot)ntt(at)gmail(dot)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

(This branch may should leave from this thread..)

At Fri, 15 Oct 2021 15:00:57 +0900 (JST), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote in
> At Thu, 14 Oct 2021 10:53:06 -0700, Andres Freund <andres(at)anarazel(dot)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

On Fri, Oct 15, 2021 at 11:31 AM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Thu, 14 Oct 2021 10:53:06 -0700, Andres Freund <andres(at)anarazel(dot)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(dot)ntt(at)gmail(dot)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.