Re: Patch for reserved connections for replication users

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