Re: Connection slots reserved for replication

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: michael(at)paquier(dot)xyz
Cc: alexk(at)hintbits(dot)com, petr(dot)jelinek(at)2ndquadrant(dot)com, cyberdemn(at)gmail(dot)com, sfrost(at)snowman(dot)net, magnus(at)hagander(dot)net, sawada(dot)mshk(at)gmail(dot)com, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Connection slots reserved for replication
Date: 2019-02-07 09:12:37
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


At Thu, 7 Feb 2019 15:51:46 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20190207065146(dot)GN4074(at)paquier(dot)xyz>
> On Sat, Feb 02, 2019 at 10:35:20AM +0900, Michael Paquier wrote:
> > Oh, I have just noticed this patch when doing my vacuum homework. I
> > have just added my name as committer, just wait a bit until the CF is
> > closed before I begin looking at it..
> So, I have looked at this thread and the latest patch, and the thing
> looks in good shape. Nice job by the authors and the reviewers. It
> is a bit of a pain for users to hope that max_connections would be
> able to handle replication connections when needed, which can cause
> backups to not happen. Using a separate GUC while we have
> max_wal_senders here to do the job is also not a good idea, so the
> approach of the patch is sound. And on top of that, dependencies
> between GUCs get reduced.
> I have spotted one error though: the patch does not check if changing
> max_wal_senders overflows MAX_BACKEND, and we need to reflect a proper
> calculation into check_autovacuum_max_workers,
> check_max_worker_processes and check_maxconnections.

I don't think it is a good thing, including the existing checker
functions. But as you (seem to have) wrote below it can be
another issue. So I agree to that.

> One thing is that the slot position calculation gets a bit more
> complicated with the new slot set for WAL senders, still the code is
> straight-forward to follow so that's not really an issue in my

I was hesitating to propose to change it (in InitProcGlobal) but
I like the fixed style.

> opinion. The potential backward-incompatible issue issue of creating
> a standby with lower max_wal_senders value than the primary is also
> something we can live with as that's simple enough to address, and I'd
> like to think that most users prefer inheriting the parameter from the
> primary to ease failover flows.

I belive so.

> It seems to me that it would be a good idea to have an
> autovacuum-worker-related message in the context of InitProcess() for
> a failed backend creation, but we can deal with that later if
> necessary.

(Maybe I'm losing the point, but) The guc validate functions for
max_connections and friends seem rather harmful than useless,
since we only see an error for max_connections and others won't
be seen, and it conceals what is happening. I think we should
remove the validators and InitializeMaxBackends should complain
on that providing more meaningful information. In another patch?

> With all that reviewed, I finish with the attached that I am
> comfortable enough to commit. XLOG_PAGE_MAGIC is bumped as well, as a
> reminder because xl_parameter_change gets an upgrade, and I am most
> likely going to forget it.

Some removed comments are revived but I'm fine with it. Adding
tags in documentation seems fine.

> Please let me know if you have comments. I am still planning to do an
> extra pass on it.

After all I (am not the author) am fine with it.


Kyotaro Horiguchi
NTT Open Source Software Center

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Ideriha, Takeshi 2019-02-07 09:40:12 RE: Protect syscache from bloating with negative cache entries
Previous Message John Naylor 2019-02-07 09:09:08 use Getopt::Long for catalog scripts