Hello. At Thu, 7 Feb 2019 15:51:46 +0900, Michael Paquier <mich...@paquier.xyz> wrote in <20190207065146.gn4...@paquier.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. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
