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 okay, then there is a chance that you can get committer level feedback for your patch in this CF. In the end it's upto you to decide. 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