On Tue, May 26, 2020 at 07:30:54PM +0000, Bossart, Nathan wrote: > While an assertion in UpdateControlFile() would not have helped us > catch the problem I initially reported, it does seem worthwhile to add > it. I have attached a patch that adds this assertion and also > attempts to fix XLogReportParameters(). Since there is only one place > where we feel it is safe to call UpdateControlFile() without a lock, I > just changed it to take the lock. I don't think this adds any sort of > significant contention risk, and IMO it is a bit cleaner than the > boolean flag.
Let's see what Fujii-san and Thomas think about that. I'd rather avoid taking a lock here because we don't need it and because it makes things IMO confusing with the beginning of StartupXLOG() where a lot of the fields are read, even if we go without this extra assertion. > For the XLogReportParameters() fix, I simply added an exclusive lock > acquisition for the portion that updates the values in shared memory > and calls UpdateControlFile(). IIUC the first part of this function > that accesses several ControlFile values should be safe, as none of > them can be updated after server start. They can get updated when replaying a XLOG_PARAMETER_CHANGE record. But you are right as all of this happens in the startup process, so your patch looks right to me here. -- Michael
signature.asc
Description: PGP signature
