Re: Patch for reserved connections for replication users

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