|From:||Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>|
|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
(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.
NTT Open Source Software Center
|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|