This is my first patch. I hope it's not stupid.

We ran into a little issue today where permission/ownership on
pg_hba.conf was accidentally changed to something that the postgres user
could not read. When a SIGHUP was issued, the postmaster quit. That was
kind of a bummer.

From the comment in hba.c, it appears that the desired behavior is to
have the system ignore the failure, and continue using what's already
loaded into memory. And, turns out, that's what I would like Postgres
to do as well.

So, this patch changes the error issued from load_hba() from FATAL to
WARNING if the file is not found, and returns.

Startup behavior (FATAL if pg_hba.conf can't be found) is not changed.

Tested against 8.4devel HEAD today.

Patch attached.

--
Selena Deckelmann
End Point Corporation
selena@endpoint.com

Search Discussions

  • Tom Lane at Mar 4, 2009 at 3:15 am

    Selena Deckelmann writes:
    From the comment in hba.c, it appears that the desired behavior is to
    have the system ignore the failure,
    I'm not sure how you could possibly read that comment that way.

    It might be sane to distinguish initial load from reload, but I think
    the behavior is correct as-is for initial load.

    Also, if we are going to do something like this, we should make sure
    it's consistent across all the config files.

    regards, tom lane
  • Selena Deckelmann at Mar 4, 2009 at 3:54 am

    Tom Lane wrote:
    Selena Deckelmann <selena@endpoint.com> writes:
    From the comment in hba.c, it appears that the desired behavior is to
    have the system ignore the failure,
    I'm not sure how you could possibly read that comment that way.
    Right. Sorry, poor choice of words. I meant "don't die on reload",
    essentially.
    It might be sane to distinguish initial load from reload, but I think
    the behavior is correct as-is for initial load. Agreed.
    Also, if we are going to do something like this, we should make sure
    it's consistent across all the config files.
    Ok. I can do that. I'll check with some other people before I send
    another patch, and I'll go through the rest of the config file loads
    tomorrow.

    -selena


    --
    Selena Deckelmann
    End Point Corporation
    selena@endpoint.com
  • Magnus Hagander at Mar 4, 2009 at 8:44 am

    Selena Deckelmann wrote:
    Tom Lane wrote:
    Selena Deckelmann <selena@endpoint.com> writes:
    From the comment in hba.c, it appears that the desired behavior is to
    have the system ignore the failure,
    I'm not sure how you could possibly read that comment that way.
    Right. Sorry, poor choice of words. I meant "don't die on reload",
    essentially.
    The comment is wrong, the patch is correct.

    It might be sane to distinguish initial load from reload, but I think
    the behavior is correct as-is for initial load.
    Agreed.
    The patch solves this perfectly fine. The caller takes care of sending a
    FATAL error if it's in startup mode.

    Also, if we are going to do something like this, we should make sure
    it's consistent across all the config files.
    Ok. I can do that. I'll check with some other people before I send
    another patch, and I'll go through the rest of the config file loads
    tomorrow.
    From what I can tell, it is already consistent (once this fix is
    applied). Permissions wrong on postgresql.conf already sends a warning
    and not a FATAL. Same for pg_ident.conf.


    So. I've updated the comment, and applied your patch. Thanks!

    (You also added a trailing space on the if line - might want to check if
    you can get your editor to warn about that. Or "git diff" will..)

    //Magnus
  • Joshua Tolley at Mar 4, 2009 at 9:14 am

    On Wed, Mar 04, 2009 at 09:43:55AM +0100, Magnus Hagander wrote:
    So. I've updated the comment, and applied your patch. Thanks!
    What would it take to get it applied to a few earlier versions as well?

    - Josh
  • Magnus Hagander at Mar 4, 2009 at 9:28 am

    Joshua Tolley wrote:
    On Wed, Mar 04, 2009 at 09:43:55AM +0100, Magnus Hagander wrote:
    So. I've updated the comment, and applied your patch. Thanks!
    What would it take to get it applied to a few earlier versions as well?
    I guess you maintaining your own fork? ;-)


    Simply put, earlier versions threw away the contents of pg_hba and
    reloaded it completely. The support for keeping the old one around in
    case of syntax errors is new for 8.4. You'd basically require
    backpatching of large parts of that patch, and that's not going to happen.

    //Magnus
  • Joshua Tolley at Mar 4, 2009 at 2:20 pm

    On Wed, Mar 04, 2009 at 10:28:42AM +0100, Magnus Hagander wrote:
    Joshua Tolley wrote:
    On Wed, Mar 04, 2009 at 09:43:55AM +0100, Magnus Hagander wrote:
    So. I've updated the comment, and applied your patch. Thanks!
    What would it take to get it applied to a few earlier versions as well?
    I guess you maintaining your own fork? ;-)


    Simply put, earlier versions threw away the contents of pg_hba and
    reloaded it completely. The support for keeping the old one around in
    case of syntax errors is new for 8.4. You'd basically require
    backpatching of large parts of that patch, and that's not going to happen.

    //Magnus
    Given that we ran into the problem in 8.3.6, how about something like
    the attached to apply to it?

    - Josh / eggyknap
  • Magnus Hagander at Mar 5, 2009 at 12:57 pm

    Joshua Tolley wrote:
    On Wed, Mar 04, 2009 at 10:28:42AM +0100, Magnus Hagander wrote:
    Joshua Tolley wrote:
    On Wed, Mar 04, 2009 at 09:43:55AM +0100, Magnus Hagander wrote:
    So. I've updated the comment, and applied your patch. Thanks!
    What would it take to get it applied to a few earlier versions as well?
    I guess you maintaining your own fork? ;-)


    Simply put, earlier versions threw away the contents of pg_hba and
    reloaded it completely. The support for keeping the old one around in
    case of syntax errors is new for 8.4. You'd basically require
    backpatching of large parts of that patch, and that's not going to happen.

    //Magnus
    Given that we ran into the problem in 8.3.6, how about something like
    the attached to apply to it?
    Yeah, the big question is if we want to backport something like this at
    all... Thoughts?

    //Magnus
  • Tom Lane at Mar 5, 2009 at 2:48 pm

    Magnus Hagander writes:
    Yeah, the big question is if we want to backport something like this at
    all... Thoughts?
    The issue never even came up before, so I'd vote to not take any risks
    for it. How often do people mess up the protections on pg_hba.conf?

    regards, tom lane
  • Joshua Tolley at Mar 5, 2009 at 6:15 pm

    On Thu, Mar 05, 2009 at 09:47:55AM -0500, Tom Lane wrote:
    Magnus Hagander <magnus@hagander.net> writes:
    Yeah, the big question is if we want to backport something like this at
    all... Thoughts?
    The issue never even came up before, so I'd vote to not take any risks
    for it. How often do people mess up the protections on pg_hba.conf?
    Apparently I do :) Whether I'm the only one or not, I can't say. I
    realize this wouldn't protect anyone from, say, syntax errors, which
    certainly are more common.

    As an aside, is access() adequately portable, ok to use within the
    backend, etc.? I just sort of took a shot in the dark.

    - Josh / eggyknap
  • Peter Eisentraut at Mar 5, 2009 at 7:16 pm

    On Thursday 05 March 2009 18:04:42 Joshua Tolley wrote:
    As an aside, is access() adequately portable, ok to use within the
    backend, etc.? I just sort of took a shot in the dark.
    Using access() is usually not a good idea. In this case it would be better to
    check the return of the actual open() call for EPERM (or the equivalent for
    fopen(), whatever is used).
  • Magnus Hagander at Mar 5, 2009 at 7:19 pm

    Peter Eisentraut wrote:
    On Thursday 05 March 2009 18:04:42 Joshua Tolley wrote:
    As an aside, is access() adequately portable, ok to use within the
    backend, etc.? I just sort of took a shot in the dark.
    Using access() is usually not a good idea. In this case it would be better to
    check the return of the actual open() call for EPERM (or the equivalent for
    fopen(), whatever is used).
    That's what we do in the proper fix in HEAD. It requires an API change
    to backport it...

    Given that I think this is the first time we've heard of this issue, I'm
    thinking we should probably just not bother to backpatch it.

    //Magnus
  • Joshua Tolley at Mar 5, 2009 at 9:43 pm

    On Thu, Mar 05, 2009 at 08:19:05PM +0100, Magnus Hagander wrote:
    Peter Eisentraut wrote:
    On Thursday 05 March 2009 18:04:42 Joshua Tolley wrote:
    As an aside, is access() adequately portable, ok to use within the
    backend, etc.? I just sort of took a shot in the dark.
    Using access() is usually not a good idea. In this case it would be better to
    check the return of the actual open() call for EPERM (or the equivalent for
    fopen(), whatever is used).
    That's what we do in the proper fix in HEAD. It requires an API change
    to backport it...

    Given that I think this is the first time we've heard of this issue, I'm
    thinking we should probably just not bother to backpatch it.
    I'm inclined to agree, FWIW.

    - Josh / eggyknap

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedMar 4, '09 at 2:26a
activeMar 5, '09 at 9:43p
posts13
users5
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase