Hi, On 2020-05-16 22:51:50 -0400, Alvaro Herrera wrote: > On 2020-May-16, Andres Freund wrote: > > > I, independent of this patch, added a few additional paths in which > > checkpointer's latch is reset, and I found a few shutdowns in regression > > tests to be extremely slow / timing out. The reason for that is that > > the only check for interrupts is at the top of the loop. So if > > checkpointer gets SIGUSR2 we don't see ShutdownRequestPending until we > > decide to do a checkpoint for other reasons. > > Ah, yeah, this seems a genuine bug. > > > I also suspect that it could have harmful consequences to not do a > > AbsorbSyncRequests() if something "ate" the set latch. > > I traced through this when looking over the previous fix, and given that > checkpoint execution itself calls AbsorbSyncRequests frequently, I > don't think this one qualifies as a bug.
There's no AbsorbSyncRequests() after CheckPointBuffers(), I think. And e.g. CheckPointTwoPhase() could take a while. Which then would mean that we'd potentially not AbsorbSyncRequests() until checkpoint_timeout causes us to wake up. Am I missing something? > > One way to do that would be to WaitLatch() call to much earlier, and > > only do a WaitLatch() if do_checkpoint is false. Roughly like in the > > attached. > > Hm. I'd do "WaitLatch() / continue" in the "!do_checkpoint" block, and > put the checpkoint code not in the else block; seems easier to read to > me. Yea, that'd probably be better. I was also pondering if we shouldn't just move the checkpoint code into, gasp, it's own function ;) Greetings, Andres Freund
