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

2013-10-20 Thread Euler Taveira
On 15-10-2013 14:34, Josh Berkus wrote:
 On 10/15/2013 07:36 AM, Robert Haas wrote:
 On Tue, Oct 15, 2013 at 10:34 AM, Andres Freund and...@2ndquadrant.com 
 wrote:
 Josh said we should treat replication connections in a separate pool
 from normal database connections, right? So you withdraw your earlier
 objection to that?

 I don't think that's what he said.  Here's what I was referring to:
 
 To clarify: I do, indeed, support the idea of treating replication
 connections as a pool outside of max_connections.  Here's why:
 
+1. I've already faced the same problem Josh described. Even if it is
documented (as in max_wal_senders), it is not easy to figure out why you
can't connect (check 2 parameters and query 2 views). Also, the 'check
connections' task is so complicated in a monitoring tool.

Let's separate the replication connections in another pool (that is not
related to user connections). We already have the infrastructure to
limit replication connections (max_wal_senders), let's use it.

 FATAL: connection limit exceeded for non-superusers
 
 SHOW max_connections;
 
 100
 
 SELECT COUNT(*) FROM pg_stat_activity;
 
 94
 
 SHOW superuser_reserved_connections;
 
 3
 
 
 
 ... search around quite a bit,  eventually figure out that you have
 three replication connections open.  We've already set up an illogical
 and hard-to-troubleshoot situation where replication connections do not
 appear in pg_stat_activity, yet they are counted against max_connections.
 
Another situation is when you can't run pg_basebackup because automated
routines got all of the available connections and the replication user
is not a superuser (even if replication user is a superuser, if some app
run as superuser there won't be slots available).


-- 
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


-- 
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-19 Thread Amit Kapila
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


-- 
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-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-19 Thread Amit Kapila
On Sun, Oct 20, 2013 at 1:26 AM, Gibheer gibh...@zero-knowledge.org wrote:
 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.

Going either way (separate management of replication connections or
unified max_connections),  user has to understand how to configure
the system, so that it serves his purpose.
Here I think the important thing is to decide which way it would be
easy for users to understand and configure the system.
As an user, I would be happy with one parameter (max_connections)
rather than having multiple parameters for connection management and
understand  each one separately to configure the system. However here
many users would be more comfortable if there are multiple parameters
for configuring the system. I was not sure which way users would like
to configure connection management and neither we had consensus to
proceed, thats why I had checked other database to know how users are
configuring connection management in database and it seems to me that
many users are using single parameter.

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


-- 
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-16 Thread Amit Kapila
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:
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 

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

2013-10-15 Thread Robert Haas
On Tue, Oct 15, 2013 at 12:13 AM, Amit Kapila amit.kapil...@gmail.com wrote:
 If we think this way, then may be we should have max_user_connections
 instead of max_connections and then max_wal_connections. But still
 there are other's like pg_basebackup who needs connections and
 tomorrow there can be new such entities which need connection.
 Also we might need to have different infrastructure in code to make
 these options available to users.
 I think having different parameters to configure maximum connections
 for different entities can complicate both code as well as user's job.

Renaming max_connections is far too big a compatibility break to
consider without far more benefit than what this patch is aiming at.
I'm not prepared to endure the number of beatings I'd have to take if
we did that.

But I also agree that making max_wal_senders act as both a minimum and
a maximum is no good.  +1 to everything Josh Berkus said.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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 Andres Freund
On 2013-10-15 10:29:58 -0400, Robert Haas wrote:
 On Tue, Oct 15, 2013 at 12:13 AM, Amit Kapila amit.kapil...@gmail.com wrote:
  If we think this way, then may be we should have max_user_connections
  instead of max_connections and then max_wal_connections. But still
  there are other's like pg_basebackup who needs connections and
  tomorrow there can be new such entities which need connection.
  Also we might need to have different infrastructure in code to make
  these options available to users.
  I think having different parameters to configure maximum connections
  for different entities can complicate both code as well as user's job.
 
 Renaming max_connections is far too big a compatibility break to
 consider without far more benefit than what this patch is aiming at.
 I'm not prepared to endure the number of beatings I'd have to take if
 we did that.

+many

 But I also agree that making max_wal_senders act as both a minimum and
 a maximum is no good.  +1 to everything Josh Berkus said.

Josh said we should treat replication connections in a separate pool
from normal database connections, right? So you withdraw your earlier
objection to that?

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
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 Robert Haas
On Tue, Oct 15, 2013 at 10:34 AM, Andres Freund and...@2ndquadrant.com wrote:
 But I also agree that making max_wal_senders act as both a minimum and
 a maximum is no good.  +1 to everything Josh Berkus said.

 Josh said we should treat replication connections in a separate pool
 from normal database connections, right? So you withdraw your earlier
 objection to that?

I don't think that's what he said.  Here's what I was referring to:

$ Changing max_wal_senders requires a restart.  As such, we currently
$ advise users to set the setting generously: as many replication
$ connections as you think you'll ever need, plus two.  If
$ max_wal_senders is a reservation which could cause the user to run out
$ of other connections sooner than expected, then the user is faced with a
$ new hard to set parameter: they don't want to set it too high *or* too
$ low.
$
$ This would result in a lot of user frustration as they try to get thier
$ connection configuration right and have to restart the server multiple
$ times.  I find few new features worth making it *harder* to configure
$ PostgreSQL, and reserved replication connections certainly don't qualify.
$
$ If it's worth having reserved replication connections (and I can see
$ some reasons to want it), then we need a new GUC for this:
$ reserved_walsender_connections

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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 Andres Freund
On 2013-10-15 10:36:41 -0400, Robert Haas wrote:
 On Tue, Oct 15, 2013 at 10:34 AM, Andres Freund and...@2ndquadrant.com 
 wrote:
  But I also agree that making max_wal_senders act as both a minimum and
  a maximum is no good.  +1 to everything Josh Berkus said.
 
  Josh said we should treat replication connections in a separate pool
  from normal database connections, right? So you withdraw your earlier
  objection to that?
 
 I don't think that's what he said.  Here's what I was referring to:
 
 $ Changing max_wal_senders requires a restart.  As such, we currently
 $ advise users to set the setting generously: as many replication
 $ connections as you think you'll ever need, plus two.  If
 $ max_wal_senders is a reservation which could cause the user to run out
 $ of other connections sooner than expected, then the user is faced with a
 $ new hard to set parameter: they don't want to set it too high *or* too
 $ low.
 $
 $ This would result in a lot of user frustration as they try to get thier
 $ connection configuration right and have to restart the server multiple
 $ times.  I find few new features worth making it *harder* to configure
 $ PostgreSQL, and reserved replication connections certainly don't qualify.
 $
 $ If it's worth having reserved replication connections (and I can see
 $ some reasons to want it), then we need a new GUC for this:
 $ reserved_walsender_connections

I am referring to
http://archives.postgresql.org/message-id/525C31D3.3010006%40agliodbs.com
:
 On 10/14/2013 10:51 AM, Andres Freund wrote:
  Imo the complications around this prove my (way earlier) point that it'd
  be much better to treat replication connections as something entirely
  different to normal SQL connections. There's really not much overlap
  here and while there's some philosophical point to be made about it all
  being connections, from a practical POV treating them separately seems
  better.
 
 Given that replication connections don't even appear in pg_stat_activity
 now, I'd agree with you.


I still fail to see what the point is in treating those classes of
connections together. It only serves to confuse users and makes
considerations way much more complicated. There's no need for reserved
replication connections or anything like it if would separate the pools.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
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 Josh Berkus
On 10/15/2013 07:36 AM, Robert Haas wrote:
 On Tue, Oct 15, 2013 at 10:34 AM, Andres Freund and...@2ndquadrant.com 
 wrote:
 Josh said we should treat replication connections in a separate pool
 from normal database connections, right? So you withdraw your earlier
 objection to that?
 
 I don't think that's what he said.  Here's what I was referring to:

To clarify: I do, indeed, support the idea of treating replication
connections as a pool outside of max_connections.  Here's why:

FATAL: connection limit exceeded for non-superusers

SHOW max_connections;

100

SELECT COUNT(*) FROM pg_stat_activity;

94

SHOW superuser_reserved_connections;

3



... search around quite a bit,  eventually figure out that you have
three replication connections open.  We've already set up an illogical
and hard-to-troubleshoot situation where replication connections do not
appear in pg_stat_activity, yet they are counted against max_connections.

You could argue that the same is true of superuser_reserved_connections,
but there's a couple reasons why it isn't:

1) if superusers are actually connected, that shows up in
pg_stat_activity (and given how many of our users run their apps as
superuser, they get to max_connections out anyway).

2) the error message spells out that there may be superuser connections
available.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


-- 
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))) and 

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

2013-10-14 Thread Amit Kapila
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.

 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 

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

2013-10-14 Thread Josh Berkus
On 10/13/2013 01:38 AM, Gibheer wrote:
 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.

I think otherwise.

Changing max_wal_senders requires a restart.  As such, we currently
advise users to set the setting generously: as many replication
connections as you think you'll ever need, plus two.  If
max_wal_senders is a reservation which could cause the user to run out
of other connections sooner than expected, then the user is faced with a
new hard to set parameter: they don't want to set it too high *or* too
low.

This would result in a lot of user frustration as they try to get thier
connection configuration right and have to restart the server multiple
times.  I find few new features worth making it *harder* to configure
PostgreSQL, and reserved replication connections certainly don't qualify.

If it's worth having reserved replication connections (and I can see
some reasons to want it), then we need a new GUC for this:
reserved_walsender_connections

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


-- 
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-14 Thread Andres Freund
On 2013-10-14 10:26:25 -0700, Josh Berkus wrote:
 On 10/13/2013 01:38 AM, Gibheer wrote:
  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.
 
 I think otherwise.
 
 Changing max_wal_senders requires a restart.  As such, we currently
 advise users to set the setting generously: as many replication
 connections as you think you'll ever need, plus two.  If
 max_wal_senders is a reservation which could cause the user to run out
 of other connections sooner than expected, then the user is faced with a
 new hard to set parameter: they don't want to set it too high *or* too
 low.
 
 This would result in a lot of user frustration as they try to get thier
 connection configuration right and have to restart the server multiple
 times.  I find few new features worth making it *harder* to configure
 PostgreSQL, and reserved replication connections certainly don't qualify.
 
 If it's worth having reserved replication connections (and I can see
 some reasons to want it), then we need a new GUC for this:
 reserved_walsender_connections

Imo the complications around this prove my (way earlier) point that it'd
be much better to treat replication connections as something entirely
different to normal SQL connections. There's really not much overlap
here and while there's some philosophical point to be made about it all
being connections, from a practical POV treating them separately seems
better.
If we were designing on a green field I'd just rename max_connections to
something explicitly talking about client database connections, but
renaming it seems like it'd cause unnecessary pain.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
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-14 Thread Josh Berkus
On 10/14/2013 10:51 AM, Andres Freund wrote:
 Imo the complications around this prove my (way earlier) point that it'd
 be much better to treat replication connections as something entirely
 different to normal SQL connections. There's really not much overlap
 here and while there's some philosophical point to be made about it all
 being connections, from a practical POV treating them separately seems
 better.

Given that replication connections don't even appear in pg_stat_activity
now, I'd agree with you.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


-- 
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-14 Thread Amit Kapila
On Mon, Oct 14, 2013 at 10:56 PM, Josh Berkus j...@agliodbs.com wrote:
 On 10/13/2013 01:38 AM, Gibheer wrote:
 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.

 I think otherwise.

 Changing max_wal_senders requires a restart.  As such, we currently
 advise users to set the setting generously: as many replication
 connections as you think you'll ever need, plus two.  If
 max_wal_senders is a reservation which could cause the user to run out
 of other connections sooner than expected, then the user is faced with a
 new hard to set parameter: they don't want to set it too high *or* too
 low.

It is documented in the patch that configuration max_wal_senders is
reserved from max_connections.
So it will not cause the user to run out of other connections sooner
than expected, if both the variables are configured properly.

 This would result in a lot of user frustration as they try to get thier
 connection configuration right and have to restart the server multiple
 times.  I find few new features worth making it *harder* to configure
 PostgreSQL, and reserved replication connections certainly don't qualify.

 If it's worth having reserved replication connections (and I can see
 some reasons to want it), then we need a new GUC for this:
 reserved_walsender_connections

Having new variable also might make users life difficult, as with new
variable he needs to take care of setting 3 parameters
(max_wal_senders, reserved_walsender_connections, max_connections)
appropriately for this purpose and then choosing value for
reserved_walsender_connections will be another thing for which he has
to consult.


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


-- 
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-14 Thread Amit Kapila
On Mon, Oct 14, 2013 at 11:21 PM, Andres Freund and...@2ndquadrant.com wrote:
 On 2013-10-14 10:26:25 -0700, Josh Berkus wrote:
 On 10/13/2013 01:38 AM, Gibheer wrote:
  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.

 I think otherwise.

 Changing max_wal_senders requires a restart.  As such, we currently
 advise users to set the setting generously: as many replication
 connections as you think you'll ever need, plus two.  If
 max_wal_senders is a reservation which could cause the user to run out
 of other connections sooner than expected, then the user is faced with a
 new hard to set parameter: they don't want to set it too high *or* too
 low.

 This would result in a lot of user frustration as they try to get thier
 connection configuration right and have to restart the server multiple
 times.  I find few new features worth making it *harder* to configure
 PostgreSQL, and reserved replication connections certainly don't qualify.

 If it's worth having reserved replication connections (and I can see
 some reasons to want it), then we need a new GUC for this:
 reserved_walsender_connections

 Imo the complications around this prove my (way earlier) point that it'd
 be much better to treat replication connections as something entirely
 different to normal SQL connections. There's really not much overlap
 here and while there's some philosophical point to be made about it all
 being connections, from a practical POV treating them separately seems
 better.
 If we were designing on a green field I'd just rename max_connections to
 something explicitly talking about client database connections, but
 renaming it seems like it'd cause unnecessary pain.

If we think this way, then may be we should have max_user_connections
instead of max_connections and then max_wal_connections. But still
there are other's like pg_basebackup who needs connections and
tomorrow there can be new such entities which need connection.
Also we might need to have different infrastructure in code to make
these options available to users.
I think having different parameters to configure maximum connections
for different entities can complicate both code as well as user's job.

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


-- 
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-13 Thread Amit Kapila
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.

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.

2.
-new replication connections will be accepted.
+new WAL or other connections will be accepted.

I think as per new implementation, we don't need to change this line.

3.
+ * reserved slots from max_connections for wal senders. If the number of free
+ * slots (max_connections - max_wal_senders) is depleted.

 Above calculation (max_connections - max_wal_senders) needs to
include super user reserved connections.

4.
+ /*
+ * Although replication connections currently require superuser privileges, we
+ * don't allow them to consume the superuser reserved slots, which are
+ * intended for interactive use.
  */
  if ((!am_superuser || am_walsender) 
  ReservedBackends  0 
  !HaveNFreeProcs(ReservedBackends))
  ereport(FATAL,
  (errcode(ERRCODE_TOO_MANY_CONNECTIONS),
- errmsg(remaining connection slots are reserved for non-replication
superuser connections)));
+ errmsg(remaining connection slots are reserved for superuser connections)));

Will there be any problem if we do the above check before the check
for wal senders and reserved replication connections (+
!HaveNFreeProcs(max_wal_senders + ReservedBackends))) and don't change
the error message in this check. I think this will ensure that users
who doesn't enable max_wal_senders will see the same error message as
before and the purpose to reserve connections for replication can be
served by your second check.


5.
+ gettext_noop(Sets the maximum number wal sender connections and
reserves them.),

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

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


-- 
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-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 it is 

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 Mike Blackwell
I'd received an email from Gibheer suggesting it be move due to lack of time to 
work on it.  I can certainly move it back if that's no longer the case.





On Oct 9, 2013, at 23:25, 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


-- 
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-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-10 Thread Amit Kapila
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 again. That's
 why I told him to just move the patch into the next commitfest.

   But you have already posted patch after fixing comments and I am
planning to review again on coming weekend; if every thing is 

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-10-09 Thread Amit Kapila
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


-- 
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-08-08 Thread Robert Haas
On Mon, Aug 5, 2013 at 2:04 AM, Andres Freund and...@2ndquadrant.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'm also not sure the decision about whether something is a WAL sender
is made early enough for the distinction to really make sense.  WAL
senders actually start off, from the postmaster's POV, as regular
backends, and then become walsenders on the fly.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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-08-05 Thread Andres Freund
On 2013-08-02 08:16:15 -0400, Robert Haas 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()).

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.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
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-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] Patch for reserved connections for replication users

2013-08-02 Thread Robert Haas
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()).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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


[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