Re: Patch for reserved connections for replication users

Sorry missed to keep -hackers in previous mail.

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Date: Mon, Oct 7, 2013 at 11:37 AM
Subject: Re: Patch for reserved connections for replication users
To: Robert Haas <robertmhaas(at)gmail(dot)com>, gibheer(at)zero-knowledge(dot)org
Cc: Andres Freund <andres(at)2ndquadrant(dot)com>, marko(at)joh(dot)to

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.

1. /* the extra unit accounts for the autovacuum launcher */
MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
- + max_worker_processes;
+ + max_worker_processes + max_wal_senders;

Above changes are not required.

2.
+ if ((!am_superuser && !am_walsender) &&
ReservedBackends > 0 &&
!HaveNFreeProcs(ReservedBackends))

Change the check as you have in patch-1 for ReserveReplicationConnections

if (!am_superuser &&
(max_wal_senders > 0 || ReservedBackends > 0) &&
!HaveNFreeProcs(max_wal_senders + ReservedBackends))
ereport(FATAL,
(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
errmsg("remaining connection slots are reserved for replication and
superuser connections")));

3. In guc.c, change description of max_wal_senders similar to
superuser_reserved_connections at below place:
{"max_wal_senders", PGC_POSTMASTER, REPLICATION_SENDING,
gettext_noop("Sets the maximum number of simultaneously running WAL
sender processes."),

4. With this approach, there is no need to change InitProcGlobal(), as
it is used to keep track bgworkerFreeProcs and autovacFreeProcs, for
others it use freeProcs.

5. Below description in config.sgml needs to be changed for
superuser_reserved_connections to include the effect of max_wal_enders
in reserved connections.
Whenever the 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.

6. Also similar description should be added to max_wal_senders
configuration parameter.

7. Below comment needs to be updated, as it assumes only superuser
reserved connections as part of MaxConnections limit.
/*
* 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.
*/
int ReservedBackends;

8. Also we can add comment on top of definition for max_wal_senders
similar to ReservedBackends.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, 7 Oct 2013 11:39:55 +0530
Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>
> 1. /* the extra unit accounts for the autovacuum launcher */
> MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
> - + max_worker_processes;
> + + max_worker_processes + max_wal_senders;
>
> Above changes are not required.
>
> 2.
> + if ((!am_superuser && !am_walsender) &&
> ReservedBackends > 0 &&
> !HaveNFreeProcs(ReservedBackends))
>
> Change the check as you have in patch-1 for
> ReserveReplicationConnections
>
> if (!am_superuser &&
> (max_wal_senders > 0 || ReservedBackends > 0) &&
> !HaveNFreeProcs(max_wal_senders + ReservedBackends))
> ereport(FATAL,
> (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
> errmsg("remaining connection slots are reserved for replication and
> superuser connections")));
>
> 3. In guc.c, change description of max_wal_senders similar to
> superuser_reserved_connections at below place:
> {"max_wal_senders", PGC_POSTMASTER, REPLICATION_SENDING,
> gettext_noop("Sets the maximum number of simultaneously running WAL
> sender processes."),
>
> 4. With this approach, there is no need to change InitProcGlobal(), as
> it is used to keep track bgworkerFreeProcs and autovacFreeProcs, for
> others it use freeProcs.
>
> 5. Below description in config.sgml needs to be changed for
> superuser_reserved_connections to include the effect of max_wal_enders
> in reserved connections.
> Whenever the 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.
>
> 6. Also similar description should be added to max_wal_senders
> configuration parameter.
>
> 7. Below comment needs to be updated, as it assumes only superuser
> reserved connections as part of MaxConnections limit.
> /*
> * 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.
> */
> int ReservedBackends;
>
> 8. Also we can add comment on top of definition for max_wal_senders
> similar to ReservedBackends.
>
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>

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.

regards,

Stefan Radomski

On Thu, Oct 10, 2013 at 3:17 AM, Gibheer <gibheer(at)zero-knowledge(dot)org> wrote:
> On Mon, 7 Oct 2013 11:39:55 +0530
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>>
>> 1. /* the extra unit accounts for the autovacuum launcher */
>> MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
>> - + max_worker_processes;
>> + + max_worker_processes + max_wal_senders;
>>
>> Above changes are not required.
>>
>> 2.
>> + if ((!am_superuser && !am_walsender) &&
>> ReservedBackends > 0 &&
>> !HaveNFreeProcs(ReservedBackends))
>>
>> Change the check as you have in patch-1 for
>> ReserveReplicationConnections
>>
>> if (!am_superuser &&
>> (max_wal_senders > 0 || ReservedBackends > 0) &&
>> !HaveNFreeProcs(max_wal_senders + ReservedBackends))
>> ereport(FATAL,
>> (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
>> errmsg("remaining connection slots are reserved for replication and
>> superuser connections")));
>>
>> 3. In guc.c, change description of max_wal_senders similar to
>> superuser_reserved_connections at below place:
>> {"max_wal_senders", PGC_POSTMASTER, REPLICATION_SENDING,
>> gettext_noop("Sets the maximum number of simultaneously running WAL
>> sender processes."),
>>
>> 4. With this approach, there is no need to change InitProcGlobal(), as
>> it is used to keep track bgworkerFreeProcs and autovacFreeProcs, for
>> others it use freeProcs.
>>
>> 5. Below description in config.sgml needs to be changed for
>> superuser_reserved_connections to include the effect of max_wal_enders
>> in reserved connections.
>> Whenever the 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.
>>
>> 6. Also similar description should be added to max_wal_senders
>> configuration parameter.
>>
>> 7. Below comment needs to be updated, as it assumes only superuser
>> reserved connections as part of MaxConnections limit.
>> /*
>> * 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.
>> */
>> int ReservedBackends;
>>
>> 8. Also we can add comment on top of definition for max_wal_senders
>> similar to ReservedBackends.
>>
>>
>> With Regards,
>> Amit Kapila.
>> EnterpriseDB: http://www.enterprisedb.com
>>
>
> Hi,
>
> I took the time and reworked the patch with the feedback till now.
> Thank you very much Amit!

Is there any specific reason why you moved this patch to next CommitFest?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

I'd received an email from Gibheer suggesting it be move due to lack of time to work on it. I can certainly move it back if that's no longer the case.

On Oct 9, 2013, at 23:25, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Thu, Oct 10, 2013 at 3:17 AM, Gibheer <gibheer(at)zero-knowledge(dot)org> wrote:
> On Mon, 7 Oct 2013 11:39:55 +0530
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>>
>> 1. /* the extra unit accounts for the autovacuum launcher */
>> MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
>> - + max_worker_processes;
>> + + max_worker_processes + max_wal_senders;
>>
>> Above changes are not required.
>>
>> 2.
>> + if ((!am_superuser && !am_walsender) &&
>> ReservedBackends > 0 &&
>> !HaveNFreeProcs(ReservedBackends))
>>
>> Change the check as you have in patch-1 for
>> ReserveReplicationConnections
>>
>> if (!am_superuser &&
>> (max_wal_senders > 0 || ReservedBackends > 0) &&
>> !HaveNFreeProcs(max_wal_senders + ReservedBackends))
>> ereport(FATAL,
>> (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
>> errmsg("remaining connection slots are reserved for replication and
>> superuser connections")));
>>
>> 3. In guc.c, change description of max_wal_senders similar to
>> superuser_reserved_connections at below place:
>> {"max_wal_senders", PGC_POSTMASTER, REPLICATION_SENDING,
>> gettext_noop("Sets the maximum number of simultaneously running WAL
>> sender processes."),
>>
>> 4. With this approach, there is no need to change InitProcGlobal(), as
>> it is used to keep track bgworkerFreeProcs and autovacFreeProcs, for
>> others it use freeProcs.
>>
>> 5. Below description in config.sgml needs to be changed for
>> superuser_reserved_connections to include the effect of max_wal_enders
>> in reserved connections.
>> Whenever the 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.
>>
>> 6. Also similar description should be added to max_wal_senders
>> configuration parameter.
>>
>> 7. Below comment needs to be updated, as it assumes only superuser
>> reserved connections as part of MaxConnections limit.
>> /*
>> * 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.
>> */
>> int ReservedBackends;
>>
>> 8. Also we can add comment on top of definition for max_wal_senders
>> similar to ReservedBackends.
>>
>>
>> With Regards,
>> Amit Kapila.
>> EnterpriseDB: http://www.enterprisedb.com
>
> Hi,
>
> I took the time and reworked the patch with the feedback till now.
> Thank you very much Amit!

Is there any specific reason why you moved this patch to next CommitFest?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, 10 Oct 2013 09:55:24 +0530
Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Thu, Oct 10, 2013 at 3:17 AM, Gibheer <gibheer(at)zero-knowledge(dot)org>
> wrote:
> > On Mon, 7 Oct 2013 11:39:55 +0530
> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
> >>
> >> 1. /* the extra unit accounts for the autovacuum launcher */
> >> MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
> >> - + max_worker_processes;
> >> + + max_worker_processes + max_wal_senders;
> >>
> >> Above changes are not required.
> >>
> >> 2.
> >> + if ((!am_superuser && !am_walsender) &&
> >> ReservedBackends > 0 &&
> >> !HaveNFreeProcs(ReservedBackends))
> >>
> >> Change the check as you have in patch-1 for
> >> ReserveReplicationConnections
> >>
> >> if (!am_superuser &&
> >> (max_wal_senders > 0 || ReservedBackends > 0) &&
> >> !HaveNFreeProcs(max_wal_senders + ReservedBackends))
> >> ereport(FATAL,
> >> (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
> >> errmsg("remaining connection slots are reserved for replication and
> >> superuser connections")));
> >>
> >> 3. In guc.c, change description of max_wal_senders similar to
> >> superuser_reserved_connections at below place:
> >> {"max_wal_senders", PGC_POSTMASTER, REPLICATION_SENDING,
> >> gettext_noop("Sets the maximum number of simultaneously running WAL
> >> sender processes."),
> >>
> >> 4. With this approach, there is no need to change
> >> InitProcGlobal(), as it is used to keep track bgworkerFreeProcs
> >> and autovacFreeProcs, for others it use freeProcs.
> >>
> >> 5. Below description in config.sgml needs to be changed for
> >> superuser_reserved_connections to include the effect of
> >> max_wal_enders in reserved connections.
> >> Whenever the 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.
> >>
> >> 6. Also similar description should be added to max_wal_senders
> >> configuration parameter.
> >>
> >> 7. Below comment needs to be updated, as it assumes only superuser
> >> reserved connections as part of MaxConnections limit.
> >> /*
> >> * 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.
> >> */
> >> int ReservedBackends;
> >>
> >> 8. Also we can add comment on top of definition for max_wal_senders
> >> similar to ReservedBackends.
> >>
> >>
> >> With Regards,
> >> Amit Kapila.
> >> EnterpriseDB: http://www.enterprisedb.com
> >>
> >
> > Hi,
> >
> > I took the time and reworked the patch with the feedback till now.
> > Thank you very much Amit!
>
> Is there any specific reason why you moved this patch to next
> CommitFest?
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>

Mike asked me about the status of the patch a couple days back and I
didn't think I would be able to work on the patch so soon again. That's
why I told him to just move the patch into the next commitfest.

On Thu, Oct 10, 2013 at 10:06 PM, Gibheer <gibheer(at)zero-knowledge(dot)org> wrote:
> On Thu, 10 Oct 2013 09:55:24 +0530
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
>> On Thu, Oct 10, 2013 at 3:17 AM, Gibheer <gibheer(at)zero-knowledge(dot)org>
>> wrote:
>> > On Mon, 7 Oct 2013 11:39:55 +0530
>> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>> >>
>> >> 1. /* the extra unit accounts for the autovacuum launcher */
>> >> MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
>> >> - + max_worker_processes;
>> >> + + max_worker_processes + max_wal_senders;
>> >>
>> >> Above changes are not required.
>> >>
>> >> 2.
>> >> + if ((!am_superuser && !am_walsender) &&
>> >> ReservedBackends > 0 &&
>> >> !HaveNFreeProcs(ReservedBackends))
>> >>
>> >> Change the check as you have in patch-1 for
>> >> ReserveReplicationConnections
>> >>
>> >> if (!am_superuser &&
>> >> (max_wal_senders > 0 || ReservedBackends > 0) &&
>> >> !HaveNFreeProcs(max_wal_senders + ReservedBackends))
>> >> ereport(FATAL,
>> >> (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
>> >> errmsg("remaining connection slots are reserved for replication and
>> >> superuser connections")));
>> >>
>> >> 3. In guc.c, change description of max_wal_senders similar to
>> >> superuser_reserved_connections at below place:
>> >> {"max_wal_senders", PGC_POSTMASTER, REPLICATION_SENDING,
>> >> gettext_noop("Sets the maximum number of simultaneously running WAL
>> >> sender processes."),
>> >>
>> >> 4. With this approach, there is no need to change
>> >> InitProcGlobal(), as it is used to keep track bgworkerFreeProcs
>> >> and autovacFreeProcs, for others it use freeProcs.
>> >>
>> >> 5. Below description in config.sgml needs to be changed for
>> >> superuser_reserved_connections to include the effect of
>> >> max_wal_enders in reserved connections.
>> >> Whenever the 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.
>> >>
>> >> 6. Also similar description should be added to max_wal_senders
>> >> configuration parameter.
>> >>
>> >> 7. Below comment needs to be updated, as it assumes only superuser
>> >> reserved connections as part of MaxConnections limit.
>> >> /*
>> >> * 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.
>> >> */
>> >> int ReservedBackends;
>> >>
>> >> 8. Also we can add comment on top of definition for max_wal_senders
>> >> similar to ReservedBackends.
>> >>
>> >>
>> >> With Regards,
>> >> Amit Kapila.
>> >> EnterpriseDB: http://www.enterprisedb.com
>> >>
>> >
>> > Hi,
>> >
>> > I took the time and reworked the patch with the feedback till now.
>> > Thank you very much Amit!
>>
>> Is there any specific reason why you moved this patch to next
>> CommitFest?
>>
>> With Regards,
>> Amit Kapila.
>> EnterpriseDB: http://www.enterprisedb.com
>>
>
> Mike asked me about the status of the patch a couple days back and I
> didn't think I would be able to work on the patch so soon again. That's
> why I told him to just move the patch into the next commitfest.

But you have already posted patch after fixing comments and I am
planning to review again on coming weekend; if every thing is okay,
then there is a chance that you can get committer level feedback for
your patch in this CF. In the end it's upto you to decide.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, 11 Oct 2013 10:00:51 +0530
Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Thu, Oct 10, 2013 at 10:06 PM, Gibheer
> <gibheer(at)zero-knowledge(dot)org> wrote:
> > On Thu, 10 Oct 2013 09:55:24 +0530
> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> >> On Thu, Oct 10, 2013 at 3:17 AM, Gibheer
> >> <gibheer(at)zero-knowledge(dot)org> wrote:
> >> > On Mon, 7 Oct 2013 11:39:55 +0530
> >> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
> >> >>
> >> >> 1. /* the extra unit accounts for the autovacuum launcher */
> >> >> MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
> >> >> - + max_worker_processes;
> >> >> + + max_worker_processes + max_wal_senders;
> >> >>
> >> >> Above changes are not required.
> >> >>
> >> >> 2.
> >> >> + if ((!am_superuser && !am_walsender) &&
> >> >> ReservedBackends > 0 &&
> >> >> !HaveNFreeProcs(ReservedBackends))
> >> >>
> >> >> Change the check as you have in patch-1 for
> >> >> ReserveReplicationConnections
> >> >>
> >> >> if (!am_superuser &&
> >> >> (max_wal_senders > 0 || ReservedBackends > 0) &&
> >> >> !HaveNFreeProcs(max_wal_senders + ReservedBackends))
> >> >> ereport(FATAL,
> >> >> (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
> >> >> errmsg("remaining connection slots are reserved for replication
> >> >> and superuser connections")));
> >> >>
> >> >> 3. In guc.c, change description of max_wal_senders similar to
> >> >> superuser_reserved_connections at below place:
> >> >> {"max_wal_senders", PGC_POSTMASTER, REPLICATION_SENDING,
> >> >> gettext_noop("Sets the maximum number of simultaneously running
> >> >> WAL sender processes."),
> >> >>
> >> >> 4. With this approach, there is no need to change
> >> >> InitProcGlobal(), as it is used to keep track bgworkerFreeProcs
> >> >> and autovacFreeProcs, for others it use freeProcs.
> >> >>
> >> >> 5. Below description in config.sgml needs to be changed for
> >> >> superuser_reserved_connections to include the effect of
> >> >> max_wal_enders in reserved connections.
> >> >> Whenever the 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.
> >> >>
> >> >> 6. Also similar description should be added to max_wal_senders
> >> >> configuration parameter.
> >> >>
> >> >> 7. Below comment needs to be updated, as it assumes only
> >> >> superuser reserved connections as part of MaxConnections limit.
> >> >> /*
> >> >> * 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.
> >> >> */
> >> >> int ReservedBackends;
> >> >>
> >> >> 8. Also we can add comment on top of definition for
> >> >> max_wal_senders similar to ReservedBackends.
> >> >>
> >> >>
> >> >> With Regards,
> >> >> Amit Kapila.
> >> >> EnterpriseDB: http://www.enterprisedb.com
> >> >>
> >> >
> >> > Hi,
> >> >
> >> > I took the time and reworked the patch with the feedback till
> >> > now. Thank you very much Amit!
> >>
> >> Is there any specific reason why you moved this patch to next
> >> CommitFest?
> >>
> >> With Regards,
> >> Amit Kapila.
> >> EnterpriseDB: http://www.enterprisedb.com
> >>
> >
> > Mike asked me about the status of the patch a couple days back and I
> > didn't think I would be able to work on the patch so soon again.
> > That's why I told him to just move the patch into the next
> > commitfest.
>
> But you have already posted patch after fixing comments and I am
> planning to review again on coming weekend; if every thing is okay,
> then there is a chance that you can get committer level feedback for
> your patch in this CF. In the end it's upto you to decide.
>
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>

You are right. If we can get it working in this commit fest, then it is
out of the way for the next.

@Mike: Can you move the patch back to this commit fest? Amit sounds
like we can get this done, so I will support that and work as hard to
make it happen.

And thank you for all your feedback.

regards,

Stefan Radomski

On Thu, Oct 10, 2013 at 3:17 AM, Gibheer <gibheer(at)zero-knowledge(dot)org> wrote:
> On Mon, 7 Oct 2013 11:39:55 +0530
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

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.

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.

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.

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.

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.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sun, 13 Oct 2013 11:38:17 +0530
Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Thu, Oct 10, 2013 at 3:17 AM, Gibheer <gibheer(at)zero-knowledge(dot)org>
> wrote:
> > On Mon, 7 Oct 2013 11:39:55 +0530
> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 Radomski

On Sun, Oct 13, 2013 at 2:08 PM, Gibheer <gibheer(at)zero-knowledge(dot)org> wrote:
> On Sun, 13 Oct 2013 11:38:17 +0530
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
>> On Thu, Oct 10, 2013 at 3:17 AM, Gibheer <gibheer(at)zero-knowledge(dot)org>
>> wrote:
>> > On Mon, 7 Oct 2013 11:39:55 +0530
>> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

+ * slots (max_connections - superuser_reserved_connections - max_wal_senders)
here it should be ReservedBackends rather than superuser_reserved_connections.

>> 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.

I think there is no major problem even if we keep max_wal_senders
check first, but there could be error message inconsistency. Please
consider below scenario:
max_connections=5
superuser_reserved_connections=3
max_wal_senders = 2

1. Start primary database server M1
2. Start first standby S1
3. Start second standby S2
4. Now try to connect by non-super user to M1 -- here it will return
error msg as "psql: FATAL: remaining connection slots are reserved
for replication
and superuser connections"
By above error message, it seems that replication connection is
allowed, but actually it will give error for new replication
connection, see next step
5. Start third standby S3 -- here an appropriate error message "FATAL:
could not connect to the primary server: FATAL: remaining connection
slots are reserved for non-replication superuser connections"

I feel there is minor inconsistency in step-4 if we use
max_wal_senders check before ReservedBackends check.

>> 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.
+ gettext_noop("Sets the maximum number of wal sender connections and reserves "
+ "them from being used non-superuser connections."),

"them from being used 'by' non-superuser connections."
'by' is missing in above line.

1.
if (ReservedBackends >= MaxConnections)
{
write_stderr("%s: superuser_reserved_connections must be less than
max_connections\n", progname);
ExitPostmaster(1);
}

I think we should have one check similar to above for (max_wal_senders
+ ReservedBackends >= MaxConnections) to avoid server starting
with values, where not even 1 non-superuser connection will be
allowed. I think this is a reasoning similar to why the check for
ReservedBackends exists.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 10/13/2013 01:38 AM, Gibheer wrote:
> 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.

I think otherwise.

Changing max_wal_senders requires a restart. As such, we currently
advise users to set the setting generously: "as many replication
connections as you think you'll ever need, plus two". If
max_wal_senders is a reservation which could cause the user to run out
of other connections sooner than expected, then the user is faced with a
new "hard to set" parameter: they don't want to set it too high *or* too
low.

This would result in a lot of user frustration as they try to get thier
connection configuration right and have to restart the server multiple
times. I find few new features worth making it *harder* to configure
PostgreSQL, and reserved replication connections certainly don't qualify.

If it's worth having reserved replication connections (and I can see
some reasons to want it), then we need a new GUC for this:
"reserved_walsender_connections"

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 2013-10-14 10:26:25 -0700, Josh Berkus wrote:
> On 10/13/2013 01:38 AM, Gibheer wrote:
> > 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.
>
> I think otherwise.
>
> Changing max_wal_senders requires a restart. As such, we currently
> advise users to set the setting generously: "as many replication
> connections as you think you'll ever need, plus two". If
> max_wal_senders is a reservation which could cause the user to run out
> of other connections sooner than expected, then the user is faced with a
> new "hard to set" parameter: they don't want to set it too high *or* too
> low.
>
> This would result in a lot of user frustration as they try to get thier
> connection configuration right and have to restart the server multiple
> times. I find few new features worth making it *harder* to configure
> PostgreSQL, and reserved replication connections certainly don't qualify.
>
> If it's worth having reserved replication connections (and I can see
> some reasons to want it), then we need a new GUC for this:
> "reserved_walsender_connections"

Imo the complications around this prove my (way earlier) point that it'd
be much better to treat replication connections as something entirely
different to normal SQL connections. There's really not much overlap
here and while there's some philosophical point to be made about it all
being connections, from a practical POV treating them separately seems
better.
If we were designing on a green field I'd just rename max_connections to
something explicitly talking about client database connections, but
renaming it seems like it'd cause unnecessary pain.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 10/14/2013 10:51 AM, Andres Freund wrote:
> Imo the complications around this prove my (way earlier) point that it'd
> be much better to treat replication connections as something entirely
> different to normal SQL connections. There's really not much overlap
> here and while there's some philosophical point to be made about it all
> being connections, from a practical POV treating them separately seems
> better.

Given that replication connections don't even appear in pg_stat_activity
now, I'd agree with you.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Mon, Oct 14, 2013 at 10:56 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 10/13/2013 01:38 AM, Gibheer wrote:
>> 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.
>
> I think otherwise.
>
> Changing max_wal_senders requires a restart. As such, we currently
> advise users to set the setting generously: "as many replication
> connections as you think you'll ever need, plus two". If
> max_wal_senders is a reservation which could cause the user to run out
> of other connections sooner than expected, then the user is faced with a
> new "hard to set" parameter: they don't want to set it too high *or* too
> low.

It is documented in the patch that configuration max_wal_senders is
reserved from max_connections.
So it will not cause the user to run out of other connections sooner
than expected, if both the variables are configured properly.

> This would result in a lot of user frustration as they try to get thier
> connection configuration right and have to restart the server multiple
> times. I find few new features worth making it *harder* to configure
> PostgreSQL, and reserved replication connections certainly don't qualify.
>
> If it's worth having reserved replication connections (and I can see
> some reasons to want it), then we need a new GUC for this:
> "reserved_walsender_connections"

Having new variable also might make users life difficult, as with new
variable he needs to take care of setting 3 parameters
(max_wal_senders, reserved_walsender_connections, max_connections)
appropriately for this purpose and then choosing value for
reserved_walsender_connections will be another thing for which he has
to consult.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Oct 14, 2013 at 11:21 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-10-14 10:26:25 -0700, Josh Berkus wrote:
>> On 10/13/2013 01:38 AM, Gibheer wrote:
>> > 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.
>>
>> I think otherwise.
>>
>> Changing max_wal_senders requires a restart. As such, we currently
>> advise users to set the setting generously: "as many replication
>> connections as you think you'll ever need, plus two". If
>> max_wal_senders is a reservation which could cause the user to run out
>> of other connections sooner than expected, then the user is faced with a
>> new "hard to set" parameter: they don't want to set it too high *or* too
>> low.
>>
>> This would result in a lot of user frustration as they try to get thier
>> connection configuration right and have to restart the server multiple
>> times. I find few new features worth making it *harder* to configure
>> PostgreSQL, and reserved replication connections certainly don't qualify.
>>
>> If it's worth having reserved replication connections (and I can see
>> some reasons to want it), then we need a new GUC for this:
>> "reserved_walsender_connections"
>
> Imo the complications around this prove my (way earlier) point that it'd
> be much better to treat replication connections as something entirely
> different to normal SQL connections. There's really not much overlap
> here and while there's some philosophical point to be made about it all
> being connections, from a practical POV treating them separately seems
> better.
> If we were designing on a green field I'd just rename max_connections to
> something explicitly talking about client database connections, but
> renaming it seems like it'd cause unnecessary pain.

If we think this way, then may be we should have max_user_connections
instead of max_connections and then max_wal_connections. But still
there are other's like pg_basebackup who needs connections and
tomorrow there can be new such entities which need connection.
Also we might need to have different infrastructure in code to make
these options available to users.
I think having different parameters to configure maximum connections
for different entities can complicate both code as well as user's job.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Oct 15, 2013 at 12:13 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> If we think this way, then may be we should have max_user_connections
> instead of max_connections and then max_wal_connections. But still
> there are other's like pg_basebackup who needs connections and
> tomorrow there can be new such entities which need connection.
> Also we might need to have different infrastructure in code to make
> these options available to users.
> I think having different parameters to configure maximum connections
> for different entities can complicate both code as well as user's job.

Renaming max_connections is far too big a compatibility break to
consider without far more benefit than what this patch is aiming at.
I'm not prepared to endure the number of beatings I'd have to take if
we did that.

But I also agree that making max_wal_senders act as both a minimum and
a maximum is no good. +1 to everything Josh Berkus said.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-10-15 10:29:58 -0400, Robert Haas wrote:
> On Tue, Oct 15, 2013 at 12:13 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > If we think this way, then may be we should have max_user_connections
> > instead of max_connections and then max_wal_connections. But still
> > there are other's like pg_basebackup who needs connections and
> > tomorrow there can be new such entities which need connection.
> > Also we might need to have different infrastructure in code to make
> > these options available to users.
> > I think having different parameters to configure maximum connections
> > for different entities can complicate both code as well as user's job.
>
> Renaming max_connections is far too big a compatibility break to
> consider without far more benefit than what this patch is aiming at.
> I'm not prepared to endure the number of beatings I'd have to take if
> we did that.

+many

> But I also agree that making max_wal_senders act as both a minimum and
> a maximum is no good. +1 to everything Josh Berkus said.

Josh said we should treat replication connections in a separate "pool"
from normal database connections, right? So you withdraw your earlier
objection to that?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Oct 15, 2013 at 10:34 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> But I also agree that making max_wal_senders act as both a minimum and
>> a maximum is no good. +1 to everything Josh Berkus said.
>
> Josh said we should treat replication connections in a separate "pool"
> from normal database connections, right? So you withdraw your earlier
> objection to that?

I don't think that's what he said. Here's what I was referring to:

$ Changing max_wal_senders requires a restart. As such, we currently
$ advise users to set the setting generously: "as many replication
$ connections as you think you'll ever need, plus two". If
$ max_wal_senders is a reservation which could cause the user to run out
$ of other connections sooner than expected, then the user is faced with a
$ new "hard to set" parameter: they don't want to set it too high *or* too
$ low.
$
$ This would result in a lot of user frustration as they try to get thier
$ connection configuration right and have to restart the server multiple
$ times. I find few new features worth making it *harder* to configure
$ PostgreSQL, and reserved replication connections certainly don't qualify.
$
$ If it's worth having reserved replication connections (and I can see
$ some reasons to want it), then we need a new GUC for this:
$ "reserved_walsender_connections"

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-10-15 10:36:41 -0400, Robert Haas wrote:
> On Tue, Oct 15, 2013 at 10:34 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> But I also agree that making max_wal_senders act as both a minimum and
> >> a maximum is no good. +1 to everything Josh Berkus said.
> >
> > Josh said we should treat replication connections in a separate "pool"
> > from normal database connections, right? So you withdraw your earlier
> > objection to that?
>
> I don't think that's what he said. Here's what I was referring to:
>
> $ Changing max_wal_senders requires a restart. As such, we currently
> $ advise users to set the setting generously: "as many replication
> $ connections as you think you'll ever need, plus two". If
> $ max_wal_senders is a reservation which could cause the user to run out
> $ of other connections sooner than expected, then the user is faced with a
> $ new "hard to set" parameter: they don't want to set it too high *or* too
> $ low.
> $
> $ This would result in a lot of user frustration as they try to get thier
> $ connection configuration right and have to restart the server multiple
> $ times. I find few new features worth making it *harder* to configure
> $ PostgreSQL, and reserved replication connections certainly don't qualify.
> $
> $ If it's worth having reserved replication connections (and I can see
> $ some reasons to want it), then we need a new GUC for this:
> $ "reserved_walsender_connections"

I am referring to
http://archives.postgresql.org/message-id/525C31D3.3010006%40agliodbs.com
:
> On 10/14/2013 10:51 AM, Andres Freund wrote:
> > Imo the complications around this prove my (way earlier) point that it'd
> > be much better to treat replication connections as something entirely
> > different to normal SQL connections. There's really not much overlap
> > here and while there's some philosophical point to be made about it all
> > being connections, from a practical POV treating them separately seems
> > better.
>
> Given that replication connections don't even appear in pg_stat_activity
> now, I'd agree with you.

I still fail to see what the point is in treating those classes of
connections together. It only serves to confuse users and makes
considerations way much more complicated. There's no need for reserved
replication connections or anything like it if would separate the pools.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 10/15/2013 07:36 AM, Robert Haas wrote:
> On Tue, Oct 15, 2013 at 10:34 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> Josh said we should treat replication connections in a separate "pool"
>> from normal database connections, right? So you withdraw your earlier
>> objection to that?
>
> I don't think that's what he said. Here's what I was referring to:

To clarify: I do, indeed, support the idea of treating replication
connections as a pool outside of max_connections. Here's why:

FATAL: connection limit exceeded for non-superusers

SHOW max_connections;

100

SELECT COUNT(*) FROM pg_stat_activity;

94

SHOW superuser_reserved_connections;

3

????

... search around quite a bit, eventually figure out that you have
three replication connections open. We've already set up an illogical
and hard-to-troubleshoot situation where replication connections do not
appear in pg_stat_activity, yet they are counted against max_connections.

You could argue that the same is true of superuser_reserved_connections,
but there's a couple reasons why it isn't:

1) if superusers are actually connected, that shows up in
pg_stat_activity (and given how many of our users run their apps as
superuser, they get to max_connections out anyway).

2) the error message spells out that there may be superuser connections
available.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Mon, 14 Oct 2013 11:52:57 +0530
Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Sun, Oct 13, 2013 at 2:08 PM, Gibheer <gibheer(at)zero-knowledge(dot)org>
> wrote:
> > On Sun, 13 Oct 2013 11:38:17 +0530
> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> >> On Thu, Oct 10, 2013 at 3:17 AM, Gibheer
> >> <gibheer(at)zero-knowledge(dot)org> wrote:
> >> > On Mon, 7 Oct 2013 11:39:55 +0530
> >> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>
> + * slots (max_connections - superuser_reserved_connections -
> max_wal_senders) here it should be ReservedBackends rather than
> superuser_reserved_connections.

fixed

> >> 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.
>
> I think there is no major problem even if we keep max_wal_senders
> check first, but there could be error message inconsistency. Please
> consider below scenario:
> max_connections=5
> superuser_reserved_connections=3
> max_wal_senders = 2
>
> 1. Start primary database server M1
> 2. Start first standby S1
> 3. Start second standby S2
> 4. Now try to connect by non-super user to M1 -- here it will return
> error msg as "psql: FATAL: remaining connection slots are reserved
> for replication
> and superuser connections"
> By above error message, it seems that replication connection is
> allowed, but actually it will give error for new replication
> connection, see next step
> 5. Start third standby S3 -- here an appropriate error message "FATAL:
> could not connect to the primary server: FATAL: remaining connection
> slots are reserved for non-replication superuser connections"
>
> I feel there is minor inconsistency in step-4 if we use
> max_wal_senders check before ReservedBackends check.

Yes, that is rather annoying. I fixed that with the other hint, that
there should be a check for ReservedBackends + max_wal_senders <
max_connections to have at least one free connection.

> >> 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.
> + gettext_noop("Sets the maximum number of wal sender connections and
> reserves "
> + "them from being used non-superuser connections."),
>
> "them from being used 'by' non-superuser connections."
> 'by' is missing in above line.
>
> 1.
> if (ReservedBackends >= MaxConnections)
> {
> write_stderr("%s: superuser_reserved_connections must be less than
> max_connections\n", progname);
> ExitPostmaster(1);
> }
>
> I think we should have one check similar to above for (max_wal_senders
> + ReservedBackends >= MaxConnections) to avoid server starting
> with values, where not even 1 non-superuser connection will be
> allowed. I think this is a reasoning similar to why the check for
> ReservedBackends exists.

I have added this, see comment above.

> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>

I would be glad, if you could also test the patch again, as I'm nearly
code blind after testing it for 4 hours.
I had the problem, that I could not attach as many replication
connections as I wanted, as they were denied as normal connections. I
think I got it fixed, but I'm not 100% sure at the moment.
After some sleep, I will read the code again and test it again, to make
sure, it really does what it is supposed to do.

Thank you,

Stefan

On Wed, Oct 16, 2013 at 4:30 AM, Gibheer <gibheer(at)zero-knowledge(dot)org> wrote:
> On Mon, 14 Oct 2013 11:52:57 +0530
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
>> On Sun, Oct 13, 2013 at 2:08 PM, Gibheer <gibheer(at)zero-knowledge(dot)org>
>> wrote:
>> > On Sun, 13 Oct 2013 11:38:17 +0530
>> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> >
>> >> On Thu, Oct 10, 2013 at 3:17 AM, Gibheer
>> >> <gibheer(at)zero-knowledge(dot)org> wrote:
>> >> > On Mon, 7 Oct 2013 11:39:55 +0530
>> >> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>>
>> + * slots (max_connections - superuser_reserved_connections -
>> max_wal_senders) here it should be ReservedBackends rather than
>> superuser_reserved_connections.
>
> fixed
>
>> >> 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.
>>
>> I think there is no major problem even if we keep max_wal_senders
>> check first, but there could be error message inconsistency. Please
>> consider below scenario:
>> max_connections=5
>> superuser_reserved_connections=3
>> max_wal_senders = 2
>>
>> 1. Start primary database server M1
>> 2. Start first standby S1
>> 3. Start second standby S2
>> 4. Now try to connect by non-super user to M1 -- here it will return
>> error msg as "psql: FATAL: remaining connection slots are reserved
>> for replication
>> and superuser connections"
>> By above error message, it seems that replication connection is
>> allowed, but actually it will give error for new replication
>> connection, see next step
>> 5. Start third standby S3 -- here an appropriate error message "FATAL:
>> could not connect to the primary server: FATAL: remaining connection
>> slots are reserved for non-replication superuser connections"
>>
>> I feel there is minor inconsistency in step-4 if we use
>> max_wal_senders check before ReservedBackends check.
>
> Yes, that is rather annoying. I fixed that with the other hint, that
> there should be a check for ReservedBackends + max_wal_senders <
> max_connections to have at least one free connection.
>
>> >> 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.
>> + gettext_noop("Sets the maximum number of wal sender connections and
>> reserves "
>> + "them from being used non-superuser connections."),
>>
>> "them from being used 'by' non-superuser connections."
>> 'by' is missing in above line.
>>
>> 1.
>> if (ReservedBackends >= MaxConnections)
>> {
>> write_stderr("%s: superuser_reserved_connections must be less than
>> max_connections\n", progname);
>> ExitPostmaster(1);
>> }
>>
>> I think we should have one check similar to above for (max_wal_senders
>> + ReservedBackends >= MaxConnections) to avoid server starting
>> with values, where not even 1 non-superuser connection will be
>> allowed. I think this is a reasoning similar to why the check for
>> ReservedBackends exists.
>
> I have added this, see comment above.
>
>> With Regards,
>> Amit Kapila.
>> EnterpriseDB: http://www.enterprisedb.com
>>
>
> I would be glad, if you could also test the patch again, as I'm nearly
> code blind after testing it for 4 hours.
> I had the problem, that I could not attach as many replication
> connections as I wanted, as they were denied as normal connections. I
> think I got it fixed, but I'm not 100% sure at the moment.
> After some sleep, I will read the code again and test it again, to make
> sure, it really does what it is supposed to do.

You have forgotten to attach the patch. However, now it is important
to first get the consensus on approach to do this feature, currently
there are 3 approaches:
1. Have replication_reserved_connections as a separate parameter to
reserve connections for replication
2. Consider max_wal_sender to reserve connections for replication
3. Treat replication connections as a pool outside max_connections

Apart from above approaches, we need to think how user can view the
usage of connections, as pg_stat_activity doesn't show replication
connections, so its difficult for user to see how the connections are
used.

I am really not sure what is best way to goahead from here, but I
think it might be helpful if we can study some use cases or how other
databases solve this problem.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Oct 17, 2013 at 8:57 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Oct 16, 2013 at 4:30 AM, Gibheer <gibheer(at)zero-knowledge(dot)org> wrote:
>> On Mon, 14 Oct 2013 11:52:57 +0530
>> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>>> On Sun, Oct 13, 2013 at 2:08 PM, Gibheer <gibheer(at)zero-knowledge(dot)org>
>>> wrote:
>>> > On Sun, 13 Oct 2013 11:38:17 +0530
>>> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> >
>>> >> On Thu, Oct 10, 2013 at 3:17 AM, Gibheer
>>> >> <gibheer(at)zero-knowledge(dot)org> wrote:
>>> >> > On Mon, 7 Oct 2013 11:39:55 +0530
>>> >> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> >> >> Robert Haas wrote:
>>> >> >> On Mon, Aug 5, 2013 at 2:04 AM, Andres Freund
>>> >> >> <andres(at)2ndquadrant(dot)com> wrote:
>> I would be glad, if you could also test the patch again, as I'm nearly
>> code blind after testing it for 4 hours.
>> I had the problem, that I could not attach as many replication
>> connections as I wanted, as they were denied as normal connections. I
>> think I got it fixed, but I'm not 100% sure at the moment.
>> After some sleep, I will read the code again and test it again, to make
>> sure, it really does what it is supposed to do.
>
> You have forgotten to attach the patch. However, now it is important
> to first get the consensus on approach to do this feature, currently
> there are 3 approaches:
> 1. Have replication_reserved_connections as a separate parameter to
> reserve connections for replication
> 2. Consider max_wal_sender to reserve connections for replication
> 3. Treat replication connections as a pool outside max_connections
>
> Apart from above approaches, we need to think how user can view the
> usage of connections, as pg_stat_activity doesn't show replication
> connections, so its difficult for user to see how the connections are
> used.
>
> I am really not sure what is best way to goahead from here, but I
> think it might be helpful if we can study some use cases or how other
> databases solve this problem.

Today I spent some time seeing how other databases (in particular
MySQL) achieve it. There seems to be no separate way to configure
replication connections, rather if user faced with
too_many_connections
(https://dev.mysql.com/doc/refman/5.5/en/too-many-connections.html),
they allow one spare connection (super user connection) to check what
all connections are doing, it seems all connections can be viewed
through one common command Show ProcessList
(https://dev.mysql.com/doc/refman/5.5/en/show-processlist.html).

By above, I don't mean that we should only do what other databases
have done, rather it is towards trying to find a way which can be
useful for users of Postgresql.

Your views/thoughts?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sat, 19 Oct 2013 12:09:57 +0530
Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Thu, Oct 17, 2013 at 8:57 AM, Amit Kapila
> <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Wed, Oct 16, 2013 at 4:30 AM, Gibheer
> > <gibheer(at)zero-knowledge(dot)org> wrote:
> >> On Mon, 14 Oct 2013 11:52:57 +0530
> >> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >>
> >>> On Sun, Oct 13, 2013 at 2:08 PM, Gibheer
> >>> <gibheer(at)zero-knowledge(dot)org> wrote:
> >>> > On Sun, 13 Oct 2013 11:38:17 +0530
> >>> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >>> >
> >>> >> On Thu, Oct 10, 2013 at 3:17 AM, Gibheer
> >>> >> <gibheer(at)zero-knowledge(dot)org> wrote:
> >>> >> > On Mon, 7 Oct 2013 11:39:55 +0530
> >>> >> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >>> >> >> Robert Haas wrote:
> >>> >> >> On Mon, Aug 5, 2013 at 2:04 AM, Andres Freund
> >>> >> >> <andres(at)2ndquadrant(dot)com> wrote:
> >> I would be glad, if you could also test the patch again, as I'm
> >> nearly code blind after testing it for 4 hours.
> >> I had the problem, that I could not attach as many replication
> >> connections as I wanted, as they were denied as normal
> >> connections. I think I got it fixed, but I'm not 100% sure at the
> >> moment. After some sleep, I will read the code again and test it
> >> again, to make sure, it really does what it is supposed to do.
> >
> > You have forgotten to attach the patch. However, now it is important
> > to first get the consensus on approach to do this feature, currently
> > there are 3 approaches:
> > 1. Have replication_reserved_connections as a separate parameter to
> > reserve connections for replication
> > 2. Consider max_wal_sender to reserve connections for replication
> > 3. Treat replication connections as a pool outside max_connections
> >
> > Apart from above approaches, we need to think how user can view the
> > usage of connections, as pg_stat_activity doesn't show replication
> > connections, so its difficult for user to see how the connections
> > are used.
> >
> > I am really not sure what is best way to goahead from here, but I
> > think it might be helpful if we can study some use cases or how
> > other databases solve this problem.
>
> Today I spent some time seeing how other databases (in particular
> MySQL) achieve it. There seems to be no separate way to configure
> replication connections, rather if user faced with
> too_many_connections
> (https://dev.mysql.com/doc/refman/5.5/en/too-many-connections.html),
> they allow one spare connection (super user connection) to check what
> all connections are doing, it seems all connections can be viewed
> through one common command Show ProcessList
> (https://dev.mysql.com/doc/refman/5.5/en/show-processlist.html).
>
> By above, I don't mean that we should only do what other databases
> have done, rather it is towards trying to find a way which can be
> useful for users of Postgresql.
>
> Your views/thoughts?
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>

Hi,

I have accessto MySQL and PostgreSQL at work and it is true, that MySQL
has not separate pools. It also happend to us, that we lost connection
from a slave and it was unable to get back into replication on MySQL
and Postgres, because of some stupid applications.
One difference is, like you said, that replication connections are
listed in `show processlist`, where replication connections in postgres
are listed in a seperate view from the rest of the connections. I think
the postgres way is the better in this case, as the picture of the
replication state of the complete cluster can be viewed by one select
on the master. In MySQL it needs one SQL on each slave.

On the other hand, wor on logical replication is done, which will have
predefined slots for it
(http://wiki.postgresql.org/wiki/BDR_User_Guide#max_logical_slots).
This will also consume slots from max_wal_senders
(http://wiki.postgresql.org/wiki/BDR_User_Guide#max_wal_senders). With
that, I think the best approach is to build a pool around replication
only connections, despite it's possible kind. Information about them
will only be available through pg_stat_replication. When a connection
limit is hit, it is clear wether it is a normal connection or a
replication connection and where the user should look for further
information about it.
Also nobody would have to calculate how many connections would have to
be reserved for what service.

I have yet to take a look how background worker connections are handled
(seperate pool or unified with normal connections), because for them
the same limitations apply. We want them running despite the load from
applications or users.

I will report back, when I had more time to look into it.

regards,

Stefan Radomski

On Sun, Oct 20, 2013 at 1:26 AM, Gibheer <gibheer(at)zero-knowledge(dot)org> wrote:
> On Sat, 19 Oct 2013 12:09:57 +0530
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
>> On Thu, Oct 17, 2013 at 8:57 AM, Amit Kapila
>> <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> > On Wed, Oct 16, 2013 at 4:30 AM, Gibheer
>> > <gibheer(at)zero-knowledge(dot)org> wrote:
>> >> On Mon, 14 Oct 2013 11:52:57 +0530
>> >> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> >>
>> >>> On Sun, Oct 13, 2013 at 2:08 PM, Gibheer
>> >>> <gibheer(at)zero-knowledge(dot)org> wrote:
>> >>> > On Sun, 13 Oct 2013 11:38:17 +0530
>> >>> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> >>> >
>> >>> >> On Thu, Oct 10, 2013 at 3:17 AM, Gibheer
>> >>> >> <gibheer(at)zero-knowledge(dot)org> wrote:
>> >>> >> > On Mon, 7 Oct 2013 11:39:55 +0530
>> >>> >> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> >>> >> >> Robert Haas wrote:
>> >>> >> >> On Mon, Aug 5, 2013 at 2:04 AM, Andres Freund
>> >>> >> >> <andres(at)2ndquadrant(dot)com> wrote:
>> >> I would be glad, if you could also test the patch again, as I'm
>> >> nearly code blind after testing it for 4 hours.
>> >> I had the problem, that I could not attach as many replication
>> >> connections as I wanted, as they were denied as normal
>> >> connections. I think I got it fixed, but I'm not 100% sure at the
>> >> moment. After some sleep, I will read the code again and test it
>> >> again, to make sure, it really does what it is supposed to do.
>> >
>> > You have forgotten to attach the patch. However, now it is important
>> > to first get the consensus on approach to do this feature, currently
>> > there are 3 approaches:
>> > 1. Have replication_reserved_connections as a separate parameter to
>> > reserve connections for replication
>> > 2. Consider max_wal_sender to reserve connections for replication
>> > 3. Treat replication connections as a pool outside max_connections
>> >
>> > Apart from above approaches, we need to think how user can view the
>> > usage of connections, as pg_stat_activity doesn't show replication
>> > connections, so its difficult for user to see how the connections
>> > are used.
>> >
>> > I am really not sure what is best way to goahead from here, but I
>> > think it might be helpful if we can study some use cases or how
>> > other databases solve this problem.
>>
>> Today I spent some time seeing how other databases (in particular
>> MySQL) achieve it. There seems to be no separate way to configure
>> replication connections, rather if user faced with
>> too_many_connections
>> (https://dev.mysql.com/doc/refman/5.5/en/too-many-connections.html),
>> they allow one spare connection (super user connection) to check what
>> all connections are doing, it seems all connections can be viewed
>> through one common command Show ProcessList
>> (https://dev.mysql.com/doc/refman/5.5/en/show-processlist.html).
>>
>> By above, I don't mean that we should only do what other databases
>> have done, rather it is towards trying to find a way which can be
>> useful for users of Postgresql.
>>
>> Your views/thoughts?
>>
>> With Regards,
>> Amit Kapila.
>> EnterpriseDB: http://www.enterprisedb.com
>>
>
> Hi,
>
> I have accessto MySQL and PostgreSQL at work and it is true, that MySQL
> has not separate pools. It also happend to us, that we lost connection
> from a slave and it was unable to get back into replication on MySQL
> and Postgres, because of some stupid applications.
> One difference is, like you said, that replication connections are
> listed in `show processlist`, where replication connections in postgres
> are listed in a seperate view from the rest of the connections. I think
> the postgres way is the better in this case, as the picture of the
> replication state of the complete cluster can be viewed by one select
> on the master. In MySQL it needs one SQL on each slave.

Going either way (separate management of replication connections or
unified max_connections), user has to understand how to configure
the system, so that it serves his purpose.
Here I think the important thing is to decide which way it would be
easy for users to understand and configure the system.
As an user, I would be happy with one parameter (max_connections)
rather than having multiple parameters for connection management and
understand each one separately to configure the system. However here
many users would be more comfortable if there are multiple parameters
for configuring the system. I was not sure which way users would like
to configure connection management and neither we had consensus to
proceed, thats why I had checked other database to know how users are
configuring connection management in database and it seems to me that
many users are using single parameter.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 15-10-2013 14:34, Josh Berkus wrote:
> On 10/15/2013 07:36 AM, Robert Haas wrote:
>> On Tue, Oct 15, 2013 at 10:34 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> Josh said we should treat replication connections in a separate "pool"
>>> from normal database connections, right? So you withdraw your earlier
>>> objection to that?
>>
>> I don't think that's what he said. Here's what I was referring to:
>
> To clarify: I do, indeed, support the idea of treating replication
> connections as a pool outside of max_connections. Here's why:
>
+1. I've already faced the same problem Josh described. Even if it is
documented (as in max_wal_senders), it is not easy to figure out why you
can't connect (check 2 parameters and query 2 views). Also, the 'check
connections' task is so complicated in a monitoring tool.

Let's separate the replication connections in another pool (that is not
related to user connections). We already have the infrastructure to
limit replication connections (max_wal_senders), let's use it.

> FATAL: connection limit exceeded for non-superusers
>
> SHOW max_connections;
>
> 100
>
> SELECT COUNT(*) FROM pg_stat_activity;
>
> 94
>
> SHOW superuser_reserved_connections;
>
> 3
>
> ????
>
> ... search around quite a bit, eventually figure out that you have
> three replication connections open. We've already set up an illogical
> and hard-to-troubleshoot situation where replication connections do not
> appear in pg_stat_activity, yet they are counted against max_connections.
>
Another situation is when you can't run pg_basebackup because automated
routines got all of the available connections and the replication user
is not a superuser (even if replication user is a superuser, if some app
run as superuser there won't be slots available).

--
Euler Taveira Timbira - http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento