Re: Patch for reserved connections for replication users

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