Skip to content

Commit 5233dc1

Browse files
tglsfdctglsfdc
committed
Improve consistency of error reporting in GUC assign_hook routines. Some
were reporting ERROR for interactive assignments and LOG for other cases, some were saying nothing for non-interactive cases, and a few did yet other things. Make them use a new function GUC_complaint_elevel() to establish a reasonably uniform policy about how to report. There are still a few edge cases such as assign_search_path(), but it's much better than before. Per gripe from Devrim Gunduz and subsequent discussion. As noted by Alvaro, it'd be better to fold these custom messages into the standard "invalid parameter value" complaint from guc.c, perhaps as the DETAIL field. However that will require more redesign than seems prudent for 8.3. This is a relatively safe, low-impact change that we can afford to risk now.
1 parent 2e4cb70 commit 5233dc1

File tree

6 files changed

+137
-110
lines changed

6 files changed

+137
-110
lines changed

‎src/backend/commands/tablespace.c

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
*
3838
*
3939
* IDENTIFICATION
40-
* $PostgreSQL: pgsql/src/backend/commands/tablespace.c,v 1.51 2007/11/15 21:14:34 momjian Exp $
40+
* $PostgreSQL: pgsql/src/backend/commands/tablespace.c,v 1.52 2007/12/28 00:23:23 tgl Exp $
4141
*
4242
*-------------------------------------------------------------------------
4343
*/
@@ -923,11 +923,10 @@ assign_default_tablespace(const char *newval, bool doit, GucSource source)
923923
if (newval[0] != '\0' &&
924924
!OidIsValid(get_tablespace_oid(newval)))
925925
{
926-
if (source >= PGC_S_INTERACTIVE)
927-
ereport(ERROR,
928-
(errcode(ERRCODE_UNDEFINED_OBJECT),
929-
errmsg("tablespace \"%s\" does not exist",
930-
newval)));
926+
ereport(GUC_complaint_elevel(source),
927+
(errcode(ERRCODE_UNDEFINED_OBJECT),
928+
errmsg("tablespace \"%s\" does not exist",
929+
newval)));
931930
return NULL;
932931
}
933932
}

‎src/backend/commands/variable.c

Lines changed: 46 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
*
1010
*
1111
* IDENTIFICATION
12-
* $PostgreSQL: pgsql/src/backend/commands/variable.c,v 1.122 2007/11/15 21:14:34 momjian Exp $
12+
* $PostgreSQL: pgsql/src/backend/commands/variable.c,v 1.123 2007/12/28 00:23:23 tgl Exp $
1313
*
1414
*-------------------------------------------------------------------------
1515
*/
@@ -57,9 +57,8 @@ assign_datestyle(const char *value, bool doit, GucSource source)
5757
/* syntax error in list */
5858
pfree(rawstring);
5959
list_free(elemlist);
60-
if (source >= PGC_S_INTERACTIVE)
61-
ereport(ERROR,
62-
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
60+
ereport(GUC_complaint_elevel(source),
61+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
6362
errmsg("invalid list syntax for parameter \"datestyle\"")));
6463
return NULL;
6564
}
@@ -157,11 +156,10 @@ assign_datestyle(const char *value, bool doit, GucSource source)
157156
}
158157
else
159158
{
160-
if (source >= PGC_S_INTERACTIVE)
161-
ereport(ERROR,
162-
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
163-
errmsg("unrecognized \"datestyle\" key word: \"%s\"",
164-
tok)));
159+
ereport(GUC_complaint_elevel(source),
160+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
161+
errmsg("unrecognized \"datestyle\" key word: \"%s\"",
162+
tok)));
165163
ok = false;
166164
break;
167165
}
@@ -172,10 +170,9 @@ assign_datestyle(const char *value, bool doit, GucSource source)
172170

173171
if (!ok)
174172
{
175-
if (source >= PGC_S_INTERACTIVE)
176-
ereport(ERROR,
177-
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
178-
errmsg("conflicting \"datestyle\" specifications")));
173+
ereport(GUC_complaint_elevel(source),
174+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
175+
errmsg("conflicting \"datestyle\" specifications")));
179176
return NULL;
180177
}
181178

@@ -271,9 +268,9 @@ assign_timezone(const char *value, bool doit, GucSource source)
271268

272269
/*
273270
* Try to parse it. XXX an invalid interval format will result in
274-
* ereport, which is not desirable for GUC. We did what we could to
275-
* guard against this in flatten_set_variable_args, but a string
276-
* coming in from postgresql.conf might contain anything.
271+
* ereport(ERROR), which is not desirable for GUC. We did what we
272+
* could to guard against this in flatten_set_variable_args, but a
273+
* string coming in from postgresql.conf might contain anything.
277274
*/
278275
interval = DatumGetIntervalP(DirectFunctionCall3(interval_in,
279276
CStringGetDatum(val),
@@ -283,19 +280,17 @@ assign_timezone(const char *value, bool doit, GucSource source)
283280
pfree(val);
284281
if (interval->month != 0)
285282
{
286-
if (source >= PGC_S_INTERACTIVE)
287-
ereport(ERROR,
288-
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
289-
errmsg("invalid interval value for time zone: month not allowed")));
283+
ereport(GUC_complaint_elevel(source),
284+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
285+
errmsg("invalid interval value for time zone: month not allowed")));
290286
pfree(interval);
291287
return NULL;
292288
}
293289
if (interval->day != 0)
294290
{
295-
if (source >= PGC_S_INTERACTIVE)
296-
ereport(ERROR,
297-
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
298-
errmsg("invalid interval value for time zone: day not allowed")));
291+
ereport(GUC_complaint_elevel(source),
292+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
293+
errmsg("invalid interval value for time zone: day not allowed")));
299294
pfree(interval);
300295
return NULL;
301296
}
@@ -361,7 +356,7 @@ assign_timezone(const char *value, bool doit, GucSource source)
361356

362357
if (!new_tz)
363358
{
364-
ereport((source >= PGC_S_INTERACTIVE) ? ERROR : LOG,
359+
ereport(GUC_complaint_elevel(source),
365360
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
366361
errmsg("unrecognized time zone name: \"%s\"",
367362
value)));
@@ -370,7 +365,7 @@ assign_timezone(const char *value, bool doit, GucSource source)
370365

371366
if (!tz_acceptable(new_tz))
372367
{
373-
ereport((source >= PGC_S_INTERACTIVE) ? ERROR : LOG,
368+
ereport(GUC_complaint_elevel(source),
374369
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
375370
errmsg("time zone \"%s\" appears to use leap seconds",
376371
value),
@@ -493,7 +488,7 @@ assign_log_timezone(const char *value, bool doit, GucSource source)
493488

494489
if (!new_tz)
495490
{
496-
ereport((source >= PGC_S_INTERACTIVE) ? ERROR : LOG,
491+
ereport(GUC_complaint_elevel(source),
497492
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
498493
errmsg("unrecognized time zone name: \"%s\"",
499494
value)));
@@ -502,7 +497,7 @@ assign_log_timezone(const char *value, bool doit, GucSource source)
502497

503498
if (!tz_acceptable(new_tz))
504499
{
505-
ereport((source >= PGC_S_INTERACTIVE) ? ERROR : LOG,
500+
ereport(GUC_complaint_elevel(source),
506501
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
507502
errmsg("time zone \"%s\" appears to use leap seconds",
508503
value),
@@ -557,22 +552,20 @@ assign_XactIsoLevel(const char *value, bool doit, GucSource source)
557552
{
558553
if (SerializableSnapshot != NULL)
559554
{
560-
if (source >= PGC_S_INTERACTIVE)
561-
ereport(ERROR,
562-
(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
563-
errmsg("SET TRANSACTION ISOLATION LEVEL must be called before any query")));
555+
ereport(GUC_complaint_elevel(source),
556+
(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
557+
errmsg("SET TRANSACTION ISOLATION LEVEL must be called before any query")));
564558
/* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
565-
else if (source != PGC_S_OVERRIDE)
559+
if (source != PGC_S_OVERRIDE)
566560
return NULL;
567561
}
568-
if (IsSubTransaction())
562+
else if (IsSubTransaction())
569563
{
570-
if (source >= PGC_S_INTERACTIVE)
571-
ereport(ERROR,
572-
(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
573-
errmsg("SET TRANSACTION ISOLATION LEVEL must not be called in a subtransaction")));
564+
ereport(GUC_complaint_elevel(source),
565+
(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
566+
errmsg("SET TRANSACTION ISOLATION LEVEL must not be called in a subtransaction")));
574567
/* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
575-
else if (source != PGC_S_OVERRIDE)
568+
if (source != PGC_S_OVERRIDE)
576569
return NULL;
577570
}
578571

@@ -667,11 +660,10 @@ assign_client_encoding(const char *value, bool doit, GucSource source)
667660
*/
668661
if (SetClientEncoding(encoding, doit) < 0)
669662
{
670-
if (source >= PGC_S_INTERACTIVE)
671-
ereport(ERROR,
672-
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
673-
errmsg("conversion between %s and %s is not supported",
674-
value, GetDatabaseEncodingName())));
663+
ereport(GUC_complaint_elevel(source),
664+
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
665+
errmsg("conversion between %s and %s is not supported",
666+
value, GetDatabaseEncodingName())));
675667
return NULL;
676668
}
677669
return value;
@@ -740,10 +732,9 @@ assign_session_authorization(const char *value, bool doit, GucSource source)
740732
0, 0, 0);
741733
if (!HeapTupleIsValid(roleTup))
742734
{
743-
if (source >= PGC_S_INTERACTIVE)
744-
ereport(ERROR,
745-
(errcode(ERRCODE_UNDEFINED_OBJECT),
746-
errmsg("role \"%s\" does not exist", value)));
735+
ereport(GUC_complaint_elevel(source),
736+
(errcode(ERRCODE_UNDEFINED_OBJECT),
737+
errmsg("role \"%s\" does not exist", value)));
747738
return NULL;
748739
}
749740

@@ -853,10 +844,9 @@ assign_role(const char *value, bool doit, GucSource source)
853844
0, 0, 0);
854845
if (!HeapTupleIsValid(roleTup))
855846
{
856-
if (source >= PGC_S_INTERACTIVE)
857-
ereport(ERROR,
858-
(errcode(ERRCODE_UNDEFINED_OBJECT),
859-
errmsg("role \"%s\" does not exist", value)));
847+
ereport(GUC_complaint_elevel(source),
848+
(errcode(ERRCODE_UNDEFINED_OBJECT),
849+
errmsg("role \"%s\" does not exist", value)));
860850
return NULL;
861851
}
862852

@@ -870,11 +860,10 @@ assign_role(const char *value, bool doit, GucSource source)
870860
*/
871861
if (!is_member_of_role(GetSessionUserId(), roleid))
872862
{
873-
if (source >= PGC_S_INTERACTIVE)
874-
ereport(ERROR,
875-
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
876-
errmsg("permission denied to set role \"%s\"",
877-
value)));
863+
ereport(GUC_complaint_elevel(source),
864+
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
865+
errmsg("permission denied to set role \"%s\"",
866+
value)));
878867
return NULL;
879868
}
880869
}

‎src/backend/tcop/postgres.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.539 2007/12/06 14:32:54 alvherre Exp $
11+
* $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.540 2007/12/28 00:23:23 tgl Exp $
1212
*
1313
* NOTES
1414
* this is the "main" module of the postgres backend and
@@ -2644,7 +2644,7 @@ assign_max_stack_depth(int newval, bool doit, GucSource source)
26442644

26452645
if (stack_rlimit > 0 && newval_bytes > stack_rlimit - STACK_DEPTH_SLOP)
26462646
{
2647-
ereport((source >= PGC_S_INTERACTIVE) ? ERROR : LOG,
2647+
ereport(GUC_complaint_elevel(source),
26482648
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
26492649
errmsg("\"max_stack_depth\" must not exceed %ldkB",
26502650
(stack_rlimit - STACK_DEPTH_SLOP) / 1024L),

‎src/backend/utils/misc/README

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
$PostgreSQL: pgsql/src/backend/utils/misc/README,v 1.7 2007/09/11 00:06:42 tgl Exp $
1+
$PostgreSQL: pgsql/src/backend/utils/misc/README,v 1.8 2007/12/28 00:23:23 tgl Exp $
22

33

44
GUC IMPLEMENTATION NOTES
@@ -28,16 +28,18 @@ function returns "true" then the assignment is completed; if it returns
2828
performed. If "doit" is false then the function should simply check
2929
validity of newvalue and not change any derived state. The "source" parameter
3030
indicates where the new value came from. If it is >= PGC_S_INTERACTIVE,
31-
then we are performing an interactive assignment (e.g., a SET command).
32-
In such cases it is okay for the assign_hook to raise an error via ereport().
33-
If the function returns false for an interactive assignment then guc.c will
34-
report a generic "invalid value" error message. (An internal ereport() in
35-
an assign_hook is only needed if you want to generate a specialized error
36-
message.) But when source < PGC_S_INTERACTIVE, we are reading a
37-
non-interactive option source, such as postgresql.conf. In this case the
38-
assign_hook should *not* ereport but should just return false if it doesn't
39-
like the newvalue. (An ereport(LOG) call would be acceptable if you feel a
40-
need for a custom complaint in this situation.)
31+
then we are performing an interactive assignment (e.g., a SET command), and
32+
ereport(ERROR) is safe to do. But when source < PGC_S_INTERACTIVE, we are
33+
reading a non-interactive option source, such as postgresql.conf. In this
34+
case the assign_hook should *not* ereport but should just return false if it
35+
doesn't like the newvalue.
36+
37+
If an assign_hook returns false then guc.c will report a generic "invalid
38+
value for option FOO" error message. If you feel the need to provide a more
39+
specific error message, ereport() it using "GUC_complaint_elevel(source)"
40+
as the error level. Note that this might return either ERROR or a lower level
41+
such as LOG, so the ereport call might or might not return. If it does
42+
return, return false out of the assign_hook.
4143

4244
For string variables, the signature for assign hooks is a bit different:
4345
const char *assign_hook(const char *newvalue,

0 commit comments

Comments
 (0)