On Thu, Jan 14, 2021 at 10:21 AM Robert Haas <robertmh...@gmail.com> wrote: > Yeah, I think this lock is useless. In fact, I think it's harmful, > because it makes large sections of the checkpointer code, basically > all of it really, run with interrupts held off for no reason. > > It's not impossible that removing the lock could reveal bugs > elsewhere: suppose we have segments of code that actually aren't safe > to interrupt, but because of this LWLock, it never happens. But, if > that happens to be true, I think we should just view those cases as > bugs to be fixed. > > One thing that blunts the impact of this quite a bit is that the > checkpointer doesn't use rely much on CHECK_FOR_INTERRUPTS() in the > first place. It has its own machinery: HandleCheckpointerInterrupts(). > Possibly that's something we should be looking to refactor somehow as > well.
Here's a patch to remove CheckpointLock completely. For ProcessInterrupts() to do anything, one of the following things would have to be true: 1. ProcDiePending = true. Normally this is set by die(), but the checkpointer does not use die(). Perhaps someday it should, or maybe not, but that's an issue for another day. 2. ClientConnectionLost = true. This gets set in pqcomm.c's internal_flush(), but the checkpointer has no client connection. 3. DoingCommandRead = true. Can't happen; only set in PostgresMain(). 4. QueryCancelPending = true. Can be set by recovery conflicts, but those shouldn't happen in the checkpointer. Normally set by StatementCancelHandler, which the checkpointer doesn't use. 5. IdleInTransactionSessionTimeoutPending = true. Set up by InitPostgres(), which is not used by auxiliary processes. 6. ProcSignalBarrierPending = true. This is the case we actually care about allowing for the ASRO patch, so if this occurs, it's good. 7. ParallelMessagePending = true. The checkpointer definitely never starts parallel workers. So I don't see any problem with just ripping this out entirely, but I'd like to know if anybody else does. -- Robert Haas EDB: http://www.enterprisedb.com
v1-0001-Remove-CheckpointLock.patch
Description: Binary data
