Re: almost-super-user problems that we haven't fixed yet
On Fri, Jan 20, 2023 at 4:02 PM Nathan Bossart wrote: > On Fri, Jan 20, 2023 at 03:42:03PM -0500, Robert Haas wrote: > > Thanks to you both. I have committed these patches. > > Thanks! Does this need a catversion bump? I was surprised by this question because I thought I'd included one. But it turns out I didn't include that in the commit and it's still in my working tree. *facepalm* -- Robert Haas EDB: http://www.enterprisedb.com
Re: almost-super-user problems that we haven't fixed yet
On Fri, Jan 20, 2023 at 03:42:03PM -0500, Robert Haas wrote: > Thanks to you both. I have committed these patches. Thanks! Does this need a catversion bump? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: almost-super-user problems that we haven't fixed yet
On Fri, Jan 20, 2023 at 1:10 PM Nathan Bossart wrote: > > Thanks, this is fixed now with the latest patches. > > Thank you for reviewing. Thanks to you both. I have committed these patches. -- Robert Haas EDB: http://www.enterprisedb.com
Re: almost-super-user problems that we haven't fixed yet
On Fri, Jan 20, 2023 at 07:04:58PM +0530, tushar wrote: > On 1/19/23 6:28 PM, tushar wrote: >> There is one typo , for the doc changes, it is mentioned >> "pg_use_reserved_backends" but i think it supposed to be >> "pg_use_reserved_connections" >> under Table 22.1. Predefined Roles. > > Thanks, this is fixed now with the latest patches. Thank you for reviewing. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: almost-super-user problems that we haven't fixed yet
On 1/19/23 6:28 PM, tushar wrote: There is one typo , for the doc changes, it is mentioned "pg_use_reserved_backends" but i think it supposed to be "pg_use_reserved_connections" under Table 22.1. Predefined Roles. Thanks, this is fixed now with the latest patches. -- regards,tushar EnterpriseDB https://www.enterprisedb.com/ The Enterprise PostgreSQL Company
Re: almost-super-user problems that we haven't fixed yet
On Thu, Jan 19, 2023 at 2:46 PM Nathan Bossart wrote: > > Thanks. I'd move it to the inner indentation level so it's closer to > > the test at issue. > > I meant for it to cover the call to HaveNFreeProcs() as well since the same > idea applies. I left it the same for now, but if you still think it makes > sense to move it, I'll do so. Hmm, OK. If you want to leave it where it is, I won't argue further. -- Robert Haas EDB: http://www.enterprisedb.com
Re: almost-super-user problems that we haven't fixed yet
On Thu, Jan 19, 2023 at 02:17:35PM -0500, Robert Haas wrote:
> On Thu, Jan 19, 2023 at 12:54 PM Nathan Bossart
> wrote:
>> > OK. Might be worth a short comment.
>>
>> I added one.
>
> Thanks. I'd move it to the inner indentation level so it's closer to
> the test at issue.
I meant for it to cover the call to HaveNFreeProcs() as well since the same
idea applies. I left it the same for now, but if you still think it makes
sense to move it, I'll do so.
> I would also suggest reordering the documentation and the
> postgresql.conf.sample file so that reserved_connections precedes
> superuser_reserved_connections, instead of following it.
Makes sense.
> Other than that, this seems like it might be about ready to commit,
> barring objections or bug reports.
Awesome.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From a6811b643df94c9057373fd687398c85a807fd5e Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 18 Jan 2023 12:43:41 -0800
Subject: [PATCH v5 1/3] Code review for ea92368.
This commit missed an error message and a line in the docs.
---
doc/src/sgml/config.sgml | 3 +--
src/backend/utils/init/postinit.c | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 89d53f2a64..e019a1aac9 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -725,8 +725,7 @@ include_dir 'conf.d'
number of active concurrent connections is at least
max_connections minus
superuser_reserved_connections, new
-connections will be accepted only for superusers, and no
-new replication connections will be accepted.
+connections will be accepted only for superusers.
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index ae5a85ed65..9145d96b38 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -931,7 +931,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
!HaveNFreeProcs(ReservedBackends))
ereport(FATAL,
(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
- errmsg("remaining connection slots are reserved for non-replication superuser connections")));
+ errmsg("remaining connection slots are reserved for superusers")));
/* Check replication permissions needed for walsender processes. */
if (am_walsender)
--
2.25.1
>From e0390f0120315746ea04a9fa1bf709def76e6196 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 17 Jan 2023 13:58:56 -0800
Subject: [PATCH v5 2/3] Rename ReservedBackends to
SuperuserReservedConnections.
This is in preparation for adding a new reserved_connections GUC.
---
src/backend/postmaster/postmaster.c | 20 ++--
src/backend/utils/init/postinit.c | 4 ++--
src/backend/utils/misc/guc_tables.c | 2 +-
src/include/postmaster/postmaster.h | 2 +-
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 9cedc1b9f0..3f799c4ac8 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -204,15 +204,15 @@ char *Unix_socket_directories;
char *ListenAddresses;
/*
- * ReservedBackends is the number of backends reserved for superuser use.
- * This number is taken out of the pool size given by MaxConnections so
- * number of backend slots available to non-superusers is
- * (MaxConnections - ReservedBackends). Note what this really means is
- * "if there are <= ReservedBackends connections available, only superusers
- * can make new connections" --- pre-existing superuser connections don't
- * count against the limit.
+ * SuperuserReservedConnections is the number of backends reserved for
+ * superuser use. This number is taken out of the pool size given by
+ * MaxConnections so number of backend slots available to non-superusers is
+ * (MaxConnections - SuperuserReservedConnections). Note what this really
+ * means is "if there are <= SuperuserReservedConnections connections
+ * available, only superusers can make new connections" --- pre-existing
+ * superuser connections don't count against the limit.
*/
-int ReservedBackends;
+int SuperuserReservedConnections;
/* The socket(s) we're listening to. */
#define MAXLISTEN 64
@@ -908,11 +908,11 @@ PostmasterMain(int argc, char *argv[])
/*
* Check for invalid combinations of GUC settings.
*/
- if (ReservedBackends >= MaxConnections)
+ if (SuperuserReservedConnections >= MaxConnections)
{
write_stderr("%s: superuser_reserved_connections (%d) must be less than max_connections (%d)\n",
progname,
- ReservedBackends, MaxConnections);
+ SuperuserReservedConnections, MaxConnections);
ExitPostmaster(1);
}
if (XLogArchiveMode > ARCHIVE_MODE_OFF && wal_level == WAL_LEVEL_MINIMAL)
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 9145d96b38..40f145e0ab 100644
---
Re: almost-super-user problems that we haven't fixed yet
On Thu, Jan 19, 2023 at 12:54 PM Nathan Bossart wrote: > > OK. Might be worth a short comment. > > I added one. Thanks. I'd move it to the inner indentation level so it's closer to the test at issue. I would also suggest reordering the documentation and the postgresql.conf.sample file so that reserved_connections precedes superuser_reserved_connections, instead of following it. Other than that, this seems like it might be about ready to commit, barring objections or bug reports. -- Robert Haas EDB: http://www.enterprisedb.com
Re: almost-super-user problems that we haven't fixed yet
On Thu, Jan 19, 2023 at 11:40:53AM -0500, Robert Haas wrote:
> On Wed, Jan 18, 2023 at 4:14 PM Nathan Bossart
> wrote:
>> On Wed, Jan 18, 2023 at 02:51:38PM -0500, Robert Haas wrote:
>> > Should (nfree < SuperuserReservedBackends) be using <=, or am I confused?
>>
>> I believe < is correct. At this point, the new backend will have already
>> claimed a proc struct, so if the number of remaining free slots equals the
>> number of reserved slots, it is okay.
>
> OK. Might be worth a short comment.
I added one.
>> > What's the deal with removing "and no new replication connections will
>> > be accepted" from the documentation? Is the existing documentation
>> > just wrong? If so, should we fix that first? And maybe delete
>> > "non-replication" from the error message that says "remaining
>> > connection slots are reserved for non-replication superuser
>> > connections"? It seems like right now the comments say that
>> > replication connections are a completely separate pool of connections,
>> > but the documentation and the error message make it sound otherwise.
>> > If that's true, then one of them is wrong, and I think it's the
>> > docs/error message. Or am I just misreading it?
>>
>> I think you are right. This seems to have been missed in ea92368. I moved
>> this part to a new patch that should probably be back-patched to v12.
>
> I'm inclined to commit it to master and not back-patch. It doesn't
> seem important enough to perturb translations.
That seems reasonable to me.
> Tushar seems to have a point about pg_use_reserved_connections vs.
> pg_use_reserved_backends. I think we should standardize on the former,
> as backends is an internal term.
Oops. This is what I meant to do. I probably flubbed it because I was
wondering why the parameter uses "connections" and the variable uses
"backends," especially considering that the variable for max_connections is
called MaxConnections. I went ahead and renamed everything to use
"connections."
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 7be7e70aaf488a924d61b21b351a3b4f7e33aedc Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 18 Jan 2023 12:43:41 -0800
Subject: [PATCH v4 1/3] Code review for ea92368.
This commit missed an error message and a line in the docs.
---
doc/src/sgml/config.sgml | 3 +--
src/backend/utils/init/postinit.c | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 89d53f2a64..e019a1aac9 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -725,8 +725,7 @@ include_dir 'conf.d'
number of active concurrent connections is at least
max_connections minus
superuser_reserved_connections, new
-connections will be accepted only for superusers, and no
-new replication connections will be accepted.
+connections will be accepted only for superusers.
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index ae5a85ed65..9145d96b38 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -931,7 +931,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
!HaveNFreeProcs(ReservedBackends))
ereport(FATAL,
(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
- errmsg("remaining connection slots are reserved for non-replication superuser connections")));
+ errmsg("remaining connection slots are reserved for superusers")));
/* Check replication permissions needed for walsender processes. */
if (am_walsender)
--
2.25.1
>From 058df2b3dcf50ecbe76c794f4f52751e6a9f765f Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 17 Jan 2023 13:58:56 -0800
Subject: [PATCH v4 2/3] Rename ReservedBackends to
SuperuserReservedConnections.
This is in preparation for adding a new reserved_connections GUC.
---
src/backend/postmaster/postmaster.c | 20 ++--
src/backend/utils/init/postinit.c | 4 ++--
src/backend/utils/misc/guc_tables.c | 2 +-
src/include/postmaster/postmaster.h | 2 +-
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 9cedc1b9f0..3f799c4ac8 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -204,15 +204,15 @@ char *Unix_socket_directories;
char *ListenAddresses;
/*
- * ReservedBackends is the number of backends reserved for superuser use.
- * This number is taken out of the pool size given by MaxConnections so
- * number of backend slots available to non-superusers is
- * (MaxConnections - ReservedBackends). Note what this really means is
- * "if there are <= ReservedBackends connections available, only superusers
- * can make new connections" --- pre-existing superuser connections don't
- * count against the limit.
+ * SuperuserReservedConnections is the number of backends reserved for
+ *
Re: almost-super-user problems that we haven't fixed yet
On Wed, Jan 18, 2023 at 4:14 PM Nathan Bossart wrote: > On Wed, Jan 18, 2023 at 02:51:38PM -0500, Robert Haas wrote: > > Should (nfree < SuperuserReservedBackends) be using <=, or am I confused? > > I believe < is correct. At this point, the new backend will have already > claimed a proc struct, so if the number of remaining free slots equals the > number of reserved slots, it is okay. OK. Might be worth a short comment. > > What's the deal with removing "and no new replication connections will > > be accepted" from the documentation? Is the existing documentation > > just wrong? If so, should we fix that first? And maybe delete > > "non-replication" from the error message that says "remaining > > connection slots are reserved for non-replication superuser > > connections"? It seems like right now the comments say that > > replication connections are a completely separate pool of connections, > > but the documentation and the error message make it sound otherwise. > > If that's true, then one of them is wrong, and I think it's the > > docs/error message. Or am I just misreading it? > > I think you are right. This seems to have been missed in ea92368. I moved > this part to a new patch that should probably be back-patched to v12. I'm inclined to commit it to master and not back-patch. It doesn't seem important enough to perturb translations. Tushar seems to have a point about pg_use_reserved_connections vs. pg_use_reserved_backends. I think we should standardize on the former, as backends is an internal term. -- Robert Haas EDB: http://www.enterprisedb.com
Re: almost-super-user problems that we haven't fixed yet
On Thu, Jan 19, 2023 at 9:21 AM tushar wrote: > that is not true because the superuser can still able to connect, It is true, but because superusers have all privileges. -- Robert Haas EDB: http://www.enterprisedb.com
Re: almost-super-user problems that we haven't fixed yet
On Thu, Jan 19, 2023 at 6:50 PM tushar wrote: > and in the error message too > > [edb@centos7tushar bin]$ ./psql postgres -U r2 > > psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: > FATAL: remaining connection slots are reserved for roles with privileges > of pg_use_reserved_backends > [edb@centos7tushar bin]$ > I think there is also a need to improve the error message if non super users are not able to connect due to slot unavailability. --Connect to psql terminal, create a user create user t1; --set these GUC parameters in postgresql.conf and restart the server max_connections = 3 # (change requires restart) superuser_reserved_connections = 1 # (change requires restart) reserved_connections = 1 psql terminal ( connect to superuser), ./psql postgres psql terminal (try to connect to user t1) , ./psql postgres -U t1 Error message is psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: FATAL: remaining connection slots are reserved for roles with privileges of pg_use_reserved_backends that is not true because the superuser can still able to connect, probably in this case message should be like this - "remaining connection slots are reserved for roles with privileges of pg_use_reserved_connections and for superusers" or something better. regards,
Re: almost-super-user problems that we haven't fixed yet
On Thu, Jan 19, 2023 at 6:28 PM tushar wrote: > On 1/19/23 2:44 AM, Nathan Bossart wrote: > > On Wed, Jan 18, 2023 at 02:51:38PM -0500, Robert Haas wrote: > >> Should (nfree < SuperuserReservedBackends) be using <=, or am I > confused? > > I believe < is correct. At this point, the new backend will have already > > claimed a proc struct, so if the number of remaining free slots equals > the > > number of reserved slots, it is okay. > > > >> What's the deal with removing "and no new replication connections will > >> be accepted" from the documentation? Is the existing documentation > >> just wrong? If so, should we fix that first? And maybe delete > >> "non-replication" from the error message that says "remaining > >> connection slots are reserved for non-replication superuser > >> connections"? It seems like right now the comments say that > >> replication connections are a completely separate pool of connections, > >> but the documentation and the error message make it sound otherwise. > >> If that's true, then one of them is wrong, and I think it's the > >> docs/error message. Or am I just misreading it? > > I think you are right. This seems to have been missed in ea92368. I > moved > > this part to a new patch that should probably be back-patched to v12. > > > > On that note, I wonder if it's worth changing the "sorry, too many > clients > > already" message to make it clear that max_connections has been reached. > > IME some users are confused by this error, and I think it would be less > > confusing if it pointed to the parameter that governs the number of > > connection slots. I'll create a new thread for this. > > > There is one typo , for the doc changes, it is mentioned > "pg_use_reserved_backends" but i think it supposed to be > "pg_use_reserved_connections" > under Table 22.1. Predefined Roles. > > and in the error message too [edb@centos7tushar bin]$ ./psql postgres -U r2 psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: FATAL: remaining connection slots are reserved for roles with privileges of pg_use_reserved_backends [edb@centos7tushar bin]$ regards,
Re: almost-super-user problems that we haven't fixed yet
On 1/19/23 2:44 AM, Nathan Bossart wrote: On Wed, Jan 18, 2023 at 02:51:38PM -0500, Robert Haas wrote: Should (nfree < SuperuserReservedBackends) be using <=, or am I confused? I believe < is correct. At this point, the new backend will have already claimed a proc struct, so if the number of remaining free slots equals the number of reserved slots, it is okay. What's the deal with removing "and no new replication connections will be accepted" from the documentation? Is the existing documentation just wrong? If so, should we fix that first? And maybe delete "non-replication" from the error message that says "remaining connection slots are reserved for non-replication superuser connections"? It seems like right now the comments say that replication connections are a completely separate pool of connections, but the documentation and the error message make it sound otherwise. If that's true, then one of them is wrong, and I think it's the docs/error message. Or am I just misreading it? I think you are right. This seems to have been missed in ea92368. I moved this part to a new patch that should probably be back-patched to v12. On that note, I wonder if it's worth changing the "sorry, too many clients already" message to make it clear that max_connections has been reached. IME some users are confused by this error, and I think it would be less confusing if it pointed to the parameter that governs the number of connection slots. I'll create a new thread for this. There is one typo , for the doc changes, it is mentioned "pg_use_reserved_backends" but i think it supposed to be "pg_use_reserved_connections" under Table 22.1. Predefined Roles. -- regards,tushar EnterpriseDB https://www.enterprisedb.com/ The Enterprise PostgreSQL Company
Re: almost-super-user problems that we haven't fixed yet
On Wed, Jan 18, 2023 at 02:51:38PM -0500, Robert Haas wrote:
> Should (nfree < SuperuserReservedBackends) be using <=, or am I confused?
I believe < is correct. At this point, the new backend will have already
claimed a proc struct, so if the number of remaining free slots equals the
number of reserved slots, it is okay.
> What's the deal with removing "and no new replication connections will
> be accepted" from the documentation? Is the existing documentation
> just wrong? If so, should we fix that first? And maybe delete
> "non-replication" from the error message that says "remaining
> connection slots are reserved for non-replication superuser
> connections"? It seems like right now the comments say that
> replication connections are a completely separate pool of connections,
> but the documentation and the error message make it sound otherwise.
> If that's true, then one of them is wrong, and I think it's the
> docs/error message. Or am I just misreading it?
I think you are right. This seems to have been missed in ea92368. I moved
this part to a new patch that should probably be back-patched to v12.
On that note, I wonder if it's worth changing the "sorry, too many clients
already" message to make it clear that max_connections has been reached.
IME some users are confused by this error, and I think it would be less
confusing if it pointed to the parameter that governs the number of
connection slots. I'll create a new thread for this.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 8f0aa2fa54ae01149cffe9a69265f98e76a08a23 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 18 Jan 2023 12:43:41 -0800
Subject: [PATCH v3 1/3] Code review for ea92368.
This commit missed an error message and a line in the docs.
Back-patch to v12.
---
doc/src/sgml/config.sgml | 3 +--
src/backend/utils/init/postinit.c | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 89d53f2a64..e019a1aac9 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -725,8 +725,7 @@ include_dir 'conf.d'
number of active concurrent connections is at least
max_connections minus
superuser_reserved_connections, new
-connections will be accepted only for superusers, and no
-new replication connections will be accepted.
+connections will be accepted only for superusers.
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index ae5a85ed65..9145d96b38 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -931,7 +931,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
!HaveNFreeProcs(ReservedBackends))
ereport(FATAL,
(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
- errmsg("remaining connection slots are reserved for non-replication superuser connections")));
+ errmsg("remaining connection slots are reserved for superusers")));
/* Check replication permissions needed for walsender processes. */
if (am_walsender)
--
2.25.1
>From bc110461e4f0a73c2b76ba0a6e821349c2cbe3df Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 17 Jan 2023 13:58:56 -0800
Subject: [PATCH v3 2/3] Rename ReservedBackends to SuperuserReservedBackends.
This is in preparation for adding a new reserved_connections GUC
that will use the ReservedBackends variable name.
---
src/backend/postmaster/postmaster.c | 18 +-
src/backend/utils/init/postinit.c | 4 ++--
src/backend/utils/misc/guc_tables.c | 2 +-
src/include/postmaster/postmaster.h | 2 +-
4 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 9cedc1b9f0..470704f364 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -204,15 +204,15 @@ char *Unix_socket_directories;
char *ListenAddresses;
/*
- * ReservedBackends is the number of backends reserved for superuser use.
- * This number is taken out of the pool size given by MaxConnections so
+ * SuperuserReservedBackends is the number of backends reserved for superuser
+ * use. This number is taken out of the pool size given by MaxConnections so
* number of backend slots available to non-superusers is
- * (MaxConnections - ReservedBackends). Note what this really means is
- * "if there are <= ReservedBackends connections available, only superusers
- * can make new connections" --- pre-existing superuser connections don't
- * count against the limit.
+ * (MaxConnections - SuperuserReservedBackends). Note what this really means
+ * is "if there are <= SuperuserReservedBackends connections available, only
+ * superusers can make new connections" --- pre-existing superuser connections
+ * don't count against the limit.
*/
-int ReservedBackends;
+int SuperuserReservedBackends;
/* The socket(s) we're listening to. */
Re: almost-super-user problems that we haven't fixed yet
On Wed, Jan 18, 2023 at 2:00 PM Nathan Bossart wrote: > On Wed, Jan 18, 2023 at 11:28:57AM -0500, Robert Haas wrote: > > In general, looks good. I think this will often call HaveNFreeProcs > > twice, though, and that would be better to avoid, e.g. > > I should have thought of this. This is fixed in v2. Should (nfree < SuperuserReservedBackends) be using <=, or am I confused? > > In the common case where we hit neither limit, this only counts free > > connection slots once. We could do even better by making > > HaveNFreeProcs have an out parameter for the number of free procs > > actually found when it returns false, but that's probably not > > important. > > Actually, I think it might be important. IIUC the separate calls to > HaveNFreeProcs might return different values for the same input, which > could result in incorrect error messages (e.g., you might get the > reserved_connections message despite setting reserved_connections to 0). > So, I made this change in v2, too. I thought of that briefly and it didn't seem that important, but the way you did it seems fine, so let's go with that. What's the deal with removing "and no new replication connections will be accepted" from the documentation? Is the existing documentation just wrong? If so, should we fix that first? And maybe delete "non-replication" from the error message that says "remaining connection slots are reserved for non-replication superuser connections"? It seems like right now the comments say that replication connections are a completely separate pool of connections, but the documentation and the error message make it sound otherwise. If that's true, then one of them is wrong, and I think it's the docs/error message. Or am I just misreading it? -- Robert Haas EDB: http://www.enterprisedb.com
Re: almost-super-user problems that we haven't fixed yet
On Wed, Jan 18, 2023 at 11:28:57AM -0500, Robert Haas wrote:
> In general, looks good. I think this will often call HaveNFreeProcs
> twice, though, and that would be better to avoid, e.g.
I should have thought of this. This is fixed in v2.
> In the common case where we hit neither limit, this only counts free
> connection slots once. We could do even better by making
> HaveNFreeProcs have an out parameter for the number of free procs
> actually found when it returns false, but that's probably not
> important.
Actually, I think it might be important. IIUC the separate calls to
HaveNFreeProcs might return different values for the same input, which
could result in incorrect error messages (e.g., you might get the
reserved_connections message despite setting reserved_connections to 0).
So, I made this change in v2, too.
> I don't think that we should default both the existing GUC and the new
> one to 3, because that raises the default limit in the case where the
> new feature is not used from 3 to 6. I think we should default one of
> them to 0 and the other one to 3. Not sure which one should get which
> value.
I chose to set reserved_connections to 0 since it is new and doesn't have a
pre-existing default value.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 94ee89548fd1080447b784993a1418480f407b49 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 17 Jan 2023 13:58:56 -0800
Subject: [PATCH v2 1/2] Rename ReservedBackends to SuperuserReservedBackends.
This is in preparation for adding a new reserved_connections GUC
that will use the ReservedBackends variable name.
---
src/backend/postmaster/postmaster.c | 18 +-
src/backend/utils/init/postinit.c | 4 ++--
src/backend/utils/misc/guc_tables.c | 2 +-
src/include/postmaster/postmaster.h | 2 +-
4 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 9cedc1b9f0..470704f364 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -204,15 +204,15 @@ char *Unix_socket_directories;
char *ListenAddresses;
/*
- * ReservedBackends is the number of backends reserved for superuser use.
- * This number is taken out of the pool size given by MaxConnections so
+ * SuperuserReservedBackends is the number of backends reserved for superuser
+ * use. This number is taken out of the pool size given by MaxConnections so
* number of backend slots available to non-superusers is
- * (MaxConnections - ReservedBackends). Note what this really means is
- * "if there are <= ReservedBackends connections available, only superusers
- * can make new connections" --- pre-existing superuser connections don't
- * count against the limit.
+ * (MaxConnections - SuperuserReservedBackends). Note what this really means
+ * is "if there are <= SuperuserReservedBackends connections available, only
+ * superusers can make new connections" --- pre-existing superuser connections
+ * don't count against the limit.
*/
-int ReservedBackends;
+int SuperuserReservedBackends;
/* The socket(s) we're listening to. */
#define MAXLISTEN 64
@@ -908,11 +908,11 @@ PostmasterMain(int argc, char *argv[])
/*
* Check for invalid combinations of GUC settings.
*/
- if (ReservedBackends >= MaxConnections)
+ if (SuperuserReservedBackends >= MaxConnections)
{
write_stderr("%s: superuser_reserved_connections (%d) must be less than max_connections (%d)\n",
progname,
- ReservedBackends, MaxConnections);
+ SuperuserReservedBackends, MaxConnections);
ExitPostmaster(1);
}
if (XLogArchiveMode > ARCHIVE_MODE_OFF && wal_level == WAL_LEVEL_MINIMAL)
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index ae5a85ed65..6fa696fe8d 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -927,8 +927,8 @@ InitPostgres(const char *in_dbname, Oid dboid,
* limited by max_connections or superuser_reserved_connections.
*/
if (!am_superuser && !am_walsender &&
- ReservedBackends > 0 &&
- !HaveNFreeProcs(ReservedBackends))
+ SuperuserReservedBackends > 0 &&
+ !HaveNFreeProcs(SuperuserReservedBackends))
ereport(FATAL,
(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
errmsg("remaining connection slots are reserved for non-replication superuser connections")));
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 5025e80f89..5aa2cda8f9 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2163,7 +2163,7 @@ struct config_int ConfigureNamesInt[] =
gettext_noop("Sets the number of connection slots reserved for superusers."),
NULL
},
- ,
+ ,
3, 0, MAX_BACKENDS,
NULL, NULL, NULL
},
diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h
index 203177e1ff..168d85a3d1 100644
---
Re: almost-super-user problems that we haven't fixed yet
On Tue, Jan 17, 2023 at 7:15 PM Nathan Bossart wrote:
> Great. Here is a first attempt at the patch.
In general, looks good. I think this will often call HaveNFreeProcs
twice, though, and that would be better to avoid, e.g.
if (!am_superuser && !am_walsender && (SuperuserReservedBackends +
ReservedBackends) > 0)
&& !HaveNFreeProcs(SuperuserReservedBackends + ReservedBackends))
{
if (!HaveNFreeProcs(SuperuserReservedBackends))
remaining connection slots are reserved for non-replication
superuser connections;
if (!has_privs_of_role(GetUserId(), ROLE_PG_USE_RESERVED_CONNECTIONS))
remaining connection slots are reserved for roles with
privileges of pg_use_reserved_backends;
}
In the common case where we hit neither limit, this only counts free
connection slots once. We could do even better by making
HaveNFreeProcs have an out parameter for the number of free procs
actually found when it returns false, but that's probably not
important.
I don't think that we should default both the existing GUC and the new
one to 3, because that raises the default limit in the case where the
new feature is not used from 3 to 6. I think we should default one of
them to 0 and the other one to 3. Not sure which one should get which
value.
> > I think the documentation will need some careful wordsmithing,
> > including adjustments to superuser_reserved_connections. We want to
> > recast superuser_reserved_connections as a final reserve to be touched
> > after even reserved_connections has been exhausted.
>
> I tried to do this, but there is probably still room for improvement,
> especially for the parts that discuss the relationship between
> max_connections, superuser_reserved_connections, and reserved_connections.
I think it's pretty good the way you have it. I agree that there might
be a way to make it even better, but I don't think I know what it is.
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: almost-super-user problems that we haven't fixed yet
On Tue, Jan 17, 2023 at 02:59:31PM -0500, Robert Haas wrote:
> On Tue, Jan 17, 2023 at 1:42 PM Nathan Bossart
> wrote:
>> If we create a new batch of reserved connections, only roles with
>> privileges of pg_use_reserved_connections would be able to connect if the
>> number of remaining slots is greater than superuser_reserved_connections
>> but less than or equal to superuser_reserved_connections +
>> reserved_connections. Only superusers would be able to connect if the
>> number of remaining slots is less than or equal to
>> superuser_reserved_connections. This helps avoid blocking new superuser
>> connections even if you've reserved some connections for non-superusers.
>
> This is precisely what I had in mind.
Great. Here is a first attempt at the patch.
> I think the documentation will need some careful wordsmithing,
> including adjustments to superuser_reserved_connections. We want to
> recast superuser_reserved_connections as a final reserve to be touched
> after even reserved_connections has been exhausted.
I tried to do this, but there is probably still room for improvement,
especially for the parts that discuss the relationship between
max_connections, superuser_reserved_connections, and reserved_connections.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From e153d2f22d4303d0bb5e8134391ebf1fa446172c Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 17 Jan 2023 13:58:56 -0800
Subject: [PATCH v1 1/2] Rename ReservedBackends to SuperuserReservedBackends.
This is in preparation for adding a new reserved_connections GUC
that will use the ReservedBackends variable name.
---
src/backend/postmaster/postmaster.c | 18 +-
src/backend/utils/init/postinit.c | 4 ++--
src/backend/utils/misc/guc_tables.c | 2 +-
src/include/postmaster/postmaster.h | 2 +-
4 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 9cedc1b9f0..470704f364 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -204,15 +204,15 @@ char *Unix_socket_directories;
char *ListenAddresses;
/*
- * ReservedBackends is the number of backends reserved for superuser use.
- * This number is taken out of the pool size given by MaxConnections so
+ * SuperuserReservedBackends is the number of backends reserved for superuser
+ * use. This number is taken out of the pool size given by MaxConnections so
* number of backend slots available to non-superusers is
- * (MaxConnections - ReservedBackends). Note what this really means is
- * "if there are <= ReservedBackends connections available, only superusers
- * can make new connections" --- pre-existing superuser connections don't
- * count against the limit.
+ * (MaxConnections - SuperuserReservedBackends). Note what this really means
+ * is "if there are <= SuperuserReservedBackends connections available, only
+ * superusers can make new connections" --- pre-existing superuser connections
+ * don't count against the limit.
*/
-int ReservedBackends;
+int SuperuserReservedBackends;
/* The socket(s) we're listening to. */
#define MAXLISTEN 64
@@ -908,11 +908,11 @@ PostmasterMain(int argc, char *argv[])
/*
* Check for invalid combinations of GUC settings.
*/
- if (ReservedBackends >= MaxConnections)
+ if (SuperuserReservedBackends >= MaxConnections)
{
write_stderr("%s: superuser_reserved_connections (%d) must be less than max_connections (%d)\n",
progname,
- ReservedBackends, MaxConnections);
+ SuperuserReservedBackends, MaxConnections);
ExitPostmaster(1);
}
if (XLogArchiveMode > ARCHIVE_MODE_OFF && wal_level == WAL_LEVEL_MINIMAL)
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index ae5a85ed65..6fa696fe8d 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -927,8 +927,8 @@ InitPostgres(const char *in_dbname, Oid dboid,
* limited by max_connections or superuser_reserved_connections.
*/
if (!am_superuser && !am_walsender &&
- ReservedBackends > 0 &&
- !HaveNFreeProcs(ReservedBackends))
+ SuperuserReservedBackends > 0 &&
+ !HaveNFreeProcs(SuperuserReservedBackends))
ereport(FATAL,
(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
errmsg("remaining connection slots are reserved for non-replication superuser connections")));
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 5025e80f89..5aa2cda8f9 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2163,7 +2163,7 @@ struct config_int ConfigureNamesInt[] =
gettext_noop("Sets the number of connection slots reserved for superusers."),
NULL
},
- ,
+ ,
3, 0, MAX_BACKENDS,
NULL, NULL, NULL
},
diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h
index 203177e1ff..168d85a3d1 100644
---
Re: almost-super-user problems that we haven't fixed yet
On Tue, Jan 17, 2023 at 1:42 PM Nathan Bossart wrote: > Alright. The one design question I have is whether this should be a new > set of reserved connections or replace superuser_reserved_connections > entirely. I think it should definitely be something new, not a replacement. > If we create a new batch of reserved connections, only roles with > privileges of pg_use_reserved_connections would be able to connect if the > number of remaining slots is greater than superuser_reserved_connections > but less than or equal to superuser_reserved_connections + > reserved_connections. Only superusers would be able to connect if the > number of remaining slots is less than or equal to > superuser_reserved_connections. This helps avoid blocking new superuser > connections even if you've reserved some connections for non-superusers. This is precisely what I had in mind. I think the documentation will need some careful wordsmithing, including adjustments to superuser_reserved_connections. We want to recast superuser_reserved_connections as a final reserve to be touched after even reserved_connections has been exhausted. -- Robert Haas EDB: http://www.enterprisedb.com
Re: almost-super-user problems that we haven't fixed yet
On Mon, Jan 16, 2023 at 09:06:10PM -0500, Robert Haas wrote: > On Mon, Jan 16, 2023 at 5:37 PM Nathan Bossart > wrote: >> On Mon, Jan 16, 2023 at 02:29:56PM -0500, Robert Haas wrote: >> > 4. You can reserve a small number of connections for the superuser >> > with superuser_reserved_connections, but there's no way to do a >> > similar thing for any other user. As mentioned above, a CREATEROLE >> > user could set connection limits for every created role such that the >> > sum of those limits is less than max_connections by some margin, but >> > that restricts each of those roles individually, not all of them in >> > the aggregate. Maybe we could address this by inventing a new GUC >> > reserved_connections and a predefined role >> > pg_use_reserved_connections. >> >> I've written something like this before, and I'd be happy to put together a >> patch if there is interest. > > Cool. I had been thinking of coding it up myself, but you doing it works, too. Alright. The one design question I have is whether this should be a new set of reserved connections or replace superuser_reserved_connections entirely. If we create a new batch of reserved connections, only roles with privileges of pg_use_reserved_connections would be able to connect if the number of remaining slots is greater than superuser_reserved_connections but less than or equal to superuser_reserved_connections + reserved_connections. Only superusers would be able to connect if the number of remaining slots is less than or equal to superuser_reserved_connections. This helps avoid blocking new superuser connections even if you've reserved some connections for non-superusers. Іf we replace superuser_reserved_connections, we're basically opening up the existing functionality to non-superusers, which is simpler and probably more in the spirit of this thread, but it doesn't provide a way to prevent blocking new superuser connections. My preference is the former approach. This is closest to what I've written before, and if I read your words carefully, it seems to be what you are proposing. WDYT? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: almost-super-user problems that we haven't fixed yet
On Mon, Jan 16, 2023 at 5:37 PM Nathan Bossart wrote: > On Mon, Jan 16, 2023 at 02:29:56PM -0500, Robert Haas wrote: > > 4. You can reserve a small number of connections for the superuser > > with superuser_reserved_connections, but there's no way to do a > > similar thing for any other user. As mentioned above, a CREATEROLE > > user could set connection limits for every created role such that the > > sum of those limits is less than max_connections by some margin, but > > that restricts each of those roles individually, not all of them in > > the aggregate. Maybe we could address this by inventing a new GUC > > reserved_connections and a predefined role > > pg_use_reserved_connections. > > I've written something like this before, and I'd be happy to put together a > patch if there is interest. Cool. I had been thinking of coding it up myself, but you doing it works, too. -- Robert Haas EDB: http://www.enterprisedb.com
Re: almost-super-user problems that we haven't fixed yet
On Mon, Jan 16, 2023 at 02:29:56PM -0500, Robert Haas wrote: > 4. You can reserve a small number of connections for the superuser > with superuser_reserved_connections, but there's no way to do a > similar thing for any other user. As mentioned above, a CREATEROLE > user could set connection limits for every created role such that the > sum of those limits is less than max_connections by some margin, but > that restricts each of those roles individually, not all of them in > the aggregate. Maybe we could address this by inventing a new GUC > reserved_connections and a predefined role > pg_use_reserved_connections. I've written something like this before, and I'd be happy to put together a patch if there is interest. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
