On Sun, 13 Oct 2013 11:38:17 +0530
Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Thu, Oct 10, 2013 at 3:17 AM, Gibheer <gibh...@zero-knowledge.org>
> wrote:
> > On Mon, 7 Oct 2013 11:39:55 +0530
> > Amit Kapila <amit.kapil...@gmail.com> wrote:
> >> Robert Haas wrote:
> >> On Mon, Aug 5, 2013 at 2:04 AM, Andres Freund
> >> <andres(at)2ndquadrant(dot)com> wrote:
> >> >>> Hmm. It seems like this match is making MaxConnections no
> >> >>> longer mean the maximum number of connections, but rather the
> >> >>> maximum number of non-replication connections. I don't think
> >> >>> I support that definitional change, and I'm kinda surprised if
> >> >>> this is sufficient to implement it anyway (e.g. see
> >> >>> InitProcGlobal()).
> >> >
> >> >> I don't think the implementation is correct, but why don't you
> >> >> like the definitional change? The set of things you can do from
> >> >> replication connections are completely different from a normal
> >> >> connection. So using separate "pools" for them seems to make
> >> >> sense. That they end up allocating similar internal data seems
> >> >> to be an implementation detail to me.
> >>
> >> > Because replication connections are still "connections". If I
> >> > tell the system I want to allow 100 connections to the server,
> >> > it should allow 100 connections, not 110 or 95 or any other
> >> > number.
> >>
> >> I think that to reserve connections for replication, mechanism
> >> similar to superuser_reserved_connections be used rather than auto
> >> vacuum workers or background workers.
> >> This won't change the definition of MaxConnections. Another thing
> >> is that rather than introducing new parameter for replication
> >> reserved connections, it is better to use max_wal_senders as it
> >> can serve the purpose.
> >>
> >> Review for replication_reserved_connections-v2.patch, considering
> >> we are going to use mechanism similar to
> >> superuser_reserved_connections and won't allow definition of
> >> MaxConnections to change.
> >>
>
> >
> > Hi,
> >
> > I took the time and reworked the patch with the feedback till now.
> > Thank you very much Amit!
> >
> > So this patch uses max_wal_senders together with the idea of the
> > first patch I sent. The error messages are also adjusted to make it
> > obvious, how it is supposed to be and all checks work, as far as I
> > could tell.
>
> If I understand correctly, now the patch has implementation such that
> a. if the number of connections left are (ReservedBackends +
> max_wal_senders), then only superusers or replication connection's
> will be allowed
> b. if the number of connections left are ReservedBackend, then only
> superuser connections will be allowed.
That is correct.
> So it will ensure that max_wal_senders is used for reserving
> connection slots from being used by non-super user connections. I find
> new usage of max_wal_senders acceptable, if anyone else thinks
> otherwise, please let us know.
>
>
> 1.
> + <varname>superuser_reserved_connections</varname>
> + <varname>max_wal_senders</varname> only superuser and WAL
> connections
> + are allowed.
>
> Here minus seems to be missing before max_wal_senders and I think it
> will be better to use replication connections rather than WAL
> connections.
This is fixed.
> 2.
> - new replication connections will be accepted.
> + new WAL or other connections will be accepted.
>
> I think as per new implementation, we don't need to change this line.
I reverted that change.
> 3.
> + * reserved slots from max_connections for wal senders. If the
> number of free
> + * slots (max_connections - max_wal_senders) is depleted.
>
> Above calculation (max_connections - max_wal_senders) needs to
> include super user reserved connections.
My first thought was, that I would not add it here. When superuser
reserved connections are not set, then only max_wal_senders would
count.
But you are right, it has to be set, as 3 connections are reserved by
default for superusers.
> 4.
> + /*
> + * Although replication connections currently require superuser
> privileges, we
> + * don't allow them to consume the superuser reserved slots, which
> are
> + * intended for interactive use.
> */
> if ((!am_superuser || am_walsender) &&
> ReservedBackends > 0 &&
> !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 superuser
> connections")));
>
> Will there be any problem if we do the above check before the check
> for wal senders and reserved replication connections (+
> !HaveNFreeProcs(max_wal_senders + ReservedBackends))) and don't change
> the error message in this check. I think this will ensure that users
> who doesn't enable max_wal_senders will see the same error message as
> before and the purpose to reserve connections for replication can be
> served by your second check.
I have attached two patches, one that checks only max_wal_senders first
and the other checks reserved_backends first. Both return the original
message, when max_wal_sender is not set and I think it is only a matter
of taste, which comes first.
Me, I would prefer max_wal_senders to get from more connections to less.
> 5.
> + gettext_noop("Sets the maximum number wal sender connections and
> reserves them."),
>
> Sets the maximum number 'of' wal sender connections and reserves them.
> a. 'of' is missing in above line.
> b. I think 'reserves them' is not completely right, because super user
> connections will be allowed. How about if we add something similar
> to 'and reserves them from being used by non-superuser
> connections' in above line.
This is fixed.
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
Thank you again for your feedback.
regards,
Stefan Radomskidiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 77a9303..7fe4be2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2193,7 +2193,13 @@ include 'filename'
Specifies the maximum number of concurrent connections from
standby servers or streaming base backup clients (i.e., the
maximum number of simultaneously running WAL sender
- processes). The default is zero, meaning replication is
+ processes). The number of WAL senders is also reserved from
+ <xref linkend="guc-max-connections">. Whenever the number of connections
+ is at least <varname>max_connections</varname> minus
+ <varname>superuser_reserved_connections</varname> minus
+ <varname>max_wal_senders</varname> only superuser and replication
+ connections are allowed.
+ The default is zero, meaning replication is
disabled. WAL sender processes count towards the total number
of connections, so the parameter cannot be set higher than
<xref linkend="guc-max-connections">. This parameter can only
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 98086f7..dff67fc 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -198,10 +198,10 @@ char *ListenAddresses;
* ReservedBackends is the number of backends reserved for superuser use.
* This number is taken out of the pool size given by MaxBackends so
* number of backend slots available to non-superusers is
- * (MaxBackends - 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.
+ * (MaxBackends - ReservedBackends - max_wal_senders). 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.
*/
int ReservedBackends;
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index afd559d..919409b 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -94,7 +94,13 @@ bool am_cascading_walsender = false; /* Am I cascading WAL to
* another standby ? */
/* User-settable parameters for walsender */
-int max_wal_senders = 0; /* the maximum number of concurrent walsenders */
+/*
+ * This is the maximum number of concurrent wal senders and the amount of
+ * reserved slots from max_connections for wal senders. If the number of free
+ * slots (max_connections - superuser_reserved_connections - max_wal_senders)
+ * is depleted, only superusers and wal senders can connect.
+ */
+int max_wal_senders = 0;
int wal_sender_timeout = 60 * 1000; /* maximum time to send one
* WAL data message */
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 2c7f0f1..c03c5bf 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -700,10 +700,18 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
}
/*
- * The last few connections slots are reserved for superusers. Although
- * replication connections currently require superuser privileges, we
- * don't allow them to consume the reserved slots, which are intended for
- * interactive use.
+ * The last few connections slots are reserved for superusers and wal_senders.
+ */
+ if (!am_superuser && max_wal_senders > 0 &&
+ !HaveNFreeProcs(max_wal_senders + ReservedBackends))
+ ereport(FATAL,
+ (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
+ errmsg("remaining connection slots are reserved for replication and superuser connections")));
+
+ /*
+ * Although replication connections currently require superuser privileges, we
+ * don't allow them to consume the superuser reserved slots, which are
+ * intended for interactive use.
*/
if ((!am_superuser || am_walsender) &&
ReservedBackends > 0 &&
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index dfc6704..f6ab4ff 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2064,7 +2064,8 @@ static struct config_int ConfigureNamesInt[] =
{
/* see max_connections */
{"max_wal_senders", PGC_POSTMASTER, REPLICATION_SENDING,
- gettext_noop("Sets the maximum number of simultaneously running WAL sender processes."),
+ gettext_noop("Sets the maximum number of wal sender connections and reserves "
+ "them from being used non-superuser connections."),
NULL
},
&max_wal_senders,
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 77a9303..7fe4be2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2193,7 +2193,13 @@ include 'filename'
Specifies the maximum number of concurrent connections from
standby servers or streaming base backup clients (i.e., the
maximum number of simultaneously running WAL sender
- processes). The default is zero, meaning replication is
+ processes). The number of WAL senders is also reserved from
+ <xref linkend="guc-max-connections">. Whenever the number of connections
+ is at least <varname>max_connections</varname> minus
+ <varname>superuser_reserved_connections</varname> minus
+ <varname>max_wal_senders</varname> only superuser and replication
+ connections are allowed.
+ The default is zero, meaning replication is
disabled. WAL sender processes count towards the total number
of connections, so the parameter cannot be set higher than
<xref linkend="guc-max-connections">. This parameter can only
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 98086f7..dff67fc 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -198,10 +198,10 @@ char *ListenAddresses;
* ReservedBackends is the number of backends reserved for superuser use.
* This number is taken out of the pool size given by MaxBackends so
* number of backend slots available to non-superusers is
- * (MaxBackends - 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.
+ * (MaxBackends - ReservedBackends - max_wal_senders). 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.
*/
int ReservedBackends;
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index afd559d..919409b 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -94,7 +94,13 @@ bool am_cascading_walsender = false; /* Am I cascading WAL to
* another standby ? */
/* User-settable parameters for walsender */
-int max_wal_senders = 0; /* the maximum number of concurrent walsenders */
+/*
+ * This is the maximum number of concurrent wal senders and the amount of
+ * reserved slots from max_connections for wal senders. If the number of free
+ * slots (max_connections - superuser_reserved_connections - max_wal_senders)
+ * is depleted, only superusers and wal senders can connect.
+ */
+int max_wal_senders = 0;
int wal_sender_timeout = 60 * 1000; /* maximum time to send one
* WAL data message */
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 2c7f0f1..75121f0 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -700,10 +700,9 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
}
/*
- * The last few connections slots are reserved for superusers. Although
- * replication connections currently require superuser privileges, we
- * don't allow them to consume the reserved slots, which are intended for
- * interactive use.
+ * Although replication connections currently require superuser privileges, we
+ * don't allow them to consume the superuser reserved slots, which are
+ * intended for interactive use.
*/
if ((!am_superuser || am_walsender) &&
ReservedBackends > 0 &&
@@ -713,6 +712,15 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
errmsg("remaining connection slots are reserved for non-replication superuser connections")));
/*
+ * The last few connections slots are reserved for superusers and wal_senders.
+ */
+ if (!am_superuser && max_wal_senders > 0 &&
+ !HaveNFreeProcs(max_wal_senders + ReservedBackends))
+ ereport(FATAL,
+ (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
+ errmsg("remaining connection slots are reserved for replication and superuser connections")));
+
+ /*
* If walsender, we don't want to connect to any particular database. Just
* finish the backend startup by processing any options from the startup
* packet, and we're done.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index dfc6704..f6ab4ff 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2064,7 +2064,8 @@ static struct config_int ConfigureNamesInt[] =
{
/* see max_connections */
{"max_wal_senders", PGC_POSTMASTER, REPLICATION_SENDING,
- gettext_noop("Sets the maximum number of simultaneously running WAL sender processes."),
+ gettext_noop("Sets the maximum number of wal sender connections and reserves "
+ "them from being used non-superuser connections."),
NULL
},
&max_wal_senders,
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
