D'Arcy J.M. Cain wrote:
On Fri, 12 Sep 2008 06:53:55 +1000
Brendan Jurd [EMAIL PROTECTED] wrote:
Josh has assigned your patch to me for an initial review.
And me.
Thanks for the reviews, guys. I've adjusted the patch per your comments
(I think), and applied it.
//Magnus
--
Sent via
On Mon, Sep 15, 2008 at 10:53 PM, Magnus Hagander [EMAIL PROTECTED] wrote:
Thanks for the reviews, guys. I've adjusted the patch per your comments
(I think), and applied it.
Cool. I had a look at the commitdiff and I think you missed one of
the error messages (it's still all on one line).
Brendan Jurd [EMAIL PROTECTED] writes:
Cool. I had a look at the commitdiff and I think you missed one of
the error messages (it's still all on one line). It's at
src/backend/libpq/hba.c line 841. I've included a patch to split it
up below.
Hm, that doesn't seem like a great improvement
Tom Lane wrote:
Brendan Jurd [EMAIL PROTECTED] writes:
Cool. I had a look at the commitdiff and I think you missed one of
the error messages (it's still all on one line). It's at
src/backend/libpq/hba.c line 841. I've included a patch to split it
up below.
Hm, that doesn't seem like a
On Mon, Sep 15, 2008 at 11:46 PM, Magnus Hagander [EMAIL PROTECTED] wrote:
Tom Lane wrote:
Hm, that doesn't seem like a great improvement --- in particular, it
violates the style guideline that detail messages should be complete
sentences.
I think the error text is all right as-is, really.
Brendan Jurd [EMAIL PROTECTED] writes:
Or perhaps the DETAIL portion would be more appropriate as a CONTEXT?
+1
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
Tom Lane wrote:
Brendan Jurd [EMAIL PROTECTED] writes:
Or perhaps the DETAIL portion would be more appropriate as a CONTEXT?
+1
Is this the proper syntax for errcontext() messags?
//Magnus
Index: hba.c
===
RCS file:
Magnus Hagander [EMAIL PROTECTED] writes:
Or perhaps the DETAIL portion would be more appropriate as a CONTEXT?
+1
Is this the proper syntax for errcontext() messags?
Hmm, I think I'd suggest following the wording already in use in
ts_locale.c:
errcontext(line %d of
Tom Lane wrote:
Magnus Hagander [EMAIL PROTECTED] writes:
Or perhaps the DETAIL portion would be more appropriate as a CONTEXT?
+1
Is this the proper syntax for errcontext() messags?
Hmm, I think I'd suggest following the wording already in use in
ts_locale.c:
Brendan Jurd wrote:
Hi Magnus,
Josh has assigned your patch to me for an initial review.
First up I'd like to say that this is a really nice upgrade.
Shielding a running server from reloading a bogus conf file makes a
whole lot of sense.
Hi!
Thanks for your review.
The patch doesn't
On Fri, 12 Sep 2008 06:53:55 +1000
Brendan Jurd [EMAIL PROTECTED] wrote:
Josh has assigned your patch to me for an initial review.
And me.
First up I'd like to say that this is a really nice upgrade.
Shielding a running server from reloading a bogus conf file makes a
whole lot of sense.
Hi Magnus,
Josh has assigned your patch to me for an initial review.
First up I'd like to say that this is a really nice upgrade.
Shielding a running server from reloading a bogus conf file makes a
whole lot of sense.
On Sun, Aug 17, 2008 at 1:47 AM, Magnus Hagander [EMAIL PROTECTED] wrote:
Bruce Momjian wrote:
Magnus Hagander wrote:
To address Magnus' specific question, right now we store the pg_hba.conf
tokens as strings in the postmaster. I am fine with storing them in a
more native format and throwing errors for values that don't convert.
What would concern me is calling
Magnus Hagander [EMAIL PROTECTED] writes:
Other than that, it moves code around to do the parsing in the
postmaster and the maching in the backend.
How does that work in EXEC_BACKEND environments?
regards, tom lane
--
Sent via pgsql-hackers mailing list
Tom Lane wrote:
Magnus Hagander [EMAIL PROTECTED] writes:
Other than that, it moves code around to do the parsing in the
postmaster and the maching in the backend.
How does that work in EXEC_BACKEND environments?
(Not tested yet, still on my TODO, but still)
It will parse the file in the
Magnus Hagander wrote:
Magnus Hagander wrote:
[about the ability to use different maps for ident auth, gss and krb
auth for example]
It wouldn't be very easy/clean to do that w/o breaking the existing
structure of pg_ident though, which makes me feel like using seperate
files is
Bruce Momjian wrote:
Magnus Hagander wrote:
Magnus Hagander wrote:
[about the ability to use different maps for ident auth, gss and krb
auth for example]
It wouldn't be very easy/clean to do that w/o breaking the existing
structure of pg_ident though, which makes me feel like using
Magnus Hagander wrote:
To address Magnus' specific question, right now we store the pg_hba.conf
tokens as strings in the postmaster. I am fine with storing them in a
more native format and throwing errors for values that don't convert.
What would concern me is calling lots of 3rd party
Magnus Hagander wrote:
[about the ability to use different maps for ident auth, gss and krb
auth for example]
It wouldn't be very easy/clean to do that w/o breaking the existing
structure of pg_ident though, which makes me feel like using seperate
files is probably the way to go.
Actually, I
Hello,
On Sat, 02 Aug 2008 18:37:25 +0200 Magnus Hagander wrote:
Tom Lane wrote:
Magnus Hagander [EMAIL PROTECTED] writes:
We could catch some simple problems at file load time, perhaps,
but those usually aren't the ones that cause trouble for people.
It would catch things like
Stephen Frost wrote:
Magnus,
* Magnus Hagander ([EMAIL PROTECTED]) wrote:
Yeah. I think the question there is just - how likely is it that the
same installation actually uses 1 authentication method. Personally, I
think it's not very uncommon at all, but fact remains that as long as
you
Stephen Frost wrote:
Magnus,
* Magnus Hagander ([EMAIL PROTECTED]) wrote:
I thought of another issue with this. My grand plan includes being
able to do username mapping (per pg_ident.conf) for other authentication
methods than ident. Specifically this would be interesting for all
external
Magnus,
* Magnus Hagander ([EMAIL PROTECTED]) wrote:
Yeah. I think the question there is just - how likely is it that the
same installation actually uses 1 authentication method. Personally, I
think it's not very uncommon at all, but fact remains that as long as
you only use one of them at a
Stephen Frost wrote:
* Magnus Hagander ([EMAIL PROTECTED]) wrote:
Tom Lane wrote:
It isn't, and I seem to recall we've had that scenario play out a couple
times already for postgresql.conf changes. But pg_hba.conf is far more
complex than variable = value ...
Ok, then I didn't misunderstand
Magnus,
* Magnus Hagander ([EMAIL PROTECTED]) wrote:
I thought of another issue with this. My grand plan includes being
able to do username mapping (per pg_ident.conf) for other authentication
methods than ident. Specifically this would be interesting for all
external methods, like
Tom Lane wrote:
Magnus Hagander [EMAIL PROTECTED] writes:
Tom Lane wrote:
Seems a lot better to me to just train people to run the check-config
code by hand before pulling the trigger to load the settings for real.
I think it'd be reasonable to refuse starting if the config is *known
Magnus Hagander [EMAIL PROTECTED] writes:
Should I read this as you warming up slightly to the idea of having the
postmaster do that? ;-)
No ;-). I still think that a postgres --check-config feature would be
far more complete and useful, as well as less likely to cause bugs in
critical code
Tom Lane wrote:
Magnus Hagander [EMAIL PROTECTED] writes:
Should I read this as you warming up slightly to the idea of having the
postmaster do that? ;-)
No ;-).
Bummer. Worth a shot though :-)
I still think that a postgres --check-config feature would be
far more complete and useful,
Magnus Hagander [EMAIL PROTECTED] writes:
Tom Lane wrote:
A point that I don't think has been made so far in the thread: the
only place the postmaster could complain in event of problems is the
postmaster log, which we know too well isn't watched by inexperienced
DBAs. I guarantee you that
Tom Lane wrote:
Magnus Hagander [EMAIL PROTECTED] writes:
Tom Lane wrote:
A point that I don't think has been made so far in the thread: the
only place the postmaster could complain in event of problems is the
postmaster log, which we know too well isn't watched by inexperienced
DBAs. I
* Magnus Hagander ([EMAIL PROTECTED]) wrote:
Tom Lane wrote:
It isn't, and I seem to recall we've had that scenario play out a couple
times already for postgresql.conf changes. But pg_hba.conf is far more
complex than variable = value ...
Ok, then I didn't misunderstand that part at
On Sun, 2008-08-03 at 10:36 +0200, Magnus Hagander wrote:
Tom Lane wrote:
Magnus Hagander [EMAIL PROTECTED] writes:
The good way to solve this would be to have independant command line
utilities which check pg_hba.conf, pg_ident.conf and postgresql.conf for
errors. Then DBAs could run a
Joshua D. Drake [EMAIL PROTECTED] writes:
Alvaro Herrera wrote:
(I think it's better to reuse the same postmaster executable, because
that way it's easier to have the same parsing routines.)
Change that to pg_ctl and you have a deal :)
Did you not understand Alvaro's point? Putting this
Magnus Hagander [EMAIL PROTECTED] writes:
Tom Lane wrote:
Seems a lot better to me to just train people to run the check-config
code by hand before pulling the trigger to load the settings for real.
I think it'd be reasonable to refuse starting if the config is *known
broken* (such as
Tom Lane wrote:
Robert Treat [EMAIL PROTECTED] writes:
Certainly there isn't any reason to allow a reload of a file that is just
going to break things when the first connection happens. For that matter,
why would we ever not want to parse it at HUP time rather than connect time?
Two or
Tom Lane wrote:
Magnus Hagander [EMAIL PROTECTED] writes:
The good way to solve this would be to have independant command line
utilities which check pg_hba.conf, pg_ident.conf and postgresql.conf for
errors. Then DBAs could run a check *before* restarting the server.
While clearly useful,
Tom Lane [EMAIL PROTECTED] writes:
Robert Treat [EMAIL PROTECTED] writes:
Certainly there isn't any reason to allow a reload of a file that is just
going to break things when the first connection happens. For that matter,
why would we ever not want to parse it at HUP time rather than
* Magnus Hagander ([EMAIL PROTECTED]) wrote:
For pg_hba.conf, I don't see that as a very big problem, really. It
doesn't (and shouldn't) modify any external variables, so it should be
as simple as parsing the new file into a completely separate
list-of-structs and only if it's all correct
Stephen Frost wrote:
* Magnus Hagander ([EMAIL PROTECTED]) wrote:
For pg_hba.conf, I don't see that as a very big problem, really. It
doesn't (and shouldn't) modify any external variables, so it should be
as simple as parsing the new file into a completely separate
list-of-structs and only if
* Magnus Hagander ([EMAIL PROTECTED]) wrote:
I think it'd be reasonable to refuse starting if the config is *known
broken* (such as containing lines that are unparseable, or that contain
completely invalid tokens), whereas you'd start if they just contain
things that are probably wrong. But
* Magnus Hagander ([EMAIL PROTECTED]) wrote:
Stephen Frost wrote:
A little extra code in the backend is well worth fixing this foot-gun.
The concerns raised so far have been who will write it? and what if
it has bugs?. Neither of these are particularly compelling arguments
when you've
The way pg_hba.conf is set up to be loaded/parsed now, we have:
postmaster: open and tokenize file (load_hba(), tokenize_file()).
backend: parse lines (parse_hba) and check for matches
(check_hba/hba_getauthmethod)
This means that the code in the postmaster is very simple, and it's
shared with
Magnus Hagander [EMAIL PROTECTED] writes:
I've also noticed that authentication methods error out in different
ways when they are not supported. For example, if I try to use Kerberos
without having it compiled in, I get an error when a client tries to
connect (because we compile in stub
Magnus Hagander [EMAIL PROTECTED] writes:
Is there any actual gain by not doing the parsing in the postmaster,
Define parsing. There's quite a lot of possible errors in pg_hba
that it would be totally unreasonable for the postmaster to detect.
We could catch some simple problems at file load
Tom Lane wrote:
Idle thought: maybe what would really make sense here is a lint
for PG config files, which you'd run as a standalone program and
which would look for not only clear errors but questionable things
to warn about. For instance it might notice multiple pg_hba.conf
entries for
Magnus,
However it would be nice to throw an error or at least a warning when parsing
the file instead of pretending everything's ok. Perhaps authentication methods
should have a function to check whether the method is supported which is
called when the file is parsed.
The good way to solve
Tom Lane wrote:
Magnus Hagander [EMAIL PROTECTED] writes:
Is there any actual gain by not doing the parsing in the postmaster,
Define parsing. There's quite a lot of possible errors in pg_hba
that it would be totally unreasonable for the postmaster to detect.
Parsing as in turning into a
Josh Berkus wrote:
Magnus,
However it would be nice to throw an error or at least a warning when
parsing
the file instead of pretending everything's ok. Perhaps authentication
methods
should have a function to check whether the method is supported which is
called when the file is parsed.
Alvaro Herrera [EMAIL PROTECTED] writes:
Tom Lane wrote:
Idle thought: maybe what would really make sense here is a lint
for PG config files,
I like this idea.
postgres --check-hba-file /path/to/hba.conf
postgres --check-conf-file /path/to/postgresql.conf
(I think it's better to reuse
Magnus Hagander [EMAIL PROTECTED] writes:
The good way to solve this would be to have independant command line
utilities which check pg_hba.conf, pg_ident.conf and postgresql.conf for
errors. Then DBAs could run a check *before* restarting the server.
While clearly useful, it'd still leave
Alvaro Herrera wrote:
Tom Lane wrote:
Idle thought: maybe what would really make sense here is a lint
for PG config files, which you'd run as a standalone program and
which would look for not only clear errors but questionable things
to warn about. For instance it might notice multiple
Tom Lane wrote:
Alvaro Herrera [EMAIL PROTECTED] writes:
Tom Lane wrote:
Idle thought: maybe what would really make sense here is a lint
for PG config files,
I like this idea.
postgres --check-hba-file /path/to/hba.conf
postgres --check-conf-file /path/to/postgresql.conf
(I think it's
Tom,
Seems a lot better to me to just train people to run the check-config
code by hand before pulling the trigger to load the settings for real.
Or just have pg_ctl --check-first as an option. Cautious DBAs would use
that. In 2-3 versions, we can make --check-first the default, and
Josh Berkus wrote:
Tom,
Seems a lot better to me to just train people to run the check-config
code by hand before pulling the trigger to load the settings for real.
Or just have pg_ctl --check-first as an option. Cautious DBAs would use
that. In 2-3 versions, we can make --check-first
Joshua D. Drake wrote:
Tom Lane wrote:
I'd go for just
postgres --check-config -D $PGDATA
(In a reload scenario, you'd edit the files in-place and then do this
before issuing SIGHUP.)
Sounds good.
Doesn't it seem reasonable that it should be pg_ctl? You should never
run postgres
Alvaro Herrera wrote:
Joshua D. Drake wrote:
Tom Lane wrote:
Doesn't it seem reasonable that it should be pg_ctl? You should never
run postgres directly unless it is for DR.
What on earth is DR?
Disaster Recovery.
The problem with pg_ctl is that it's indirectly calling postgres, and
Joshua D. Drake wrote:
Alvaro Herrera wrote:
The problem with pg_ctl is that it's indirectly calling postgres, and it
doesn't have a lot of a way to know what happened after calling it;
consider the mess we have with pg_ctl -w.
True enough but perhaps that is a problem in itself. IMO, we
On Saturday 02 August 2008 13:36:40 Magnus Hagander wrote:
Josh Berkus wrote:
Tom,
Seems a lot better to me to just train people to run the check-config
code by hand before pulling the trigger to load the settings for real.
Or just have pg_ctl --check-first as an option. Cautious
Joshua D. Drake [EMAIL PROTECTED] writes:
True enough but perhaps that is a problem in itself. IMO, we should be
encouraging people to never touch the postgres binary.
I don't buy that at all. pg_ctl is useful for some people and not so
useful for others; in particular, from the perspective
Robert Treat [EMAIL PROTECTED] writes:
Certainly there isn't any reason to allow a reload of a file that is just
going to break things when the first connection happens. For that matter,
why would we ever not want to parse it at HUP time rather than connect time?
Two or three reasons why
60 matches
Mail list logo
