Re: Patch for reserved connections for replication users

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.