Andres Freund <and...@2ndquadrant.com> writes: > On 2013-12-13 11:26:44 -0500, Tom Lane wrote: >> On closer inspection, I'm thinking that actually it'd be a good idea if >> handle_sig_alarm did what we do in, for example, HandleCatchupInterrupt: >> it should save, clear, and restore ImmediateInterruptOK, so as to make >> the world safe for timeout handlers to do things that might include a >> CHECK_FOR_INTERRUPTS.
> Shouldn't the HOLD_INTERRUPTS() in handle_sig_alarm() prevent any > eventual ProcessInterrupts() in the timeout handlers from doing anything > harmful? Sorry, I misspoke there. The case I'm worried about is doing something like a wait for lock, which would unconditionally set and then reset ImmediateInterruptOK. That's not very plausible perhaps, but on the other hand we are calling DeadLockCheck() in there, and who knows what future timeout handlers might try to do? BTW, I'm about to go put a HOLD_INTERRUPTS/RESUME_INTERRUPTS into HandleCatchupInterrupt and HandleNotifyInterrupt too, for essentially the same reason. At least the first of these *does* include semaphore ops, so I think it's theoretically vulnerable to losing control if a timeout occurs while it's waiting for a semaphore. There's probably no real bug today because I don't think we enable catchup interrupts at any point where a timeout would be active, but that doesn't sound terribly future-proof. If a timeout did happen, holding off interrupts would have the effect of postponing the query cancel till we're done with the catchup interrupt, which seems reasonable. > One thing I randomly noticed just now is the following in > RecoveryConflictInterrupt(): > elog(FATAL, "unrecognized conflict mode: %d", > (int) reason); > obviously that's not really ever going to hit, but it should either be a > PANIC or an Assert() for the reasons you cite. Yeah, PANIC there seems good. I also thought about using START_CRIT_SECTION/END_CRIT_SECTION instead of HOLD_INTERRUPTS/RESUME_INTERRUPTS in these signal handlers. That would both hold off interrupts and cause any elog(ERROR/FATAL) within the handler to be promoted to PANIC. But I'm not sure that'd be a net stability improvement... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers