Re: Better detail logging for password auth failures

On Wed, Dec 30, 2015 at 10:18:35AM -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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