Connection slots reserved for replication

classic Classic list List threaded Threaded
36 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Connection slots reserved for replication

Alexander Kukushkin
Hello hackers,

at the moment it is possible to reserve some amount of connection slots for superusers and this behavior is controlled by superuser_reserved_connections configuration parameter with the default value = 3.

In case if all non-reserved connection slots are busy, replica fails to open a new connection and start streaming from the primary. Such behavior is very bad if you want to run postgresql HA clusters

Initially, replication connections required superuser privileges (in 9.0) and therefore they were deliberately excluded from superuser_reserved_connections. Basically that means it has never been possible to reserve come connection slots for replication connections.

Later (9.1) it became possible to create a user with REPLICATION and NOSUPERUSER options, but comment in the postinit.c still tells that superuser is required.

Now I think now it is a time to go further, and we should make it possible to reserve some connection slots for replication in a manner similar to superuser connections.

How should it work:
1. If we know that we got the replication connection, we just should make sure that there are at least superuser_reserved_connections free connection slots are available.
2. If we know that this is neither superuser nor replication connection, we should check that there are at least (superuser_reserved_connections + NumWalSenders() - max_wal_senders) connection slots are available.

And the last question how to control the number of reserved slots for replication. There are two options:
1. We can introduce a new GUC for that: replication_reserved_connections
2. Or we can just use the value of max_wal_senders

Personally, I more like the second option.


Attached patch implements above described functionality.
Feedback is very appretiated.


Regards,
--
Alexander Kukushkin

replication_reserved_connections.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Masahiko Sawada
On Wed, Aug 1, 2018 at 9:30 PM, Alexander Kukushkin <[hidden email]> wrote:
> Hello hackers,
>
> at the moment it is possible to reserve some amount of connection slots for
> superusers and this behavior is controlled by superuser_reserved_connections
> configuration parameter with the default value = 3.
>
> In case if all non-reserved connection slots are busy, replica fails to open
> a new connection and start streaming from the primary. Such behavior is very
> bad if you want to run postgresql HA clusters

Yeah, that's also bad if we want to use pg_baseback in the situation.

>
> Initially, replication connections required superuser privileges (in 9.0)
> and therefore they were deliberately excluded from
> superuser_reserved_connections. Basically that means it has never been
> possible to reserve come connection slots for replication connections.
>
> Later (9.1) it became possible to create a user with REPLICATION and
> NOSUPERUSER options, but comment in the postinit.c still tells that
> superuser is required.
>
> Now I think now it is a time to go further, and we should make it possible
> to reserve some connection slots for replication in a manner similar to
> superuser connections.
>

+1

> How should it work:
> 1. If we know that we got the replication connection, we just should make
> sure that there are at least superuser_reserved_connections free connection
> slots are available.
> 2. If we know that this is neither superuser nor replication connection, we
> should check that there are at least (superuser_reserved_connections +
> NumWalSenders() - max_wal_senders) connection slots are available.

You wanted to mean (superuser_reserved_connections + max_wal_senders -
NumWalSenders()) in the second point?

>
> And the last question how to control the number of reserved slots for
> replication. There are two options:
> 1. We can introduce a new GUC for that: replication_reserved_connections
> 2. Or we can just use the value of max_wal_senders
>
> Personally, I more like the second option.
>

One argrable point of the second option could be that it breaks
backward compatibility of the parameter configurations. That is, the
existing systems need to re-configure the max_connections. So it might
be better to take the first option with
replication_reservd_connections = 0 by default.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Alexander Kukushkin
Hi,

2018-09-14 12:23 GMT+02:00 Masahiko Sawada <[hidden email]>:

>> 2. If we know that this is neither superuser nor replication connection, we
>> should check that there are at least (superuser_reserved_connections +
>> NumWalSenders() - max_wal_senders) connection slots are available.
>
> You wanted to mean (superuser_reserved_connections + max_wal_senders -
> NumWalSenders()) in the second point?

Sure, my bad. Did a mistake when writing an email, but in the attached
file it looks good.

>
> One argrable point of the second option could be that it breaks
> backward compatibility of the parameter configurations. That is, the
> existing systems need to re-configure the max_connections. So it might
> be better to take the first option with
> replication_reservd_connections = 0 by default.

Please find attached the new version of the patch, which introduces
replication_reservd_connections GUC

Regards,
--
Alexander Kukushkin

replication_reserved_connections-v2.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Kyotaro HORIGUCHI-2
Hello.

I agree to the objective.

At Mon, 17 Sep 2018 14:25:56 +0200, Alexander Kukushkin <[hidden email]> wrote in <CAFh8B=[hidden email]>

> Hi,
>
> 2018-09-14 12:23 GMT+02:00 Masahiko Sawada <[hidden email]>:
>
> >> 2. If we know that this is neither superuser nor replication connection, we
> >> should check that there are at least (superuser_reserved_connections +
> >> NumWalSenders() - max_wal_senders) connection slots are available.
> >
> > You wanted to mean (superuser_reserved_connections + max_wal_senders -
> > NumWalSenders()) in the second point?
>
> Sure, my bad. Did a mistake when writing an email, but in the attached
> file it looks good.
>
> >
> > One argrable point of the second option could be that it breaks
> > backward compatibility of the parameter configurations. That is, the
> > existing systems need to re-configure the max_connections. So it might
> > be better to take the first option with
> > replication_reservd_connections = 0 by default.
>
> Please find attached the new version of the patch, which introduces
> replication_reservd_connections GUC

With this patch, non-superuser is rejected if there are less than
super_res_conn + (remain of repl_res_conn). It gives the same
effect for walsenders with just sharing
superuser_reserved_connection by superusers and walsenders.

Instaed, we can iterally "reserve" connection slots for the
specific use by providing ProcGlobal->walsenderFreeProcs. The
slots are never stolen for other usage. Allowing additional
walsenders comes after all the slots are filled to grab an
available "normal" slot, it works as the same as the current
behavior when walsender_reserved_connectsions = 0.

What do you think about this?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Alexander Kukushkin
Hi,

On 20 September 2018 at 08:18, Kyotaro HORIGUCHI
<[hidden email]> wrote:

>
> Instaed, we can iterally "reserve" connection slots for the
> specific use by providing ProcGlobal->walsenderFreeProcs. The
> slots are never stolen for other usage. Allowing additional
> walsenders comes after all the slots are filled to grab an
> available "normal" slot, it works as the same as the current
> behavior when walsender_reserved_connectsions = 0.
>
> What do you think about this?

Sounds reasonable, please see the updated patch.

Regards,
--
Alexander Kukushkin

replication_reserved_connections-v3.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Alexander Kukushkin
Hi,

Attached rebased version patch to the current HEAD and created commit fest entry
On Fri, 21 Sep 2018 at 13:43, Alexander Kukushkin <[hidden email]> wrote:

>
> Hi,
>
> On 20 September 2018 at 08:18, Kyotaro HORIGUCHI
> <[hidden email]> wrote:
>
> >
> > Instaed, we can iterally "reserve" connection slots for the
> > specific use by providing ProcGlobal->walsenderFreeProcs. The
> > slots are never stolen for other usage. Allowing additional
> > walsenders comes after all the slots are filled to grab an
> > available "normal" slot, it works as the same as the current
> > behavior when walsender_reserved_connectsions = 0.
> >
> > What do you think about this?
>
> Sounds reasonable, please see the updated patch.
>
> Regards,
> --
> Alexander Kukushkin

replication_reserved_connections-v4.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Kyotaro HORIGUCHI-2
Hello. Thank you for the new version.

At Thu, 1 Nov 2018 11:58:52 +0100, Alexander Kukushkin <[hidden email]> wrote in <CAFh8B=[hidden email]>

> Hi,
>
> Attached rebased version patch to the current HEAD and created commit fest entry
> On Fri, 21 Sep 2018 at 13:43, Alexander Kukushkin <[hidden email]> wrote:
> >
> > Hi,
> >
> > On 20 September 2018 at 08:18, Kyotaro HORIGUCHI
> > <[hidden email]> wrote:
> >
> > >
> > > Instaed, we can iterally "reserve" connection slots for the
> > > specific use by providing ProcGlobal->walsenderFreeProcs. The
> > > slots are never stolen for other usage. Allowing additional
> > > walsenders comes after all the slots are filled to grab an
> > > available "normal" slot, it works as the same as the current
> > > behavior when walsender_reserved_connectsions = 0.
> > >
> > > What do you think about this?
> >
> > Sounds reasonable, please see the updated patch.
InitializeMaxBackends()
  MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
- max_worker_processes;
+ max_worker_processes + replication_reserved_connections;

This means walsender doesn't comsume a connection, which is
different from the current behavior. We should reserve a part of
MaxConnections for walsenders.  (in PostmasterMain,
max_wal_senders is counted as a part of MaxConnections)

+ if (am_walsender && replication_reserved_connections < max_wal_senders
+ && *procgloballist == NULL)
+ procgloballist = &ProcGlobal->freeProcs;

Currently exccesive number of walsenders are rejected in
InitWalSenderSlot and emit the following error.

>    ereport(FATAL,
>        (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
>         errmsg("number of requested standby connections "
>            "exceeds max_wal_senders (currently %d)",
>            max_wal_senders)));

With this patch, if max_wal_senders =
replication_reserved_connections = 3 and the fourth walreceiver
comes, we will get "FATAL: sorry, too many clients already"
instead. It should be fixed.

When r_r_conn = 2 and max_wal_senders = 3 and the three
walsenders are active, in an exreme case where a new replication
connection comes at the same time another is exiting, we could
end up using two normal slots despite that one slot is vacant in
reserved slots. If we want to avoid the case, we need to limit
the number of normal slots to use. I don't think it is acceptable
as is but basically something like the attached would do that.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 3b6c636077..f86c05e8e0 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2250,6 +2250,7 @@ static void
 InitWalSenderSlot(void)
 {
  int i;
+ bool reject = false;
 
  /*
  * WalSndCtl should be set up already (we inherit this by fork() or
@@ -2258,41 +2259,63 @@ InitWalSenderSlot(void)
  Assert(WalSndCtl != NULL);
  Assert(MyWalSnd == NULL);
 
+ /* limit the maximum number of non-reserved slots to use */
+ if (MyProc->procgloballist == &ProcGlobal->freeProcs)
+ {
+ int max_normal_slots
+ = max_wal_senders - replication_reserved_connections;
+
+ if (max_normal_slots <= 0)
+ {
+ /* we mustn't use a normal slot */
+ reject = true;
+ }
+ else if (pg_atomic_add_fetch_u32(&WalSndCtl->n_using_normal_slots, 1)
+ > max_normal_slots)
+ {
+ pg_atomic_sub_fetch_u32(&WalSndCtl->n_using_normal_slots, 1);
+ reject = true;
+ }
+ }
+
  /*
  * Find a free walsender slot and reserve it. If this fails, we must be
  * out of WalSnd structures.
  */
- for (i = 0; i < max_wal_senders; i++)
+ if (!reject)
  {
- WalSnd   *walsnd = &WalSndCtl->walsnds[i];
-
- SpinLockAcquire(&walsnd->mutex);
-
- if (walsnd->pid != 0)
+ for (i = 0; i < max_wal_senders; i++)
  {
- SpinLockRelease(&walsnd->mutex);
- continue;
- }
- else
- {
- /*
- * Found a free slot. Reserve it for us.
- */
- walsnd->pid = MyProcPid;
- walsnd->sentPtr = InvalidXLogRecPtr;
- walsnd->write = InvalidXLogRecPtr;
- walsnd->flush = InvalidXLogRecPtr;
- walsnd->apply = InvalidXLogRecPtr;
- walsnd->writeLag = -1;
- walsnd->flushLag = -1;
- walsnd->applyLag = -1;
- walsnd->state = WALSNDSTATE_STARTUP;
- walsnd->latch = &MyProc->procLatch;
- SpinLockRelease(&walsnd->mutex);
- /* don't need the lock anymore */
- MyWalSnd = (WalSnd *) walsnd;
+ WalSnd   *walsnd = &WalSndCtl->walsnds[i];
 
- break;
+ SpinLockAcquire(&walsnd->mutex);
+
+ if (walsnd->pid != 0)
+ {
+ SpinLockRelease(&walsnd->mutex);
+ continue;
+ }
+ else
+ {
+ /*
+ * Found a free slot. Reserve it for us.
+ */
+ walsnd->pid = MyProcPid;
+ walsnd->sentPtr = InvalidXLogRecPtr;
+ walsnd->write = InvalidXLogRecPtr;
+ walsnd->flush = InvalidXLogRecPtr;
+ walsnd->apply = InvalidXLogRecPtr;
+ walsnd->writeLag = -1;
+ walsnd->flushLag = -1;
+ walsnd->applyLag = -1;
+ walsnd->state = WALSNDSTATE_STARTUP;
+ walsnd->latch = &MyProc->procLatch;
+ SpinLockRelease(&walsnd->mutex);
+ /* don't need the lock anymore */
+ MyWalSnd = (WalSnd *) walsnd;
+
+ break;
+ }
  }
  }
  if (MyWalSnd == NULL)
@@ -2322,6 +2345,10 @@ WalSndKill(int code, Datum arg)
  /* Mark WalSnd struct as no longer being in use. */
  walsnd->pid = 0;
  SpinLockRelease(&walsnd->mutex);
+
+ /* decrement usage count of normal connection slots if needed */
+ if (MyProc->procgloballist == &ProcGlobal->freeProcs)
+ pg_atomic_sub_fetch_u32(&WalSndCtl->n_using_normal_slots, 1);
 }
 
 /*
@@ -3047,6 +3074,9 @@ WalSndShmemInit(void)
 
  SpinLockInit(&walsnd->mutex);
  }
+
+ /* set the number of non-reserved slots walsenders can use */
+ pg_atomic_init_u32(&WalSndCtl->n_using_normal_slots, 0);
  }
 }
 
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 2d04a8204a..c461cf2f12 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -333,8 +333,7 @@ InitProcess(void)
  * Try to use ProcGlobal->freeProcs as a fallback when
  * all reserved walsender slots are already busy.
  */
- if (am_walsender && replication_reserved_connections < max_wal_senders
- && *procgloballist == NULL)
+ if (am_walsender && *procgloballist == NULL)
  procgloballist = &ProcGlobal->freeProcs;
 
  MyProc = *procgloballist;
diff --git a/src/include/replication/walsender_private.h b/src/include/replication/walsender_private.h
index 4b90477936..bc84c287a4 100644
--- a/src/include/replication/walsender_private.h
+++ b/src/include/replication/walsender_private.h
@@ -14,6 +14,7 @@
 
 #include "access/xlog.h"
 #include "nodes/nodes.h"
+#include "port/atomics.h"
 #include "replication/syncrep.h"
 #include "storage/latch.h"
 #include "storage/shmem.h"
@@ -101,6 +102,8 @@ typedef struct
  */
  bool sync_standbys_defined;
 
+ pg_atomic_uint32 n_using_normal_slots;
+
  WalSnd walsnds[FLEXIBLE_ARRAY_MEMBER];
 } WalSndCtlData;
 
Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Masahiko Sawada
On Tue, Nov 6, 2018 at 9:16 PM Kyotaro HORIGUCHI
<[hidden email]> wrote:

>
> Hello. Thank you for the new version.
>
> At Thu, 1 Nov 2018 11:58:52 +0100, Alexander Kukushkin <[hidden email]> wrote in <CAFh8B=[hidden email]>
> > Hi,
> >
> > Attached rebased version patch to the current HEAD and created commit fest entry
> > On Fri, 21 Sep 2018 at 13:43, Alexander Kukushkin <[hidden email]> wrote:
> > >
> > > Hi,
> > >
> > > On 20 September 2018 at 08:18, Kyotaro HORIGUCHI
> > > <[hidden email]> wrote:
> > >
> > > >
> > > > Instaed, we can iterally "reserve" connection slots for the
> > > > specific use by providing ProcGlobal->walsenderFreeProcs. The
> > > > slots are never stolen for other usage. Allowing additional
> > > > walsenders comes after all the slots are filled to grab an
> > > > available "normal" slot, it works as the same as the current
> > > > behavior when walsender_reserved_connectsions = 0.
> > > >
> > > > What do you think about this?
> > >
> > > Sounds reasonable, please see the updated patch.
>
> InitializeMaxBackends()
>         MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
> -               max_worker_processes;
> +               max_worker_processes + replication_reserved_connections;
>
> This means walsender doesn't comsume a connection, which is
> different from the current behavior. We should reserve a part of
> MaxConnections for walsenders.  (in PostmasterMain,
> max_wal_senders is counted as a part of MaxConnections)

Yes. We can force replication_reserved_connections <= max_wal_senders
and then reserved connections for replication should be a part of
MaxConnections.

>
> +       if (am_walsender && replication_reserved_connections < max_wal_senders
> +                       && *procgloballist == NULL)
> +               procgloballist = &ProcGlobal->freeProcs;
>
> Currently exccesive number of walsenders are rejected in
> InitWalSenderSlot and emit the following error.
>
> >    ereport(FATAL,
> >        (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
> >         errmsg("number of requested standby connections "
> >            "exceeds max_wal_senders (currently %d)",
> >            max_wal_senders)));
>
> With this patch, if max_wal_senders =
> replication_reserved_connections = 3 and the fourth walreceiver
> comes, we will get "FATAL: sorry, too many clients already"
> instead. It should be fixed.
>
> When r_r_conn = 2 and max_wal_senders = 3 and the three
> walsenders are active, in an exreme case where a new replication
> connection comes at the same time another is exiting, we could
> end up using two normal slots despite that one slot is vacant in
> reserved slots.

Doesn't the max_wal_senders prevent the case?

Wal senders can get connection if we have free procs more than
(MaxConnections - reserved for superusers). So I think for normal
users the connection must be refused if (MaxConnections - (reserved
for superuser and replication) > # of freeprocs) and for wal senders
the connection also must be refused if (MaxConnections - (reserved for
superuser) > # of freeprocs). I'm not sure we need such trick in
InitWalSenderSlot().

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATIONNTT Open Source Software Center

Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Kyotaro HORIGUCHI-2
Hello.

At Wed, 7 Nov 2018 19:31:00 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoASCq808+iqcFoVuLu-+i8kon=6wN3+sY=[hidden email]>

> On Tue, Nov 6, 2018 at 9:16 PM Kyotaro HORIGUCHI
> <[hidden email]> wrote:
> > InitializeMaxBackends()
> >         MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
> > -               max_worker_processes;
> > +               max_worker_processes + replication_reserved_connections;
> >
> > This means walsender doesn't comsume a connection, which is
> > different from the current behavior. We should reserve a part of
> > MaxConnections for walsenders.  (in PostmasterMain,
> > max_wal_senders is counted as a part of MaxConnections)
>
> Yes. We can force replication_reserved_connections <= max_wal_senders
> and then reserved connections for replication should be a part of
> MaxConnections.
>
> >
> > +       if (am_walsender && replication_reserved_connections < max_wal_senders
> > +                       && *procgloballist == NULL)
> > +               procgloballist = &ProcGlobal->freeProcs;
> >
> > Currently exccesive number of walsenders are rejected in
> > InitWalSenderSlot and emit the following error.
> >
> > >    ereport(FATAL,
> > >        (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
> > >         errmsg("number of requested standby connections "
> > >            "exceeds max_wal_senders (currently %d)",
> > >            max_wal_senders)));
> >
> > With this patch, if max_wal_senders =
> > replication_reserved_connections = 3 and the fourth walreceiver
> > comes, we will get "FATAL: sorry, too many clients already"
> > instead. It should be fixed.
> >
> > When r_r_conn = 2 and max_wal_senders = 3 and the three
> > walsenders are active, in an exreme case where a new replication
> > connection comes at the same time another is exiting, we could
> > end up using two normal slots despite that one slot is vacant in
> > reserved slots.
>
> Doesn't the max_wal_senders prevent the case?

Currently the variable doesn't work as so. We once accept the
connection request and searches for a vacant slot in
InitWalSenderSlot and reject the connection if it found that no
room is available. Even with this patch, we don't count the
accurate number of active walsenders (for performance reason). If
reserved slot are filled, there's no way other than to accept the
connection using non-reserved slot if r_r_conn <
max_wal_senders. If one of active walsenders went away since we
allocated non-reserved connection slot until InitWalSenderSlot
starts searching sendnds[] array. Finally the new walsender on
the unreserved slot is activated, and one reserved slot is left
empty. So this is "an extreme case". We could ignore the case.

I'm doubt that we should allow the setting where r_r_conn <
max_wal_senders, or even r_r_conn != max_wal_senders. We don't
have a problem like this if we don't allow the cases.

> Wal senders can get connection if we have free procs more than
> (MaxConnections - reserved for superusers). So I think for normal
> users the connection must be refused if (MaxConnections - (reserved
> for superuser and replication) > # of freeprocs) and for wal senders
> the connection also must be refused if (MaxConnections - (reserved for
> superuser) > # of freeprocs). I'm not sure we need such trick in
> InitWalSenderSlot().

(For clarity, I don't mean my previous patch is good solution.)

It works as far as we accept that some reserved slots can be left
unused despite of some walsenders are using normal slots. (Just
exiting a walsender using reserved slot causes this but it is
usually occupied by walsenders comes later)

Another idea is we acquire a walsnd[] slot before getting a
connection slot..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Masahiko Sawada
On Thu, Nov 8, 2018 at 9:30 PM Kyotaro HORIGUCHI
<[hidden email]> wrote:

>
> Hello.
>
> At Wed, 7 Nov 2018 19:31:00 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoASCq808+iqcFoVuLu-+i8kon=6wN3+sY=[hidden email]>
> > On Tue, Nov 6, 2018 at 9:16 PM Kyotaro HORIGUCHI
> > <[hidden email]> wrote:
> > > InitializeMaxBackends()
> > >         MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
> > > -               max_worker_processes;
> > > +               max_worker_processes + replication_reserved_connections;
> > >
> > > This means walsender doesn't comsume a connection, which is
> > > different from the current behavior. We should reserve a part of
> > > MaxConnections for walsenders.  (in PostmasterMain,
> > > max_wal_senders is counted as a part of MaxConnections)
> >
> > Yes. We can force replication_reserved_connections <= max_wal_senders
> > and then reserved connections for replication should be a part of
> > MaxConnections.
> >
> > >
> > > +       if (am_walsender && replication_reserved_connections < max_wal_senders
> > > +                       && *procgloballist == NULL)
> > > +               procgloballist = &ProcGlobal->freeProcs;
> > >
> > > Currently exccesive number of walsenders are rejected in
> > > InitWalSenderSlot and emit the following error.
> > >
> > > >    ereport(FATAL,
> > > >        (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
> > > >         errmsg("number of requested standby connections "
> > > >            "exceeds max_wal_senders (currently %d)",
> > > >            max_wal_senders)));
> > >
> > > With this patch, if max_wal_senders =
> > > replication_reserved_connections = 3 and the fourth walreceiver
> > > comes, we will get "FATAL: sorry, too many clients already"
> > > instead. It should be fixed.
> > >
> > > When r_r_conn = 2 and max_wal_senders = 3 and the three
> > > walsenders are active, in an exreme case where a new replication
> > > connection comes at the same time another is exiting, we could
> > > end up using two normal slots despite that one slot is vacant in
> > > reserved slots.
> >
> > Doesn't the max_wal_senders prevent the case?
>
> Currently the variable doesn't work as so. We once accept the
> connection request and searches for a vacant slot in
> InitWalSenderSlot and reject the connection if it found that no
> room is available. Even with this patch, we don't count the
> accurate number of active walsenders (for performance reason). If
> reserved slot are filled, there's no way other than to accept the
> connection using non-reserved slot if r_r_conn <
> max_wal_senders. If one of active walsenders went away since we
> allocated non-reserved connection slot until InitWalSenderSlot
> starts searching sendnds[] array. Finally the new walsender on
> the unreserved slot is activated, and one reserved slot is left
> empty. So this is "an extreme case". We could ignore the case.
>
> I'm doubt that we should allow the setting where r_r_conn <
> max_wal_senders, or even r_r_conn != max_wal_senders. We don't
> have a problem like this if we don't allow the cases.
>
> > Wal senders can get connection if we have free procs more than
> > (MaxConnections - reserved for superusers). So I think for normal
> > users the connection must be refused if (MaxConnections - (reserved
> > for superuser and replication) > # of freeprocs) and for wal senders
> > the connection also must be refused if (MaxConnections - (reserved for
> > superuser) > # of freeprocs). I'm not sure we need such trick in
> > InitWalSenderSlot().
>
> (For clarity, I don't mean my previous patch is good solution.)
>
> It works as far as we accept that some reserved slots can be left
> unused despite of some walsenders are using normal slots. (Just
> exiting a walsender using reserved slot causes this but it is
> usually occupied by walsenders comes later)
>
> Another idea is we acquire a walsnd[] slot before getting a
> connection slot..

After more thought, I'm inclined to agree to reserve max_wal_senders
slots and not to have replication_reserved_connections parameter.

For superuser_reserved_connection, actually it works so that we
certainly reserve slots for superuser in case where slots are almost
full regardless of who is using other slots incluing superusers
themselves. But replication connections requires different behaviour
as it has the another limit (max_wal_senders). If we have
replication_reserved_connections < max_wal_senders, it could end up
with the same issue as what originally reported on this thread.
Therefore many users would set replication_reserved_connections =
max_wal_senders.

On the other hand, If we always reserve max_wal_senders slots
available slots for normal backend will get decreased in the next
release, which require for users to re-confiugre the max_connection.
But I felt this behavior seems more natural than the current one, so I
think the re-configuration can be acceptable for users.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATIONNTT Open Source Software Center

Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Magnus Hagander-2
On Fri, Nov 9, 2018 at 2:02 AM Masahiko Sawada <[hidden email]> wrote:
On Thu, Nov 8, 2018 at 9:30 PM Kyotaro HORIGUCHI
<[hidden email]> wrote:
>
> Hello.
>
> At Wed, 7 Nov 2018 19:31:00 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoASCq808+iqcFoVuLu-+i8kon=6wN3+sY=[hidden email]>
> > On Tue, Nov 6, 2018 at 9:16 PM Kyotaro HORIGUCHI
> > <[hidden email]> wrote:
> > > InitializeMaxBackends()
> > >         MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
> > > -               max_worker_processes;
> > > +               max_worker_processes + replication_reserved_connections;
> > >
> > > This means walsender doesn't comsume a connection, which is
> > > different from the current behavior. We should reserve a part of
> > > MaxConnections for walsenders.  (in PostmasterMain,
> > > max_wal_senders is counted as a part of MaxConnections)
> >
> > Yes. We can force replication_reserved_connections <= max_wal_senders
> > and then reserved connections for replication should be a part of
> > MaxConnections.
> >
> > >
> > > +       if (am_walsender && replication_reserved_connections < max_wal_senders
> > > +                       && *procgloballist == NULL)
> > > +               procgloballist = &ProcGlobal->freeProcs;
> > >
> > > Currently exccesive number of walsenders are rejected in
> > > InitWalSenderSlot and emit the following error.
> > >
> > > >    ereport(FATAL,
> > > >        (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
> > > >         errmsg("number of requested standby connections "
> > > >            "exceeds max_wal_senders (currently %d)",
> > > >            max_wal_senders)));
> > >
> > > With this patch, if max_wal_senders =
> > > replication_reserved_connections = 3 and the fourth walreceiver
> > > comes, we will get "FATAL: sorry, too many clients already"
> > > instead. It should be fixed.
> > >
> > > When r_r_conn = 2 and max_wal_senders = 3 and the three
> > > walsenders are active, in an exreme case where a new replication
> > > connection comes at the same time another is exiting, we could
> > > end up using two normal slots despite that one slot is vacant in
> > > reserved slots.
> >
> > Doesn't the max_wal_senders prevent the case?
>
> Currently the variable doesn't work as so. We once accept the
> connection request and searches for a vacant slot in
> InitWalSenderSlot and reject the connection if it found that no
> room is available. Even with this patch, we don't count the
> accurate number of active walsenders (for performance reason). If
> reserved slot are filled, there's no way other than to accept the
> connection using non-reserved slot if r_r_conn <
> max_wal_senders. If one of active walsenders went away since we
> allocated non-reserved connection slot until InitWalSenderSlot
> starts searching sendnds[] array. Finally the new walsender on
> the unreserved slot is activated, and one reserved slot is left
> empty. So this is "an extreme case". We could ignore the case.
>
> I'm doubt that we should allow the setting where r_r_conn <
> max_wal_senders, or even r_r_conn != max_wal_senders. We don't
> have a problem like this if we don't allow the cases.
>
> > Wal senders can get connection if we have free procs more than
> > (MaxConnections - reserved for superusers). So I think for normal
> > users the connection must be refused if (MaxConnections - (reserved
> > for superuser and replication) > # of freeprocs) and for wal senders
> > the connection also must be refused if (MaxConnections - (reserved for
> > superuser) > # of freeprocs). I'm not sure we need such trick in
> > InitWalSenderSlot().
>
> (For clarity, I don't mean my previous patch is good solution.)
>
> It works as far as we accept that some reserved slots can be left
> unused despite of some walsenders are using normal slots. (Just
> exiting a walsender using reserved slot causes this but it is
> usually occupied by walsenders comes later)
>
> Another idea is we acquire a walsnd[] slot before getting a
> connection slot..

After more thought, I'm inclined to agree to reserve max_wal_senders
slots and not to have replication_reserved_connections parameter.

For superuser_reserved_connection, actually it works so that we
certainly reserve slots for superuser in case where slots are almost
full regardless of who is using other slots incluing superusers
themselves. But replication connections requires different behaviour
as it has the another limit (max_wal_senders). If we have
replication_reserved_connections < max_wal_senders, it could end up
with the same issue as what originally reported on this thread.
Therefore many users would set replication_reserved_connections =
max_wal_senders.

On the other hand, If we always reserve max_wal_senders slots
available slots for normal backend will get decreased in the next
release, which require for users to re-confiugre the max_connection.
But I felt this behavior seems more natural than the current one, so I
think the re-configuration can be acceptable for users.


Maybe what we should do instead is not consider max_wal_senders a part of the total number of connections, and instead size the things that needs to be sized by them by max_connections + max_wal_senders. That seems more logical given how the parameters are named as well. 

--
Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Stephen Frost
Greetings,

* Magnus Hagander ([hidden email]) wrote:

> On Fri, Nov 9, 2018 at 2:02 AM Masahiko Sawada <[hidden email]>
> wrote:
> > On the other hand, If we always reserve max_wal_senders slots
> > available slots for normal backend will get decreased in the next
> > release, which require for users to re-confiugre the max_connection.
> > But I felt this behavior seems more natural than the current one, so I
> > think the re-configuration can be acceptable for users.
>
> Maybe what we should do instead is not consider max_wal_senders a part of
> the total number of connections, and instead size the things that needs to
> be sized by them by max_connections + max_wal_senders. That seems more
> logical given how the parameters are named as well.
I tend to agree with having max_connections + max_wal_senders.

Thanks!

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Alexander Kukushkin
Hi,

attaching the new version of the patch.
Now it simply reserves max_wal_senders slots in the ProcGlobal, what
guarantees that only walsender process could use them.

Regards,
--
Alexander Kukushkin

replication_reserved_connections-v5.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Oleksii Kliukin-2

> On 30. Nov 2018, at 13:58, Alexander Kukushkin <[hidden email]> wrote:
>
> attaching the new version of the patch.
> Now it simply reserves max_wal_senders slots in the ProcGlobal, what
> guarantees that only walsender process could use them.

With this patch It looks like InitProcess will trigger the generic error about 'too many clients' before the more specific error message in InitWalSenderSlot when exceeding the number of max_wal_senders.

Does excluding WAL senders from the max_connections limit and including max_wal_senders in MaxBackends also imply that we need to add max_wal_senders to the list at xlog.c: CheckRequiredParameterValues, requiring its value on the replica to be not lower than the one on the primary?

Cheers,
Oleksii
Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Petr Jelinek-4
On 05/12/2018 15:33, Oleksii Kliukin wrote:

>
>> On 30. Nov 2018, at 13:58, Alexander Kukushkin <[hidden email]> wrote:
>>
>> attaching the new version of the patch.
>> Now it simply reserves max_wal_senders slots in the ProcGlobal, what
>> guarantees that only walsender process could use them.
>
> With this patch It looks like InitProcess will trigger the generic error about 'too many clients' before the more specific error message in InitWalSenderSlot when exceeding the number of max_wal_senders.
>
> Does excluding WAL senders from the max_connections limit and including max_wal_senders in MaxBackends also imply that we need to add max_wal_senders to the list at xlog.c: CheckRequiredParameterValues, requiring its value on the replica to be not lower than the one on the primary?
>

I think it does, we need the proc slots for walsenders on the standby
same way we do for normal backends.

--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Alexander Kukushkin
Hi,

On Thu, 6 Dec 2018 at 00:55, Petr Jelinek <[hidden email]> wrote:
> > Does excluding WAL senders from the max_connections limit and including max_wal_senders in MaxBackends also imply that we need to add max_wal_senders to the list at xlog.c: CheckRequiredParameterValues, requiring its value on the replica to be not lower than the one on the primary?
> >
>
> I think it does, we need the proc slots for walsenders on the standby
> same way we do for normal backends.

You are absolutely right. Attaching the new version of the patch.

Regards,
--
Alexander Kukushkin

replication_reserved_connections-v6.patch (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Oleksii Kliukin-2
On Mon, Dec 17, 2018, at 14:10, Alexander Kukushkin wrote:

> Hi,
>
> On Thu, 6 Dec 2018 at 00:55, Petr Jelinek <[hidden email]> wrote:
> > > Does excluding WAL senders from the max_connections limit and including max_wal_senders in MaxBackends also imply that we need to add max_wal_senders to the list at xlog.c: CheckRequiredParameterValues, requiring its value on the replica to be not lower than the one on the primary?
> > >
> >
> > I think it does, we need the proc slots for walsenders on the standby
> > same way we do for normal backends.
>
> You are absolutely right. Attaching the new version of the patch.
Thank you. I've checked that the replica correctly complains when its value of max_wal_senders is lower than the one on the primary at v6.

As stated in my previous comment, I think we should retain the specific error message on exceeding max_wal_senders, instead of showing the generic "too many clients already'.  Attached is the patch that fixes this small thing. I've also rebased it against the master and took a liberty of naming it v7.  It makes me wondering why don't we apply the same level of details to the regular out of connection message and don't show the actual value of max_connections in the error text?

The code diff to the previous version is rather small:


diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index d1a8113cb6..df073673f5 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2273,8 +2273,8 @@ InitWalSenderSlot(void)
  Assert(MyWalSnd == NULL);
 
  /*
- * Find a free walsender slot and reserve it. If this fails, we must be
- * out of WalSnd structures.
+ * Find a free walsender slot and reserve it. This must not fail due
+ * to the prior check for free walsenders at InitProcess.
  */
  for (i = 0; i < max_wal_senders; i++)
  {
@@ -2310,13 +2310,7 @@ InitWalSenderSlot(void)
  break;
  }
  }
- if (MyWalSnd == NULL)
- ereport(FATAL,
- (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
- errmsg("number of requested standby connections "
- "exceeds max_wal_senders (currently %d)",
- max_wal_senders)));
-
+ Assert(MyWalSnd != NULL);
  /* Arrange to clean up at walsender exit */
  on_shmem_exit(WalSndKill, 0);
 }

diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 33387fb71b..fe33fc42e3 100644
@ -341,6 +353,12 @@ InitProcess(void)
  * in the autovacuum case?
  */
  SpinLockRelease(ProcStructLock);
+ if (am_walsender)
+ ereport(FATAL,
+ (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
+ errmsg("number of requested standby connections "
+ "exceeds max_wal_senders (currently %d)",
+ max_wal_senders)));
  ereport(FATAL,
  (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
  errmsg("sorry, too many clients already")));


Cheers,
Oleksii


replication_reserved_connections_v7.patch (22K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Petr Jelinek-4
Hi,

On 02/01/2019 10:21, Oleksii Kliukin wrote:

> On Mon, Dec 17, 2018, at 14:10, Alexander Kukushkin wrote:
>> Hi,
>>
>> On Thu, 6 Dec 2018 at 00:55, Petr Jelinek <[hidden email]> wrote:
>>>> Does excluding WAL senders from the max_connections limit and including max_wal_senders in MaxBackends also imply that we need to add max_wal_senders to the list at xlog.c: CheckRequiredParameterValues, requiring its value on the replica to be not lower than the one on the primary?
>>>>
>>>
>>> I think it does, we need the proc slots for walsenders on the standby
>>> same way we do for normal backends.
>>
>> You are absolutely right. Attaching the new version of the patch.
>
> Thank you. I've checked that the replica correctly complains when its value of max_wal_senders is lower than the one on the primary at v6.
>
> As stated in my previous comment, I think we should retain the specific error message on exceeding max_wal_senders, instead of showing the generic "too many clients already'.  Attached is the patch that fixes this small thing. I've also rebased it against the master and took a liberty of naming it v7.  It makes me wondering why don't we apply the same level of details to the regular out of connection message and don't show the actual value of max_connections in the error text?
>

+1

The patch generally looks good, but the documentation of max_wal_senders
needs updating. In config.sgml we say:

> WAL sender processes count towards the total number
> of connections, so this parameter's value must be less than
> <xref linkend="guc-max-connections"/> minus
> <xref linkend="guc-superuser-reserved-connections"/>.

This is now misleading.

--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Oleksii Kliukin-2


On Wed, Jan 2, 2019, at 11:02, Petr Jelinek wrote:

> The patch generally looks good, but the documentation of max_wal_senders
> needs updating. In config.sgml we say:
>
> > WAL sender processes count towards the total number
> > of connections, so this parameter's value must be less than
> > <xref linkend="guc-max-connections"/> minus
> > <xref linkend="guc-superuser-reserved-connections"/>.
>
> This is now misleading.

Thank you. Attached the new version(called it v8) with the following changes to the documentation:

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e94b305add..6634ce8cdc 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -697,8 +697,7 @@ include_dir 'conf.d'
 
        <para>
         The default value is three connections. The value must be less
-        than <varname>max_connections</varname> minus
-        <xref linkend="guc-max-wal-senders"/>.
+        than <varname>max_connections</varname>.
         This parameter can only be set at server start.
        </para>
       </listitem>
@@ -3453,24 +3452,25 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
        </term>
        <listitem>
        <para>
-        Specifies the maximum number of concurrent connections from
-        standby servers or streaming base backup clients (i.e., the
-        maximum number of simultaneously running WAL sender
-        processes). The default is 10. The value 0 means replication is
-        disabled. WAL sender processes count towards the total number
-        of connections, so this parameter's value must be less than
-        <xref linkend="guc-max-connections"/> minus
-        <xref linkend="guc-superuser-reserved-connections"/>.
-        Abrupt streaming client disconnection might leave an orphaned
-        connection slot behind until
-        a timeout is reached, so this parameter should be set slightly
-        higher than the maximum number of expected clients so disconnected
-        clients can immediately reconnect.  This parameter can only
-        be set at server start.
+        Specifies the maximum number of concurrent connections from standby
+        servers or streaming base backup clients (i.e., the maximum number of
+        simultaneously running WAL sender processes). The default is 10. The
+        value 0 means replication is disabled.  Abrupt streaming client
+        disconnection might leave an orphaned connection slot behind until a
+        timeout is reached, so this parameter should be set slightly higher
+        than the maximum number of expected clients so disconnected clients
+        can immediately reconnect.  This parameter can only be set at server
+        start.
         Also, <varname>wal_level</varname> must be set to
         <literal>replica</literal> or higher to allow connections from standby
         servers.
        </para>
+
+       <para>
+         When running a standby server, you must set this parameter to the
+         same or higher value than on the master server. Otherwise, queries
+         will not be allowed in the standby server.
+        </para>
        </listitem>
       </varlistentry>
 
Cheers,
Oleksii

replication_reserved_connections_v8.patch (25K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Connection slots reserved for replication

Alexander Kukushkin
Hi,

On Wed, 2 Jan 2019 at 12:17, Oleksii Kliukin <[hidden email]> wrote:

> Thank you. Attached the new version(called it v8) with the following changes to the documentation:

Thank you for jumping on it. Your changes look good to me.


I was also thinking about changing the value in PG_CONTROL_VERSION,
because we added the new field into the control file, but decided to
leave this change to committer.

Regards,
--
Alexander Kukushkin

12