Re: Better detail logging for password auth failures

Em 31 de dezembro de 2015 04:56:55 BRST, Noah Misch <noah(at)leadboat(dot)com> escreveu:
>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
>
>
>--
>Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>To make changes to your subscription:
>http://www.postgresql.org/mailpref/pgsql-hackers

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.