Em 31 de dezembro de 2015 04:56:55 BRST, Noah Misch <noah@leadboat.com> escreveu:
On Wed, Dec 30, 2015 at 10:18:35AM -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2015-12-29 11:07:26 -0500, Tom Lane wrote:
In passing, the patch gets rid of a vestigial CHECK_FOR_INTERRUPTS() call; it was added by e710b65c and IMO should have been removed again by 6647248e. There's certainly no very good reason to have one right at that spot anymore.
Why? Doesn't seem like the worst place for an
explicit interrupt check? I think we don't really have a problem with too many such checks... We surely could move it, but I don't really see how it's related to the topic at hand nor do I think it's really worth pondering about extensively.
Agreed.
The only reason there was one there at all was that e710b65c added code like this:
+ /* + * Disable immediate interrupts while doing database access. (Note + * we don't bother to turn this back on if we hit one of the failure + * conditions, since we can expect we'll just exit right away anyway.) + */ + ImmediateInterruptOK = false;
... some catalog access here ...
+ /* Re-enable immediate response to SIGTERM/SIGINT/timeout interrupts */ +
ImmediateInterruptOK = true; + /* And don't forget to detect one that already arrived */ + CHECK_FOR_INTERRUPTS();
In 6647248e you got rid of nine of these ten lines, leaving something that's both pointless and undocumented. There are more than enough CHECK_FOR_INTERRUPTS calls already in the auth code; there's not a reason to expend code space on one here. (If MD5 ran long enough to be worth interrupting, there would be an argument for a check inside its hashing loop, but that still wouldn't be this check.)
I see no general benefit from being parsimonious with CHECK_FOR_INTERRUPTS calls or documenting them. As you explain, it's probably fine to remove the two calls that commit e710b65 had added. However, the sole connection to $SUBJECT is one of those two calls sharing a screenful with lines $SUBJECT changed. The removal, if worthwhile, is worth a freestanding patch. Squashing the changes makes both topics harder to review.
nm
-- Sent from my Android device with K-9 Mail. Please excuse my brevity.
