Re: [HACKERS] Patch for reserved connections for replication users

2013-10-19 Thread Gibheer
On Sat, 19 Oct 2013 12:09:57 +0530
Amit Kapila amit.kapil...@gmail.com wrote:

 On Thu, Oct 17, 2013 at 8:57 AM, Amit Kapila
 amit.kapil...@gmail.com wrote:
  On Wed, Oct 16, 2013 at 4:30 AM, Gibheer
  gibh...@zero-knowledge.org wrote:
  On Mon, 14 Oct 2013 11:52:57 +0530
  Amit Kapila amit.kapil...@gmail.com wrote:
 
  On Sun, Oct 13, 2013 at 2:08 PM, Gibheer
  gibh...@zero-knowledge.org wrote:
   On Sun, 13 Oct 2013 11:38:17 +0530
   Amit Kapila amit.kapil...@gmail.com wrote:
  
   On Thu, Oct 10, 2013 at 3:17 AM, Gibheer
   gibh...@zero-knowledge.org wrote:
On Mon, 7 Oct 2013 11:39:55 +0530
Amit Kapila amit.kapil...@gmail.com wrote:
Robert Haas wrote:
On Mon, Aug 5, 2013 at 2:04 AM, Andres Freund
andres(at)2ndquadrant(dot)com wrote:
  I would be glad, if you could also test the patch again, as I'm
  nearly code blind after testing it for 4 hours.
  I had the problem, that I could not attach as many replication
  connections as I wanted, as they were denied as normal
  connections. I think I got it fixed, but I'm not 100% sure at the
  moment. After some sleep, I will read the code again and test it
  again, to make sure, it really does what it is supposed to do.
 
  You have forgotten to attach the patch. However, now it is important
  to first get the consensus on approach to do this feature, currently
  there are 3 approaches:
  1. Have replication_reserved_connections as a separate parameter to
  reserve connections for replication
  2. Consider max_wal_sender to reserve connections for replication
  3. Treat replication connections as a pool outside max_connections
 
  Apart from above approaches, we need to think how user can view the
  usage of connections, as pg_stat_activity doesn't show replication
  connections, so its difficult for user to see how the connections
  are used.
 
  I am really not sure what is best way to goahead from here, but I
  think it might be helpful if we can study some use cases or how
  other databases solve this problem.
 
 Today I spent some time seeing how other databases (in particular
 MySQL) achieve it. There seems to be no separate way to configure
 replication connections, rather if user faced with
 too_many_connections
 (https://dev.mysql.com/doc/refman/5.5/en/too-many-connections.html),
 they allow one spare connection (super user connection) to check what
 all connections are doing, it seems all connections can be viewed
 through one common command Show ProcessList
 (https://dev.mysql.com/doc/refman/5.5/en/show-processlist.html).
 
 By above, I don't mean that we should only do what other databases
 have done, rather it is towards trying to find a way which can be
 useful for users of Postgresql.
 
 Your views/thoughts?
 
 With Regards,
 Amit Kapila.
 EnterpriseDB: http://www.enterprisedb.com
 

Hi,

I have accessto MySQL and PostgreSQL at work and it is true, that MySQL
has not separate pools. It also happend to us, that we lost connection
from a slave and it was unable to get back into replication on MySQL
and Postgres, because of some stupid applications.
One difference is, like you said, that replication connections are
listed in `show processlist`, where replication connections in postgres
are listed in a seperate view from the rest of the connections. I think
the postgres way is the better in this case, as the picture of the
replication state of the complete cluster can be viewed by one select
on the master. In MySQL it needs one SQL on each slave.

On the other hand, wor on logical replication is done, which will have
predefined slots for it
(http://wiki.postgresql.org/wiki/BDR_User_Guide#max_logical_slots).
This will also consume slots from max_wal_senders
(http://wiki.postgresql.org/wiki/BDR_User_Guide#max_wal_senders). With
that, I think the best approach is to build a pool around replication
only connections, despite it's possible kind. Information about them
will only be available through pg_stat_replication. When a connection
limit is hit, it is clear wether it is a normal connection or a
replication connection and where the user should look for further
information about it.
Also nobody would have to calculate how many connections would have to
be reserved for what service.

I have yet to take a look how background worker connections are handled
(seperate pool or unified with normal connections), because for them
the same limitations apply. We want them running despite the load from
applications or users.

I will report back, when I had more time to look into it.

regards,

Stefan Radomski


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch for reserved connections for replication users

2013-10-15 Thread Gibheer
On Mon, 14 Oct 2013 11:52:57 +0530
Amit Kapila amit.kapil...@gmail.com wrote:

 On Sun, Oct 13, 2013 at 2:08 PM, Gibheer gibh...@zero-knowledge.org
 wrote:
  On Sun, 13 Oct 2013 11:38:17 +0530
  Amit Kapila amit.kapil...@gmail.com wrote:
 
  On Thu, Oct 10, 2013 at 3:17 AM, Gibheer
  gibh...@zero-knowledge.org wrote:
   On Mon, 7 Oct 2013 11:39:55 +0530
   Amit Kapila amit.kapil...@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.
  +varnamesuperuser_reserved_connections/varname
  +varnamemax_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.
 
 + * slots (max_connections - superuser_reserved_connections -
 max_wal_senders) here it should be ReservedBackends rather than
 superuser_reserved_connections.

fixed

  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

Re: [HACKERS] Patch for reserved connections for replication users

2013-10-13 Thread Gibheer
On Sun, 13 Oct 2013 11:38:17 +0530
Amit Kapila amit.kapil...@gmail.com wrote:

 On Thu, Oct 10, 2013 at 3:17 AM, Gibheer gibh...@zero-knowledge.org
 wrote:
  On Mon, 7 Oct 2013 11:39:55 +0530
  Amit Kapila amit.kapil...@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.
 +varnamesuperuser_reserved_connections/varname
 +varnamemax_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

Re: [HACKERS] Patch for reserved connections for replication users

2013-10-11 Thread Gibheer
On Fri, 11 Oct 2013 10:00:51 +0530
Amit Kapila amit.kapil...@gmail.com wrote:

 On Thu, Oct 10, 2013 at 10:06 PM, Gibheer
 gibh...@zero-knowledge.org wrote:
  On Thu, 10 Oct 2013 09:55:24 +0530
  Amit Kapila amit.kapil...@gmail.com wrote:
 
  On Thu, Oct 10, 2013 at 3:17 AM, Gibheer
  gibh...@zero-knowledge.org wrote:
   On Mon, 7 Oct 2013 11:39:55 +0530
   Amit Kapila amit.kapil...@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.
  
   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
 
 
  Mike asked me about the status of the patch a couple days back and I
  didn't think I would be able to work on the patch so soon

Re: [HACKERS] Patch for reserved connections for replication users

2013-10-10 Thread Gibheer
On Thu, 10 Oct 2013 09:55:24 +0530
Amit Kapila amit.kapil...@gmail.com wrote:

 On Thu, Oct 10, 2013 at 3:17 AM, Gibheer gibh...@zero-knowledge.org
 wrote:
  On Mon, 7 Oct 2013 11:39:55 +0530
  Amit Kapila amit.kapil...@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.
 
  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
 

Mike asked me about the status of the patch a couple days back and I
didn't think I would be able to work on the patch so soon again. That's
why I told him to just move the patch into the next commitfest.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch for reserved connections for replication users

2013-10-09 Thread Gibheer
On Mon, 7 Oct 2013 11:39:55 +0530
Amit Kapila amit.kapil...@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.
 
 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 Radomskidiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e8e8e6f..a67cd2f 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -519,7 +519,7 @@ include 'filename'
 varnamemax_connections/ minus
 varnamesuperuser_reserved_connections/varname, new
 connections will be accepted only for superusers, and no
-new replication connections will be accepted.
+new WAL or other connections will be accepted.
/para
 
para
@@ -2167,7 +2167,13 @@ include 'filename'
 Specifies the maximum number of concurrent 

Re: [HACKERS] Patch for reserved connections for replication users

2013-08-04 Thread Gibheer
On Fri, 2 Aug 2013 08:16:15 -0400
Robert Haas robertmh...@gmail.com wrote:

 On Tue, Jul 30, 2013 at 3:10 AM, Gibheer gibh...@zero-knowledge.org
 wrote:
  here is an update off my patch based on the discussion with Marko
  Tiikkaja and Andres Freund.
 
  Marko and I had the idea of introducing reserved connections based
  on roles as it would create a way to garantuee specific roles to
  connect when other roles use up all connections for whatever
  reason. But Andreas said, that it would make connecting take much
  too long.
 
  So to just fix the issue at hand, we decided that adding
  max_wal_senders to the pool of reserved connections is better. With
  that, we are sure that streaming replication can connect to the
  master.
 
  So instead of creating a new configuration option I added
  max_wal_senders to the reserved connections and changed the check
  for new connections.
 
  The test.pl is a small script to test, if the patch does what it
  should.
 
 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()).
 

You are right, that can't be correct. The slots I added with
max_wal_sender would end up as background worker slots. I have to check
my tests again.

In my first patch I just copied the part to limit the connections based
on superuser reserved connections + replication reserved connections.
That did not change the definition of max_connections and made
superuser connections higher in priority than replication connections.
Is that the better approach?

Thank you for your input.

Stefan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Backup throttling

2013-07-31 Thread Gibheer
On Wed, 31 Jul 2013 22:50:19 +0200
Antonin Houska antonin.hou...@gmail.com wrote:

 On 07/31/2013 07:13 AM, Gibheer wrote:
  Hi,
 
  That is a really nice feature.
 I don't pretend it's my idea, I just coded it. My boss proposed the 
 feature as such :-)
  I took a first look at your patch and some empty lines you added
  (e.g. line 60 your patch). Can you remove them?
 Sure, will do in the next version.
  Why did you move localGetCurrentTimestamp() into streamutil.c? Is
  sys/time.h still needed in receivelog.c after the move?
 Because both receivelog.c and pg_basebackup.c need it now. I thought
 I could move localTimestampDifference() and 
 localTimestampDifferenceExceeds() as well for the sake of consistency 
 (these are actually utilities too) but I didn't get convinced enough 
 that the feature alone justifies such a change.
 
 As mentioned in 
 http://www.postgresql.org/message-id/20130731173624.gx14...@eldon.alvh.no-ip.org
  
 these functions ideally shouldn't have separate implementation at
 all. However the problem is that pg_basebackup is not linked to the
 backend.
 
 You're right about sys/time.h, it's included via via streamutil.h.
 I'll fix that too.
  I will try your patch later today to see, if it works.
 
 Whenever you have time. Thanks!
 
 // Tony

Hi,

I got around to test your patch and it works. I've build a small script
for others to test it easily. You can tell it with ROOTDIR and BASEDIR
environment variables where to look for the binaries and where to put
the test directories.

There is only one small thing I hit, namely the error message when the
limit is too small. It was like

transfer rate out of range '10k'

but it does not mention what the actual range is.

Maybe a message like

transfer rate of 10k is too small. Lower limit is 32k.

or

transfer rate has to be between 32k and 1GB, was 10k.

would be better as they mention what the actual limit is. That avoids
that people have to look up the limits in the manual.
We should also add the current limits to the documentation.

regards,

Stefan Radomski

test.sh
Description: application/shellscript

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Patch for reserved connections for replication users

2013-07-30 Thread Gibheer
Hi,

here is an update off my patch based on the discussion with Marko
Tiikkaja and Andres Freund.

Marko and I had the idea of introducing reserved connections based on
roles as it would create a way to garantuee specific roles to connect
when other roles use up all connections for whatever reason. But
Andreas said, that it would make connecting take much too long.

So to just fix the issue at hand, we decided that adding
max_wal_senders to the pool of reserved connections is better. With
that, we are sure that streaming replication can connect to the master.

So instead of creating a new configuration option I added
max_wal_senders to the reserved connections and changed the check for
new connections.

The test.pl is a small script to test, if the patch does what it should.

regards,

Stefan Radomskidiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 23ebc11..2ba98e2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2170,8 +2170,10 @@ include 'filename'
 processes). The default is zero, meaning replication is
 disabled. WAL sender processes count towards the total number
 of connections, so the parameter cannot be set higher than
-xref linkend=guc-max-connections.  This parameter can only
-be set at server start. varnamewal_level/ must be set
+xref linkend=guc-max-connections. Like
+xref linkend=guc-superuser-reserved-connections this option reserves
+connections from xref linkend=guc-max-connections. This parameter
+can only be set at server start. varnamewal_level/ must be set
 to literalarchive/ or literalhot_standby/ to allow
 connections from standby servers.
/para
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 2c7f0f1..3194894 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -436,7 +436,7 @@ InitializeMaxBackends(void)
 
 	/* the extra unit accounts for the autovacuum launcher */
 	MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
-		+ max_worker_processes;
+		+ max_worker_processes + max_wal_senders;
 
 	/* internal error because the values were all checked previously */
 	if (MaxBackends  MAX_BACKENDS)
@@ -705,7 +705,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
 	 * don't allow them to consume the reserved slots, which are intended for
 	 * interactive use.
 	 */
-	if ((!am_superuser || am_walsender) 
+	if ((!am_superuser  !am_walsender) 
 		ReservedBackends  0 
 		!HaveNFreeProcs(ReservedBackends))
 		ereport(FATAL,


test.pl
Description: Perl program

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Backup throttling

2013-07-30 Thread Gibheer
On Wed, 24 Jul 2013 09:20:52 +0200
Antonin Houska antonin.hou...@gmail.com wrote:

 Hello,
 the purpose of this patch is to limit impact of pg_backup on running 
 server. Feedback is appreciated.
 
 // Antonin Houska (Tony)

Hi,

That is a really nice feature. I took a first look at your patch and
some empty lines you added (e.g. line 60 your patch). Can you remove
them?

Why did you move localGetCurrentTimestamp() into streamutil.c? Is
sys/time.h still needed in receivelog.c after the move?

I will try your patch later today to see, if it works.

regards,

Stefan Radomski


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] replication_reserved_connections

2013-07-28 Thread Gibheer
On Sun, 28 Jul 2013 02:23:47 +0200
Marko Tiikkaja ma...@joh.to wrote:

 Hi,
 
 Yesterday an interesting scenario was diagnosed on IRC.  If you're 
 running a synchronous slave and the connection to the slave is lost 
 momentarily, your backends start naturally waiting for the slave to 
 reconnect.  If then your application keeps trying to create new 
 connections, it can use all non-reserved connections, thus locking
 out the synchronous slave when the connection problem has resolved
 itself. This brings the entire cluster into a state where manual
 intervention is necessary.
 
 While you could limit the number of connections for non-replication 
 roles, that's not always possible or desirable.  I would like to 
 introduce a way to reserve connection slots for replication.
 However, it's not clear how this would work.  I looked at how 
 superuser_reserved_connections is implented, and with small changes I 
 could see how to implement two ideas:
 
1) Reserve a portion of superuser_reserved_connections for
 replication connections.  For example, with max_connections=10,
   superuser_reserved_connections=2 and
   replication_reserved_connections=1, at 8 connections either a
   replication connection or a superuser connection can be created,
   and at 9 connections only a superuser one would be allowed.
 This is a bit clumsy as there still aren't guaranteed slots for
   replication.
2) A GUC which says superuser_reserved_connections can be used up
 by replication connections, and then limiting the number of
   replication connections using per-role limits to make sure
   superusers aren't locked out.
 
 Does anyone see a better way to do this?  I'm not too satisfied with 
 either of these ideas.
 
 
 Regards,
 Marko Tiikkaja
 
 

Hi,

I had the same problem and I created a patch to introduce a GUC for
reserved_replication_connections as a seperate flag.
You can find my patch here
https://commitfest.postgresql.org/action/patch_view?id=1180

I am still waiting for feedback though.

regards,

Stefan Radomski


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Patch for reserved connections for replication users

2013-07-11 Thread Gibheer
Hi,

this patch introduces a new configuration flag
replication_reserved_connections to reserve connection slots for
replication in the same way superuser_reserved_connections works for
superusers.

This helps in cases where the application opens connections until
max_connections is reached. A slave would not be able to connect to the
master now and would just be down. With this patch the slave is able to
connect.

This option does not influence the superuser_reserved_connections, so
new slaves are not able to open new connections when the reserved
replication slots are filled.

The only thing this patch is missing are tests. Where should I put them
into the source tree?

thank you,

Stefan Radomskidiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
new file mode 100644
index 6a4d15f..cd6264e
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*** include 'filename'
*** 530,535 
--- 530,562 
/listitem
   /varlistentry
  
+ varlistentry id=guc-replication_reserved_connections
+ xreflabel=replication_reserved_connections
+   termvarnamereplication_reserved_connections/varname/term
+   (typeinteger/type)/term
+   indexterm
+primaryvarnamereplication_reserved_connections/ configuration parameter/primary
+   /indexterm
+   listitem
+para
+ Determines the number of connection quoteslots/quote that
+ are reserved for connections by productnamePostgreSQL/
+ replication users.  At most xref linkend=guc-max-connections
+ connections can ever be active simultaneously.  Whenever the
+ number of active concurrent connections is at least
+ varnamemax_connections/ minus
+ varnamesuperuser_reserved_connections/varname minus
+ varnamereplication_reserved_connections/varname, new
+ connections will be accepted only for replication and superusers.
+/para
+ 
+para
+ The default value is zero connections. The value must be less
+ than the value of varnamemax_connections/varname. This
+ parameter can only be set at server start.
+/para
+ /varlistentry
+ 
   varlistentry id=guc-unix-socket-directories xreflabel=unix_socket_directories
termvarnameunix_socket_directories/varname (typestring/type)/term
indexterm
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
new file mode 100644
index 496192d..ce37b0d
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*** char	   *ListenAddresses;
*** 224,229 
--- 224,238 
   * count against the limit.
   */
  int			ReservedBackends;
+ /*
+  * ReservedReplicationBackends is the number of backends reserved for
+  * replication user use. Like ReservedBackends the number will be taken out
+  * of the pool size given by MaxBackends so the number available to
+  * non-superusers is
+  * (MaxBackends - ReservedBackends - ReservedReplicationBackends).
+  * This option does not block superusers.
+  */
+ int			ReservedReplicationBackends;
  
  /* The socket(s) we're listening to. */
  #define MAXLISTEN	64
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
new file mode 100644
index 2c7f0f1..d0f8f91
*** a/src/backend/utils/init/postinit.c
--- b/src/backend/utils/init/postinit.c
*** InitPostgres(const char *in_dbname, Oid 
*** 700,707 
  	}
  
  	/*
! 	 * The last few connections slots are reserved for superusers. Although
! 	 * replication connections currently require superuser privileges, we
  	 * don't allow them to consume the reserved slots, which are intended for
  	 * interactive use.
  	 */
--- 700,716 
  	}
  
  	/*
! 	 * The last few connections slots are reserved for superusers and replication.
! 	 */
! 	if (!am_superuser 
! 		(ReservedReplicationBackends  0 || ReservedBackends  0) 
! 		!HaveNFreeProcs(ReservedReplicationBackends + ReservedBackends))
! 		ereport(FATAL,
! (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
!  errmsg(remaining connection slots are reserved for replication and superuser connections)));
! 
! 	/*
! 	 * Although replication connections currently require superuser privileges, we
  	 * don't allow them to consume the reserved slots, which are intended for
  	 * interactive use.
  	 */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 5aefd1b..b03d722
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*** static struct config_int ConfigureNamesI
*** 1633,1638 
--- 1633,1647 
  		3, 0, MAX_BACKENDS,
  		NULL, NULL, NULL
  	},
+ 	{
+ 		{replication_reserved_connections, PGC_POSTMASTER, CONN_AUTH_SETTINGS,
+ 			gettext_noop(Sets the number of connection slots reserved for replication.),
+ 			NULL
+ 		},
+ 		ReservedReplicationBackends,
+ 		1, 0, MAX_BACKENDS,
+ 		NULL, NULL, NULL
+ 	},
  
  	/*
  	 * We 

Re: [HACKERS] 9.2 Cascading replication after slave promotion

2012-09-22 Thread Gibheer
On Tue, 14 Aug 2012 10:50:07 -0700
Josh Berkus j...@agliodbs.com wrote:

 
  Yeah, I think there's more people that agree with this use-case
  than you seem to think..  That said, I appreciate that it's not a
  trivial thing to support cleanly.
 
 Not trivial, no, but not major either.  Really what needs to happen is
 for the timeline change record to get transmitted over the WAL stream.
 
 Hmmm.  You know, I bet I could get stream-only remastering working in
 an unsafe way just by disabling the timeline checks.  Time to test ...
 

Isn't that, what recovery_target_timeline in the recovery.conf already
does? It switches to the next timeline after a master migration. See
http://www.postgresql.org/docs/current/static/recovery-target-settings.html
for further information.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] LLVM / clang

2010-06-30 Thread Gibheer
On Fri, 25 Jun 2010 15:49:40 -0400, Peter Eisentraut pete...@gmx.net
wrote:
 
 For the record, here is a patch that would address these issues.
 
 At the moment, I'm waiting to get my hands on the new version 2.7 of
 clang to see if some of these issues have gone away.
 
 Considering that clang already helped us find one bug in the code, I
 think it's worth trying to make this work.

I tried your patch, but it is only working, when I set CLANG=yes. As
I'm not really an expert in makefiles, my first thought was, that it
should work, when I set CC=clang or is it not possible to detect,
which compiler is used?

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers