Thread: Server does not start when log_statement_stats is set to on
Hi, I'm getting this error when I set lot_statement_stats to on : FATAL: invalid value for parameter "log_statement_stats": 1 and server does not start. This is "almost" default configuration file (8.2.5, Fedora 8, running on my laptop): http://www.gunduz.org/tmp/postgresql.conf Per Alexey (and Alvaro), both log_planner_stats and log_statement_stats cannot be turned on at the same time. Is that documented somewhere, or can we please improve the error message here -- or is it a bug? Thoughts? Regards, -- Devrim GÜNDÜZ , RHCE PostgreSQL Replication, Consulting, Custom Development, 24x7 support Managed Services, Shared and Dedicated Hosting Co-Authors: plPHP, ODBCng - http://www.commandprompt.com/
Devrim GÜNDÜZ <devrim@CommandPrompt.com> writes:
> I'm getting this error when I set lot_statement_stats to on :
> FATAL: invalid value for parameter "log_statement_stats": 1
> Per Alexey (and Alvaro), both log_planner_stats and log_statement_stats
> cannot be turned on at the same time.
Yup:
regression=# set log_planner_stats TO 1;
SET
regression=# set log_statement_stats TO 1;
ERROR: cannot enable "log_statement_stats" when "log_parser_stats", "log_planner_stats", or "log_executor_stats" is
true
> Is that documented somewhere,
Yes.
> or can we please improve the error message here -- or is it a bug?
It's not a bug. However, the specific error message only comes out in
interactive-SET cases:
if (source >= PGC_S_INTERACTIVE) ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("cannot enable \"log_statement_stats\" when "
"\"log_parser_stats\", \"log_planner_stats\", " "or \"log_executor_stats\"
istrue")));
There are a bunch of other GUC assign hooks that behave similarly.
Perhaps it'd be sensible to emit these complaints as LOG messages
when we're dealing with a noninteractive source (ie, the config file)?
regards, tom lane
Tom Lane wrote:
> Devrim GÜNDÜZ <devrim@CommandPrompt.com> writes:
> > or can we please improve the error message here -- or is it a bug?
>
> It's not a bug. However, the specific error message only comes out in
> interactive-SET cases:
>
> if (source >= PGC_S_INTERACTIVE)
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("cannot enable \"log_statement_stats\" when "
> "\"log_parser_stats\", \"log_planner_stats\", "
> "or \"log_executor_stats\" is true")));
>
> There are a bunch of other GUC assign hooks that behave similarly.
> Perhaps it'd be sensible to emit these complaints as LOG messages
> when we're dealing with a noninteractive source (ie, the config file)?
I was wondering whether we could make this error message (and
equivalent ones) be the FATAL one that prevents the server from
starting.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@CommandPrompt.com> writes:
> Tom Lane wrote:
>> There are a bunch of other GUC assign hooks that behave similarly.
>> Perhaps it'd be sensible to emit these complaints as LOG messages
>> when we're dealing with a noninteractive source (ie, the config file)?
> I was wondering whether we could make this error message (and
> equivalent ones) be the FATAL one that prevents the server from
> starting.
There is some good reason, which I don't recall at the moment, why
assign-hooks (and most of the rest of GUC) shouldn't FATAL in the
middle of processing a config file. However a LOG seems relatively
harmless, and better than not emitting anything.
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@CommandPrompt.com> writes: > > Tom Lane wrote: > >> There are a bunch of other GUC assign hooks that behave similarly. > >> Perhaps it'd be sensible to emit these complaints as LOG messages > >> when we're dealing with a noninteractive source (ie, the config file)? > > > I was wondering whether we could make this error message (and > > equivalent ones) be the FATAL one that prevents the server from > > starting. > > There is some good reason, which I don't recall at the moment, why > assign-hooks (and most of the rest of GUC) shouldn't FATAL in the > middle of processing a config file. However a LOG seems relatively > harmless, and better than not emitting anything. I'm thinking of keeping the ERROR, which (I think) would cause postmaster to treat it as FATAL because there is no error handler set up yet (no, I haven't tried it). Perhaps you are correct in that LOG is the path of least resistance ... -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@CommandPrompt.com> writes:
> Tom Lane wrote:
>> There is some good reason, which I don't recall at the moment, why
>> assign-hooks (and most of the rest of GUC) shouldn't FATAL in the
>> middle of processing a config file.
> I'm thinking of keeping the ERROR, which (I think) would cause
> postmaster to treat it as FATAL because there is no error handler set up
> yet (no, I haven't tried it).
Yeah, it would, and that is the wrong thing.
It is definitely not acceptable to throw an ERROR unconditionally,
as that would cause postmaster abort during SIGHUP if an error is
introduced into the config file after startup. It's possible that
we could add some state by which assign-hook routines could check
whether this is initial startup or not, but then we'd still have
the issue of whether there are any other reasons not to throw ERROR
just there (I have a feeling there are some internal-to-GUC reasons
why not, as well as the obvious one that SIGHUP shouldn't be able
to crash the postmaster).
The whole idea sounds pretty shaky to me, and definitely not
something to change in late beta. LOG we could do now without
risking breaking anything.
regards, tom lane
I wrote:
> The whole idea sounds pretty shaky to me, and definitely not
> something to change in late beta. LOG we could do now without
> risking breaking anything.
Poking around a bit more, I notice that there are already some places
that do it the way I was thinking of, eg in commands/variable.c:
if (!new_tz)
{
ereport((source >= PGC_S_INTERACTIVE) ? ERROR : LOG,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("unrecognized time zone name: \"%s\"",
value)));
return NULL;
}
However, even this is not really good enough: in the event of a wrong
entry inserted into postgresql.conf, this coding will result in every
extant backend emitting the same LOG message at SIGHUP. That's
annoying. The right way to do it is as illustrated in
set_config_option(): only the postmaster should log at LOG level,
everyone else should be down around DEBUG2 (if not lower).
That's getting to be a bit complicated to replicate in N places, though.
Plus if we ever want to make it work like Alvaro is thinking of, we'd
have to go back and change all those places again. So I propose
inventing a function
int guc_complaint_level(GucSource source)
that encapsulates this logic. The correct coding for specialized
error messages in assign-hook routines would then be like
if (!new_tz)
{
ereport(guc_complaint_level(source),
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("unrecognized time zone name: \"%s\"",
value)));
return NULL;
}
giving us only one place to change to alter this logic.
Comments, objections?
regards, tom lane
Tom Lane wrote: > That's getting to be a bit complicated to replicate in N places, though. > Plus if we ever want to make it work like Alvaro is thinking of, we'd > have to go back and change all those places again. So I propose > inventing a function > > int guc_complaint_level(GucSource source) > > that encapsulates this logic. I think this makes plenty of sense. However, something that occured to me just now is that perhaps the right thing to do in the long term is to put this message in errcontext and leave the "invalid value for XXX" as the main error message. That would probably involve attaching a errcontext callback and removing the complaint_level from this message altogether, letting the outer caller deal with it. I'm not sure how would GUC work if the assign hook did not raise an ERROR in the interactive case though. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@CommandPrompt.com> writes:
> I think this makes plenty of sense. However, something that occured to
> me just now is that perhaps the right thing to do in the long term is to
> put this message in errcontext and leave the "invalid value for XXX" as
> the main error message. That would probably involve attaching a
> errcontext callback and removing the complaint_level from this message
> altogether, letting the outer caller deal with it.
errcontext wouldn't help --- by the time guc.c is ready to throw the
error, the assign hook isn't in the call stack anymore. We'd have to
invent some other special-purpose gizmo to make this work.
Might be worth doing, but again, seems too big a change for 8.3.
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@CommandPrompt.com> writes: > > I think this makes plenty of sense. However, something that occured to > > me just now is that perhaps the right thing to do in the long term is to > > put this message in errcontext and leave the "invalid value for XXX" as > > the main error message. That would probably involve attaching a > > errcontext callback and removing the complaint_level from this message > > altogether, letting the outer caller deal with it. > > errcontext wouldn't help --- by the time guc.c is ready to throw the > error, the assign hook isn't in the call stack anymore. We'd have to > invent some other special-purpose gizmo to make this work. > > Might be worth doing, but again, seems too big a change for 8.3. Right -- your proposed fix seems fine for 8.3. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
I am discarding this thread from the patches queue. We have fixed some
of it so a LOG message is issued for invalid postgresql.conf
combinations. We could do more, but there doesn't seem to be a clear
TODO here.
---------------------------------------------------------------------------
Tom Lane wrote:
> I wrote:
> > The whole idea sounds pretty shaky to me, and definitely not
> > something to change in late beta. LOG we could do now without
> > risking breaking anything.
>
> Poking around a bit more, I notice that there are already some places
> that do it the way I was thinking of, eg in commands/variable.c:
>
> if (!new_tz)
> {
> ereport((source >= PGC_S_INTERACTIVE) ? ERROR : LOG,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("unrecognized time zone name: \"%s\"",
> value)));
> return NULL;
> }
>
> However, even this is not really good enough: in the event of a wrong
> entry inserted into postgresql.conf, this coding will result in every
> extant backend emitting the same LOG message at SIGHUP. That's
> annoying. The right way to do it is as illustrated in
> set_config_option(): only the postmaster should log at LOG level,
> everyone else should be down around DEBUG2 (if not lower).
>
> That's getting to be a bit complicated to replicate in N places, though.
> Plus if we ever want to make it work like Alvaro is thinking of, we'd
> have to go back and change all those places again. So I propose
> inventing a function
>
> int guc_complaint_level(GucSource source)
>
> that encapsulates this logic. The correct coding for specialized
> error messages in assign-hook routines would then be like
>
> if (!new_tz)
> {
> ereport(guc_complaint_level(source),
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("unrecognized time zone name: \"%s\"",
> value)));
> return NULL;
> }
>
> giving us only one place to change to alter this logic.
>
> Comments, objections?
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > > I am discarding this thread from the patches queue. We have fixed some > of it so a LOG message is issued for invalid postgresql.conf > combinations. We could do more, but there doesn't seem to be a clear > TODO here. Actually there's a clear TODO in GUC_complaint_elevel's header comment: /* * GUC_complaint_elevel * Get the ereport error level to use in an assign_hook's error report. * * This should be used by assign hooks that want to emit a custom error * report (in addition to the generic "invalid value for option FOO" that * guc.c will provide). Note that the result might be ERROR or a lower * level, so the caller must be prepared for control to return from ereport, * or not. If control does return, return false/NULL from the hook function. * * At some point it'd be nice to replace this with a mechanism that allows * the custom message to become the DETAIL line of guc.c's generic message. */ -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support