Re: Patch for reserved connections for replication users - Mailing list pgsql-hackers

On Sun, 13 Oct 2013 11:38:17 +0530
Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Thu, Oct 10, 2013 at 3:17 AM, Gibheer <gibheer@zero-knowledge.org>
> wrote:
> > On Mon, 7 Oct 2013 11:39:55 +0530
> > Amit Kapila <amit.kapila16@gmail.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.
> >>
>
> >
> > 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.
>
> If I understand correctly, now the patch has implementation such that
> a. if the number of connections left are (ReservedBackends +
> max_wal_senders), then only superusers or replication connection's
> will be allowed
> b. if the number of connections left are ReservedBackend, then only
> superuser connections will be allowed.

That is correct.

> So it will ensure that max_wal_senders is used for reserving
> connection slots from being used by non-super user connections. I find
> new usage of max_wal_senders acceptable, if anyone else thinks
> otherwise, please let us know.
>
>
> 1.
> +        <varname>superuser_reserved_connections</varname>
> +        <varname>max_wal_senders</varname> only superuser and WAL
> connections
> +        are allowed.
>
> Here minus seems to be missing before max_wal_senders and I think it
> will be better to use replication connections rather than WAL
> connections.

This is fixed.

> 2.
> -        new replication connections will be accepted.
> +        new WAL or other connections will be accepted.
>
> I think as per new implementation, we don't need to change this line.

I reverted that change.

> 3.
> + * reserved slots from max_connections for wal senders. If the
> number of free
> + * slots (max_connections - max_wal_senders) is depleted.
>
>  Above calculation (max_connections - max_wal_senders) needs to
> include super user reserved connections.

My first thought was, that I would not add it here. When superuser
reserved connections are not set, then only max_wal_senders would
count.
But you are right, it has to be set, as 3 connections are reserved by
default for superusers.

> 4.
> + /*
> + * Although replication connections currently require superuser
> privileges, we
> + * don't allow them to consume the superuser reserved slots, which
> are
> + * intended for interactive use.
>   */
>   if ((!am_superuser || am_walsender) &&
>   ReservedBackends > 0 &&
>   !HaveNFreeProcs(ReservedBackends))
>   ereport(FATAL,
>   (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
> - errmsg("remaining connection slots are reserved for non-replication
> superuser connections")));
> + errmsg("remaining connection slots are reserved for superuser
> connections")));
>
> Will there be any problem if we do the above check before the check
> for wal senders and reserved replication connections (+
> !HaveNFreeProcs(max_wal_senders + ReservedBackends))) and don't change
> the error message in this check. I think this will ensure that users
> who doesn't enable max_wal_senders will see the same error message as
> before and the purpose to reserve connections for replication can be
> served by your second check.

I have attached two patches, one that checks only max_wal_senders first
and the other checks reserved_backends first. Both return the original
message, when max_wal_sender is not set and I think it is only a matter
of taste, which comes first.
Me, I would prefer max_wal_senders to get from more connections to less.

> 5.
> + gettext_noop("Sets the maximum number wal sender connections and
> reserves them."),
>
> Sets the maximum number 'of' wal sender connections and reserves them.
> a. 'of' is missing in above line.
> b. I think 'reserves them' is not completely right, because super user
> connections will be allowed. How about if we add something similar
>     to 'and reserves them from being used by non-superuser
> connections' in above line.

This is fixed.

> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>

Thank you again for your feedback.

regards,

Stefan Radomski