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.
>>

>
> 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.

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.

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.

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.

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.

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.

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