Accommodate startup process in a separate ProcState array slot instead of in MaxBackends slots.
Hi,
While working on [1]/messages/by-id/2222ab6f-46b1-d5c0-603d-8f6680739db4@oss.nttdata.com, it is found that currently the ProcState array
doesn't have entries for auxiliary processes, it does have entries for
MaxBackends. But the startup process is eating up one slot from
MaxBackends. We need to increase the size of the ProcState array by 1
at least for the startup process. The startup process uses ProcState
slot via InitRecoveryTransactionEnvironment->SharedInvalBackendInit.
The procState array size is initialized to MaxBackends in
SInvalShmemSize.
The consequence of not fixing this issue is that the database may hit
the error "sorry, too many clients already" soon in
SharedInvalBackendInit.
Attaching a patch to fix this issue. Thoughts?
[1]: /messages/by-id/2222ab6f-46b1-d5c0-603d-8f6680739db4@oss.nttdata.com
Regards,
Bharath Rupireddy.
Attachments:
v1-0001-Accommodate-startup-process-in-a-separate-ProcSta.patchapplication/octet-stream; name=v1-0001-Accommodate-startup-process-in-a-separate-ProcSta.patchDownload
From b217f466ea820c78a6eb27dc5048ce5af40eccdb Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Mon, 11 Oct 2021 18:49:43 +0000
Subject: [PATCH v1] Accommodate startup process in a separate ProcState array
slot instead of in MaxBackends slots.
Currently the ProcState array doesn't have entries for auxiliary
processes, it does have entries for MaxBackends. But the startup
process is eating up one slot from MaxBackends. We need to
increase the size of the ProcState array by 1 at least for the
startup process. The startup process uses ProcState via
InitRecoveryTransactionEnvironment()->SharedInvalBackendInit().
The procState array size is initialized to MaxBackends in SInvalShmemSize.
The consequence of not fixing this issue is clear: the database hits
the error "sorry, too many clients already" soon. For instance,
if MaxBackends is 100, then the error gets emitted on 100th backend
connection attempt as opposed to the error on 101th connection attempt.
---
src/backend/storage/ipc/sinvaladt.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/backend/storage/ipc/sinvaladt.c b/src/backend/storage/ipc/sinvaladt.c
index 946bd8e3cb..f49fdd78f6 100644
--- a/src/backend/storage/ipc/sinvaladt.c
+++ b/src/backend/storage/ipc/sinvaladt.c
@@ -183,7 +183,8 @@ typedef struct SISeg
SharedInvalidationMessage buffer[MAXNUMMESSAGES];
/*
- * Per-backend invalidation state info (has MaxBackends entries).
+ * Invalidation state info for backends and startup process
+ * (MaxBackends + 1 entries).
*/
ProcState procState[FLEXIBLE_ARRAY_MEMBER];
} SISeg;
@@ -205,7 +206,8 @@ SInvalShmemSize(void)
Size size;
size = offsetof(SISeg, procState);
- size = add_size(size, mul_size(sizeof(ProcState), MaxBackends));
+ /* The startup process also requires a slot, along with backends */
+ size = add_size(size, mul_size(sizeof(ProcState), MaxBackends + 1));
return size;
}
@@ -231,7 +233,8 @@ CreateSharedInvalidationState(void)
shmInvalBuffer->maxMsgNum = 0;
shmInvalBuffer->nextThreshold = CLEANUP_MIN;
shmInvalBuffer->lastBackend = 0;
- shmInvalBuffer->maxBackends = MaxBackends;
+ /* The startup process also requires a slot, along with backends */
+ shmInvalBuffer->maxBackends = MaxBackends + 1;
SpinLockInit(&shmInvalBuffer->msgnumLock);
/* The buffer[] array is initially all unused, so we need not fill it */
@@ -288,7 +291,7 @@ SharedInvalBackendInit(bool sendOnly)
else
{
/*
- * out of procState slots: MaxBackends exceeded -- report normally
+ * out of procState slots: maxBackends exceeded -- report normally
*/
MyBackendId = InvalidBackendId;
LWLockRelease(SInvalWriteLock);
--
2.25.1
On 2021/10/12 4:07, Bharath Rupireddy wrote:
Hi,
While working on [1], it is found that currently the ProcState array
doesn't have entries for auxiliary processes, it does have entries for
MaxBackends. But the startup process is eating up one slot from
MaxBackends. We need to increase the size of the ProcState array by 1
at least for the startup process. The startup process uses ProcState
slot via InitRecoveryTransactionEnvironment->SharedInvalBackendInit.
The procState array size is initialized to MaxBackends in
SInvalShmemSize.The consequence of not fixing this issue is that the database may hit
the error "sorry, too many clients already" soon in
SharedInvalBackendInit.Attaching a patch to fix this issue. Thoughts?
Thanks for making the patch! LGTM.
Barring any objection, I will commit it.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Tue, Oct 12, 2021 at 5:37 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/10/12 4:07, Bharath Rupireddy wrote:
Hi,
While working on [1], it is found that currently the ProcState array
doesn't have entries for auxiliary processes, it does have entries for
MaxBackends. But the startup process is eating up one slot from
MaxBackends. We need to increase the size of the ProcState array by 1
at least for the startup process. The startup process uses ProcState
slot via InitRecoveryTransactionEnvironment->SharedInvalBackendInit.
The procState array size is initialized to MaxBackends in
SInvalShmemSize.The consequence of not fixing this issue is that the database may hit
the error "sorry, too many clients already" soon in
SharedInvalBackendInit.Attaching a patch to fix this issue. Thoughts?
Thanks for making the patch! LGTM.
Barring any objection, I will commit it.
Thanks for reviewing. I've made a CF entry for this, just to ensure
the tests on different CF bot server passes(and yes no failures) -
https://commitfest.postgresql.org/35/3355/
Regards,
Bharath Rupireddy.
On 2021/10/12 15:46, Bharath Rupireddy wrote:
On Tue, Oct 12, 2021 at 5:37 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/10/12 4:07, Bharath Rupireddy wrote:
Hi,
While working on [1], it is found that currently the ProcState array
doesn't have entries for auxiliary processes, it does have entries for
MaxBackends. But the startup process is eating up one slot from
MaxBackends. We need to increase the size of the ProcState array by 1
at least for the startup process. The startup process uses ProcState
slot via InitRecoveryTransactionEnvironment->SharedInvalBackendInit.
The procState array size is initialized to MaxBackends in
SInvalShmemSize.The consequence of not fixing this issue is that the database may hit
the error "sorry, too many clients already" soon in
SharedInvalBackendInit.
On second thought, I wonder if this error could not happen in practice. No?
Because autovacuum doesn't work during recovery and the startup process
can safely use the ProcState entry for autovacuum worker process.
Also since the minimal allowed value of autovacuum_max_workers is one,
the ProcState array guarantees to have at least one entry for autovacuum worker.
If this understanding is right, we don't need to enlarge the array and
can just update the comment. I don't strongly oppose to enlarge
the array in the master, but I'm not sure it's worth doing that
in back branches if the issue can cause no actual error.
Attaching a patch to fix this issue. Thoughts?
Thanks for making the patch! LGTM.
Barring any objection, I will commit it.Thanks for reviewing. I've made a CF entry for this, just to ensure
the tests on different CF bot server passes(and yes no failures) -
https://commitfest.postgresql.org/35/3355/
Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Thu, Oct 14, 2021 at 10:56 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
On 2021/10/12 15:46, Bharath Rupireddy wrote:
On Tue, Oct 12, 2021 at 5:37 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/10/12 4:07, Bharath Rupireddy wrote:
Hi,
While working on [1], it is found that currently the ProcState array
doesn't have entries for auxiliary processes, it does have entries for
MaxBackends. But the startup process is eating up one slot from
MaxBackends. We need to increase the size of the ProcState array by 1
at least for the startup process. The startup process uses ProcState
slot via InitRecoveryTransactionEnvironment->SharedInvalBackendInit.
The procState array size is initialized to MaxBackends in
SInvalShmemSize.The consequence of not fixing this issue is that the database may hit
the error "sorry, too many clients already" soon in
SharedInvalBackendInit.On second thought, I wonder if this error could not happen in practice. No?
Because autovacuum doesn't work during recovery and the startup process
can safely use the ProcState entry for autovacuum worker process.
Also since the minimal allowed value of autovacuum_max_workers is one,
the ProcState array guarantees to have at least one entry for autovacuum worker.If this understanding is right, we don't need to enlarge the array and
can just update the comment. I don't strongly oppose to enlarge
the array in the master, but I'm not sure it's worth doing that
in back branches if the issue can cause no actual error.
Yes, the issue can't happen. The comment in the SInvalShmemSize,
mentioning about the startup process always having an extra slot
because the autovacuum worker is not active during recovery, looks
okay. But, is it safe to assume that always? Do we have a way to
specify that in the form an Assert(when_i_am_startup_proc &&
autovacuum_not_running) (this looks a bit dirty though)? Instead, we
can just enlarge the array in the master and be confident about the
fact that the startup process always has one dedicated slot.
Regards,
Bharath Rupireddy.
В Сб, 16/10/2021 в 16:37 +0530, Bharath Rupireddy пишет:
On Thu, Oct 14, 2021 at 10:56 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:On 2021/10/12 15:46, Bharath Rupireddy wrote:
On Tue, Oct 12, 2021 at 5:37 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/10/12 4:07, Bharath Rupireddy wrote:
Hi,
While working on [1], it is found that currently the ProcState array
doesn't have entries for auxiliary processes, it does have entries for
MaxBackends. But the startup process is eating up one slot from
MaxBackends. We need to increase the size of the ProcState array by 1
at least for the startup process. The startup process uses ProcState
slot via InitRecoveryTransactionEnvironment->SharedInvalBackendInit.
The procState array size is initialized to MaxBackends in
SInvalShmemSize.The consequence of not fixing this issue is that the database may hit
the error "sorry, too many clients already" soon in
SharedInvalBackendInit.On second thought, I wonder if this error could not happen in practice. No?
Because autovacuum doesn't work during recovery and the startup process
can safely use the ProcState entry for autovacuum worker process.
Also since the minimal allowed value of autovacuum_max_workers is one,
the ProcState array guarantees to have at least one entry for autovacuum worker.If this understanding is right, we don't need to enlarge the array and
can just update the comment. I don't strongly oppose to enlarge
the array in the master, but I'm not sure it's worth doing that
in back branches if the issue can cause no actual error.Yes, the issue can't happen. The comment in the SInvalShmemSize,
mentioning about the startup process always having an extra slot
because the autovacuum worker is not active during recovery, looks
okay. But, is it safe to assume that always? Do we have a way to
specify that in the form an Assert(when_i_am_startup_proc &&
autovacuum_not_running) (this looks a bit dirty though)? Instead, we
can just enlarge the array in the master and be confident about the
fact that the startup process always has one dedicated slot.
But this slot wont be used for most of cluster life. It will be just
waste.
And `Assert(there_is_startup_proc && autovacuum_not_running)` has
value on its own, hasn't it? So why doesn't add it with comment.
regards,
Yura Sokolov
On Fri, Feb 11, 2022 at 7:56 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote:
В Сб, 16/10/2021 в 16:37 +0530, Bharath Rupireddy пишет:
On Thu, Oct 14, 2021 at 10:56 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:On 2021/10/12 15:46, Bharath Rupireddy wrote:
On Tue, Oct 12, 2021 at 5:37 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/10/12 4:07, Bharath Rupireddy wrote:
Hi,
While working on [1], it is found that currently the ProcState array
doesn't have entries for auxiliary processes, it does have entries for
MaxBackends. But the startup process is eating up one slot from
MaxBackends. We need to increase the size of the ProcState array by 1
at least for the startup process. The startup process uses ProcState
slot via InitRecoveryTransactionEnvironment->SharedInvalBackendInit.
The procState array size is initialized to MaxBackends in
SInvalShmemSize.The consequence of not fixing this issue is that the database may hit
the error "sorry, too many clients already" soon in
SharedInvalBackendInit.On second thought, I wonder if this error could not happen in practice. No?
Because autovacuum doesn't work during recovery and the startup process
can safely use the ProcState entry for autovacuum worker process.
Also since the minimal allowed value of autovacuum_max_workers is one,
the ProcState array guarantees to have at least one entry for autovacuum worker.If this understanding is right, we don't need to enlarge the array and
can just update the comment. I don't strongly oppose to enlarge
the array in the master, but I'm not sure it's worth doing that
in back branches if the issue can cause no actual error.Yes, the issue can't happen. The comment in the SInvalShmemSize,
mentioning about the startup process always having an extra slot
because the autovacuum worker is not active during recovery, looks
okay. But, is it safe to assume that always? Do we have a way to
specify that in the form an Assert(when_i_am_startup_proc &&
autovacuum_not_running) (this looks a bit dirty though)? Instead, we
can just enlarge the array in the master and be confident about the
fact that the startup process always has one dedicated slot.But this slot wont be used for most of cluster life. It will be just
waste.
Correct. In the standby autovacuum launcher and worker are not started
so, the startup process will always have a slot free for it to use.
And `Assert(there_is_startup_proc && autovacuum_not_running)` has
value on its own, hasn't it? So why doesn't add it with comment.
Assertion doesn't make sense to me now. Because the postmaster ensures
that the autovacuum launcher/workers will not get started in standby
mode and we can't reliably know in InitRecoveryTransactionEnvironment
(startup process) whether or not autovacuum launcher process has been
started.
FWIW, here's a patch just adding a comment on how the startup process
can get a free procState array slot even when SInvalShmemSize hasn't
accounted for it.
Regards,
Bharath Rupireddy.
Attachments:
v1-0001-Add-comment-about-startup-process-getting-procSta.patchapplication/octet-stream; name=v1-0001-Add-comment-about-startup-process-getting-procSta.patchDownload
From 44398b0c34f11865aa4e35e6ce17369544940a17 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sat, 12 Feb 2022 11:19:51 +0000
Subject: [PATCH v1] Add comment about startup process getting procState array
slot
---
src/backend/storage/ipc/standby.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 87ac0f74b2..bd41a1f060 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -94,6 +94,14 @@ InitRecoveryTransactionEnvironment(void)
&hash_ctl,
HASH_ELEM | HASH_BLOBS);
+ /*
+ * Here, the startup process is guaranteed to get a free procSatate array
+ * slot, even though SInvalShmemSize has not accounted for it. This is
+ * because autovacuum launcher/worker processes will not get started in
+ * standby mode for which procSatate array slots have already been
+ * allocated.
+ */
+
/*
* Initialize shared invalidation management for Startup process, being
* careful to register ourselves as a sendOnly process so we don't need to
--
2.25.1
В Сб, 12/02/2022 в 16:56 +0530, Bharath Rupireddy пишет:
On Fri, Feb 11, 2022 at 7:56 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote:
В Сб, 16/10/2021 в 16:37 +0530, Bharath Rupireddy пишет:
On Thu, Oct 14, 2021 at 10:56 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:On 2021/10/12 15:46, Bharath Rupireddy wrote:
On Tue, Oct 12, 2021 at 5:37 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/10/12 4:07, Bharath Rupireddy wrote:
Hi,
While working on [1], it is found that currently the ProcState array
doesn't have entries for auxiliary processes, it does have entries for
MaxBackends. But the startup process is eating up one slot from
MaxBackends. We need to increase the size of the ProcState array by 1
at least for the startup process. The startup process uses ProcState
slot via InitRecoveryTransactionEnvironment->SharedInvalBackendInit.
The procState array size is initialized to MaxBackends in
SInvalShmemSize.The consequence of not fixing this issue is that the database may hit
the error "sorry, too many clients already" soon in
SharedInvalBackendInit.On second thought, I wonder if this error could not happen in practice. No?
Because autovacuum doesn't work during recovery and the startup process
can safely use the ProcState entry for autovacuum worker process.
Also since the minimal allowed value of autovacuum_max_workers is one,
the ProcState array guarantees to have at least one entry for autovacuum worker.If this understanding is right, we don't need to enlarge the array and
can just update the comment. I don't strongly oppose to enlarge
the array in the master, but I'm not sure it's worth doing that
in back branches if the issue can cause no actual error.Yes, the issue can't happen. The comment in the SInvalShmemSize,
mentioning about the startup process always having an extra slot
because the autovacuum worker is not active during recovery, looks
okay. But, is it safe to assume that always? Do we have a way to
specify that in the form an Assert(when_i_am_startup_proc &&
autovacuum_not_running) (this looks a bit dirty though)? Instead, we
can just enlarge the array in the master and be confident about the
fact that the startup process always has one dedicated slot.But this slot wont be used for most of cluster life. It will be just
waste.Correct. In the standby autovacuum launcher and worker are not started
so, the startup process will always have a slot free for it to use.And `Assert(there_is_startup_proc && autovacuum_not_running)` has
value on its own, hasn't it? So why doesn't add it with comment.Assertion doesn't make sense to me now. Because the postmaster ensures
that the autovacuum launcher/workers will not get started in standby
mode and we can't reliably know in InitRecoveryTransactionEnvironment
(startup process) whether or not autovacuum launcher process has been
started.FWIW, here's a patch just adding a comment on how the startup process
can get a free procState array slot even when SInvalShmemSize hasn't
accounted for it.
I think, comment is a good thing.
Marked as "Ready for committer".
Show quoted text
Regards,
Bharath Rupireddy.
On Sat, Feb 12, 2022 at 6:26 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
FWIW, here's a patch just adding a comment on how the startup process
can get a free procState array slot even when SInvalShmemSize hasn't
accounted for it.
I don't think the positioning of this code comment is very good,
because it's commenting on 0 lines of code. Perhaps that problem could
be fixed by making it the second paragraph of the immediately
preceding comment instead of a separate block, but I think the right
place to comment on this sort of thing is actually in the code that
sizes the data structure - i.e. SInvalShmemSize. If someone looks at
that function and says "hey, this uses GetMaxBackends(), that's off by
one!" they are not ever going to find this comment explaining the
reasoning.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Sat, Mar 26, 2022 at 1:20 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Feb 12, 2022 at 6:26 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:FWIW, here's a patch just adding a comment on how the startup process
can get a free procState array slot even when SInvalShmemSize hasn't
accounted for it.I don't think the positioning of this code comment is very good,
because it's commenting on 0 lines of code. Perhaps that problem could
be fixed by making it the second paragraph of the immediately
preceding comment instead of a separate block, but I think the right
place to comment on this sort of thing is actually in the code that
sizes the data structure - i.e. SInvalShmemSize. If someone looks at
that function and says "hey, this uses GetMaxBackends(), that's off by
one!" they are not ever going to find this comment explaining the
reasoning.
Thanks. It makes sense to put the comment in SInvalShmemSize.
Attaching v2 patch. Please review it.
Regards,
Bharath Rupireddy.
Attachments:
v2-0001-Add-comment-about-startup-process-getting-procSta.patchapplication/octet-stream; name=v2-0001-Add-comment-about-startup-process-getting-procSta.patchDownload
From 773a3df5331df8e6b90493a5829905be52c71a3c Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sat, 26 Mar 2022 05:42:52 +0000
Subject: [PATCH v2] Add comment about startup process getting procState array
slot
---
src/backend/storage/ipc/sinvaladt.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/backend/storage/ipc/sinvaladt.c b/src/backend/storage/ipc/sinvaladt.c
index 68e7160b30..92d2f50dce 100644
--- a/src/backend/storage/ipc/sinvaladt.c
+++ b/src/backend/storage/ipc/sinvaladt.c
@@ -205,6 +205,15 @@ SInvalShmemSize(void)
Size size;
size = offsetof(SISeg, procState);
+
+ /*
+ * In Hot Standby mode, startup process requests for a free procSatate
+ * array slot, see InitRecoveryTransactionEnvironment(). Even though
+ * MaxBackends doesn't account for the startup process, it is guaranteed to
+ * get a free slot. This is because autovacuum launcher and worker
+ * processes are not started in Hot Standby mode, which MaxBackends would
+ * have already accounted for.
+ */
size = add_size(size, mul_size(sizeof(ProcState), GetMaxBackends()));
return size;
--
2.25.1
On Sat, Mar 26, 2022 at 2:23 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
Thanks. It makes sense to put the comment in SInvalShmemSize.
Attaching v2 patch. Please review it.
How about this version, which I have edited lightly for grammar?
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
sinvalshmemsize-comment-rmh.patchapplication/octet-stream; name=sinvalshmemsize-comment-rmh.patchDownload
diff --git a/src/backend/storage/ipc/sinvaladt.c b/src/backend/storage/ipc/sinvaladt.c
index 68e7160b30..2dec668bbc 100644
--- a/src/backend/storage/ipc/sinvaladt.c
+++ b/src/backend/storage/ipc/sinvaladt.c
@@ -205,6 +205,14 @@ SInvalShmemSize(void)
Size size;
size = offsetof(SISeg, procState);
+
+ /*
+ * In Hot Standby mode, the startup process requests a procState array
+ * slot using InitRecoveryTransactionEnvironment(). Even though MaxBackends
+ * doesn't account for the startup process, it is guaranteed to get a
+ * free slot. This is because the autovacuum launcher and worker processes,
+ * which are included in MaxBackends, are not started in Hot Standby mode.
+ */
size = add_size(size, mul_size(sizeof(ProcState), GetMaxBackends()));
return size;
On Tue, Mar 29, 2022 at 12:47 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Mar 26, 2022 at 2:23 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:Thanks. It makes sense to put the comment in SInvalShmemSize.
Attaching v2 patch. Please review it.How about this version, which I have edited lightly for grammar?
Thanks. LGTM.
Regards,
Bharath Rupireddy.
On Tue, Mar 29, 2022 at 3:21 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
Thanks. LGTM.
Committed.
--
Robert Haas
EDB: http://www.enterprisedb.com