Thread: Make SIGHUP less painful if pg_hba.conf is not readable
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
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index a134b45..931ca86 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1307,11 +1307,14 @@ load_hba(void)
file = AllocateFile(HbaFileName, "r");
/* Failure is fatal since with no HBA entries we can do nothing... */
- if (file == NULL)
- ereport(FATAL,
+ if (file == NULL)
+ {
+ ereport(WARNING,
(errcode_for_file_access(),
errmsg("could not open configuration file \"%s\": %m",
HbaFileName)));
+ return false;
+ }
tokenize_file(HbaFileName, file, &hba_lines, &hba_line_nums);
FreeFile(file);
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.
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
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
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
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
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
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
Attachment
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
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?
regards, tom lane
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
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).
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'mthinking we should probably just not bother to backpatchit. //Magnus
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