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