Re: Connection slots reserved for replication

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Oleksii Kliukin <alexk(at)hintbits(dot)com>
Cc: Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Alexander Kukushkin <cyberdemn(at)gmail(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Magnus Hagander <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 06:51:46
Message-ID: 20190207065146.GN4074@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

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

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.

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.

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

Attachment Content-Type Size
replication_reserved_connections_v10.patch text/x-diff 22.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2019-02-07 07:52:34 Re: Allow some recovery parameters to be changed with reload
Previous Message Dean Rasheed 2019-02-07 06:41:03 Re: [HACKERS] PATCH: multivariate histograms and MCV lists