Re: Patch for reserved connections for replication users

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