On Mon, Nov 21, 2022 at 10:37 PM Robert Haas <robertmh...@gmail.com> wrote: > > On Sun, Nov 20, 2022 at 9:20 PM Kyotaro Horiguchi > <horikyota....@gmail.com> wrote: > > I prefer Robert's approach as it is more robust for future changes and > > simple. I prefer to avoid this kind of piggy-backing and it doesn't > > seem to be needed in this case. XLogShutdownWalRcv() looks like a > > similar case to me and honestly I don't like it in the sense of > > robustness but it is simpler than checking walreceiver status at every > > site that refers to the flag. > > I don't understand what you want changed. Can you be more specific > about what you mean by "Robert's approach"? > > I don't agree with Bharath's logic for preferring an if-test to an > Assert. There are some cases where we think we've written the code > correctly but also recognize that the logic is complicated enough that > something might slip through the cracks. Then, a runtime check makes > sense, because otherwise something real bad might happen on a > production instance. But here, I don't think that's the main risk. I > think the main risk is that some future patch tries to add code that > should print startup log messages later on. That would probably be a > coding mistake, and Assert would alert the patch author about that, > whereas amending the if-test would just make the code do something > differently then the author intended. > > But I don't feel super-strongly about this, which is why I mentioned > both options in my previous email. I'm not clear on whether you are > expressing an opinion on this point in particular or something more > general.
If we place just the Assert(!StandbyMode); in enable_startup_progress_timeout(), it fails for begin_startup_progress_phase() in ResetUnloggedRelations() because the InitWalRecovery(), that sets StandbyMode to true, is called before ResetUnloggedRelations() . However, with the if (StandbyMode) { return; }, we fail to report progress of ResetUnloggedRelations() in a standby, which isn't a good idea at all because we only want to disable the timeout during the recovery's main loop. I introduced an assert-only function returning true when we're in standby's main redo apply loop and modified the assertion to be Assert(!InStandbyMainRedoApplyLoop()); works better but it complicates the code a bit. FWIW, I'm attaching the v6 patch with this change. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
v6-0001-Disable-STARTUP_PROGRESS_TIMEOUT-in-standby-mode.patch
Description: Binary data