Skip to content

Commit 8810356

Browse files
tglsfdctglsfdc
committed
Disallow setting bogus GUCs within an extension's reserved namespace.
Commit 75d2206 tried to throw a warning for setting a custom GUC whose prefix belongs to a previously-loaded extension, if there is no such GUC defined by the extension. But that caused unstable behavior with parallel workers, because workers don't necessarily load extensions and GUCs in the same order their leader did. To make that work safely, we have to completely disallow the case. We now actually remove any such GUCs at the time of initial extension load, and then throw an error not just a warning if you try to add one later. While this might create a compatibility issue for a few people, the improvement in error-detection capability seems worth it; it's hard to believe that there's any good use-case for choosing such GUC names. This also un-reverts 5609cc0 (Rename EmitWarningsOnPlaceholders() to MarkGUCPrefixReserved()), since that function's old name is now even more of a misnomer. Florin Irion and Tom Lane Discussion: https://postgr.es/m/1902182.1640711215@sss.pgh.pa.us
1 parent 2776922 commit 8810356

File tree

18 files changed

+101
-31
lines changed

18 files changed

+101
-31
lines changed

‎contrib/auth_delay/auth_delay.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ _PG_init(void)
6868
NULL,
6969
NULL);
7070

71-
EmitWarningsOnPlaceholders("auth_delay");
71+
MarkGUCPrefixReserved("auth_delay");
7272

7373
/* Install Hooks */
7474
original_client_auth_hook = ClientAuthentication_hook;

‎contrib/auto_explain/auto_explain.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ _PG_init(void)
231231
NULL,
232232
NULL);
233233

234-
EmitWarningsOnPlaceholders("auto_explain");
234+
MarkGUCPrefixReserved("auto_explain");
235235

236236
/* Install hooks. */
237237
prev_ExecutorStart = ExecutorStart_hook;

‎contrib/basic_archive/basic_archive.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ _PG_init(void)
6969
0,
7070
check_archive_directory, NULL, NULL);
7171

72-
EmitWarningsOnPlaceholders("basic_archive");
72+
MarkGUCPrefixReserved("basic_archive");
7373

7474
basic_archive_context = AllocSetContextCreate(TopMemoryContext,
7575
"basic_archive",

‎contrib/pg_prewarm/autoprewarm.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ _PG_init(void)
137137
NULL,
138138
NULL);
139139

140-
EmitWarningsOnPlaceholders("pg_prewarm");
140+
MarkGUCPrefixReserved("pg_prewarm");
141141

142142
RequestAddinShmemSpace(MAXALIGN(sizeof(AutoPrewarmSharedState)));
143143

‎contrib/pg_stat_statements/pg_stat_statements.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -437,7 +437,7 @@ _PG_init(void)
437437
NULL,
438438
NULL);
439439

440-
EmitWarningsOnPlaceholders("pg_stat_statements");
440+
MarkGUCPrefixReserved("pg_stat_statements");
441441

442442
/*
443443
* Request additional shared resources. (These are no-ops if we're not in

‎contrib/pg_trgm/trgm_op.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ _PG_init(void)
101101
NULL,
102102
NULL);
103103

104-
EmitWarningsOnPlaceholders("pg_trgm");
104+
MarkGUCPrefixReserved("pg_trgm");
105105
}
106106

107107
/*

‎contrib/postgres_fdw/option.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -538,5 +538,5 @@ _PG_init(void)
538538
NULL,
539539
NULL);
540540

541-
EmitWarningsOnPlaceholders("postgres_fdw");
541+
MarkGUCPrefixReserved("postgres_fdw");
542542
}

‎contrib/sepgsql/hooks.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -455,7 +455,7 @@ _PG_init(void)
455455
NULL,
456456
NULL);
457457

458-
EmitWarningsOnPlaceholders("sepgsql");
458+
MarkGUCPrefixReserved("sepgsql");
459459

460460
/* Initialize userspace access vector cache */
461461
sepgsql_avc_init();

‎src/backend/utils/misc/guc.c

+64-15
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,8 @@ extern bool optimize_bounded_sort;
150150

151151
static int GUC_check_errcode_value;
152152

153+
static List *reserved_class_prefix = NIL;
154+
153155
/* global variables for check hook support */
154156
char *GUC_check_errmsg_string;
155157
char *GUC_check_errdetail_string;
@@ -5590,18 +5592,44 @@ find_option(const char *name, bool create_placeholders, bool skip_errors,
55905592
* doesn't contain a separator, don't assume that it was meant to be a
55915593
* placeholder.
55925594
*/
5593-
if (strchr(name, GUC_QUALIFIER_SEPARATOR) != NULL)
5595+
const char *sep = strchr(name, GUC_QUALIFIER_SEPARATOR);
5596+
5597+
if (sep != NULL)
55945598
{
5595-
if (valid_custom_variable_name(name))
5596-
return add_placeholder_variable(name, elevel);
5597-
/* A special error message seems desirable here */
5598-
if (!skip_errors)
5599-
ereport(elevel,
5600-
(errcode(ERRCODE_INVALID_NAME),
5601-
errmsg("invalid configuration parameter name \"%s\"",
5602-
name),
5603-
errdetail("Custom parameter names must be two or more simple identifiers separated by dots.")));
5604-
return NULL;
5599+
size_t classLen = sep - name;
5600+
ListCell *lc;
5601+
5602+
/* The name must be syntactically acceptable ... */
5603+
if (!valid_custom_variable_name(name))
5604+
{
5605+
if (!skip_errors)
5606+
ereport(elevel,
5607+
(errcode(ERRCODE_INVALID_NAME),
5608+
errmsg("invalid configuration parameter name \"%s\"",
5609+
name),
5610+
errdetail("Custom parameter names must be two or more simple identifiers separated by dots.")));
5611+
return NULL;
5612+
}
5613+
/* ... and it must not match any previously-reserved prefix */
5614+
foreach(lc, reserved_class_prefix)
5615+
{
5616+
const char *rcprefix = lfirst(lc);
5617+
5618+
if (strlen(rcprefix) == classLen &&
5619+
strncmp(name, rcprefix, classLen) == 0)
5620+
{
5621+
if (!skip_errors)
5622+
ereport(elevel,
5623+
(errcode(ERRCODE_INVALID_NAME),
5624+
errmsg("invalid configuration parameter name \"%s\"",
5625+
name),
5626+
errdetail("\"%s\" is a reserved prefix.",
5627+
rcprefix)));
5628+
return NULL;
5629+
}
5630+
}
5631+
/* OK, create it */
5632+
return add_placeholder_variable(name, elevel);
56055633
}
56065634
}
56075635

@@ -9355,15 +9383,26 @@ DefineCustomEnumVariable(const char *name,
93559383
}
93569384

93579385
/*
9386+
* Mark the given GUC prefix as "reserved".
9387+
*
9388+
* This deletes any existing placeholders matching the prefix,
9389+
* and then prevents new ones from being created.
93589390
* Extensions should call this after they've defined all of their custom
93599391
* GUCs, to help catch misspelled config-file entries.
93609392
*/
93619393
void
9362-
EmitWarningsOnPlaceholders(const char *className)
9394+
MarkGUCPrefixReserved(const char *className)
93639395
{
93649396
int classLen = strlen(className);
93659397
int i;
9398+
MemoryContext oldcontext;
93669399

9400+
/*
9401+
* Check for existing placeholders. We must actually remove invalid
9402+
* placeholders, else future parallel worker startups will fail. (We
9403+
* don't bother trying to free associated memory, since this shouldn't
9404+
* happen often.)
9405+
*/
93679406
for (i = 0; i < num_guc_variables; i++)
93689407
{
93699408
struct config_generic *var = guc_variables[i];
@@ -9373,11 +9412,21 @@ EmitWarningsOnPlaceholders(const char *className)
93739412
var->name[classLen] == GUC_QUALIFIER_SEPARATOR)
93749413
{
93759414
ereport(WARNING,
9376-
(errcode(ERRCODE_UNDEFINED_OBJECT),
9377-
errmsg("unrecognized configuration parameter \"%s\"",
9378-
var->name)));
9415+
(errcode(ERRCODE_INVALID_NAME),
9416+
errmsg("invalid configuration parameter name \"%s\", removing it",
9417+
var->name),
9418+
errdetail("\"%s\" is now a reserved prefix.",
9419+
className)));
9420+
num_guc_variables--;
9421+
memmove(&guc_variables[i], &guc_variables[i + 1],
9422+
(num_guc_variables - i) * sizeof(struct config_generic *));
93799423
}
93809424
}
9425+
9426+
/* And remember the name so we can prevent future mistakes. */
9427+
oldcontext = MemoryContextSwitchTo(TopMemoryContext);
9428+
reserved_class_prefix = lappend(reserved_class_prefix, pstrdup(className));
9429+
MemoryContextSwitchTo(oldcontext);
93819430
}
93829431

93839432

‎src/include/utils/guc.h

+4-1
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,10 @@ extern void DefineCustomEnumVariable(const char *name,
354354
GucEnumAssignHook assign_hook,
355355
GucShowHook show_hook);
356356

357-
extern void EmitWarningsOnPlaceholders(const char *className);
357+
extern void MarkGUCPrefixReserved(const char *className);
358+
359+
/* old name for MarkGUCPrefixReserved, for backwards compatibility: */
360+
#define EmitWarningsOnPlaceholders(className) MarkGUCPrefixReserved(className)
358361

359362
extern const char *GetConfigOption(const char *name, bool missing_ok,
360363
bool restrict_privileged);

‎src/pl/plperl/plperl.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -455,7 +455,7 @@ _PG_init(void)
455455
PGC_SUSET, 0,
456456
NULL, NULL, NULL);
457457

458-
EmitWarningsOnPlaceholders("plperl");
458+
MarkGUCPrefixReserved("plperl");
459459

460460
/*
461461
* Create hash tables.

‎src/pl/plpgsql/src/pl_handler.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ _PG_init(void)
197197
plpgsql_extra_errors_assign_hook,
198198
NULL);
199199

200-
EmitWarningsOnPlaceholders("plpgsql");
200+
MarkGUCPrefixReserved("plpgsql");
201201

202202
plpgsql_HashTableInit();
203203
RegisterXactCallback(plpgsql_xact_cb, NULL);

‎src/pl/tcl/pltcl.c

+2-2
Original file line numberDiff line numberDiff line change
@@ -474,8 +474,8 @@ _PG_init(void)
474474
PGC_SUSET, 0,
475475
NULL, NULL, NULL);
476476

477-
EmitWarningsOnPlaceholders("pltcl");
478-
EmitWarningsOnPlaceholders("pltclu");
477+
MarkGUCPrefixReserved("pltcl");
478+
MarkGUCPrefixReserved("pltclu");
479479

480480
pltcl_pm_init_done = true;
481481
}

‎src/test/modules/delay_execution/delay_execution.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ _PG_init(void)
9191
NULL,
9292
NULL);
9393

94-
EmitWarningsOnPlaceholders("delay_execution");
94+
MarkGUCPrefixReserved("delay_execution");
9595

9696
/* Install our hook */
9797
prev_planner_hook = planner_hook;

‎src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ _PG_init(void)
4949
NULL,
5050
NULL);
5151

52-
EmitWarningsOnPlaceholders("ssl_passphrase");
52+
MarkGUCPrefixReserved("ssl_passphrase");
5353

5454
if (ssl_passphrase)
5555
openssl_tls_init_hook = set_rot13;

‎src/test/modules/worker_spi/worker_spi.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,7 @@ _PG_init(void)
322322
0,
323323
NULL, NULL, NULL);
324324

325-
EmitWarningsOnPlaceholders("worker_spi");
325+
MarkGUCPrefixReserved("worker_spi");
326326

327327
/* set up common data for all our workers */
328328
memset(&worker, 0, sizeof(worker));

‎src/test/regress/expected/guc.out

+11
Original file line numberDiff line numberDiff line change
@@ -548,6 +548,17 @@ ERROR: invalid configuration parameter name "special.weird name"
548548
DETAIL: Custom parameter names must be two or more simple identifiers separated by dots.
549549
SHOW special."weird name";
550550
ERROR: unrecognized configuration parameter "special.weird name"
551+
-- Check what happens when you try to set a "custom" GUC within the
552+
-- namespace of an extension.
553+
SET plpgsql.extra_foo_warnings = true; -- allowed if plpgsql is not loaded yet
554+
LOAD 'plpgsql'; -- this will throw a warning and delete the variable
555+
WARNING: invalid configuration parameter name "plpgsql.extra_foo_warnings", removing it
556+
DETAIL: "plpgsql" is now a reserved prefix.
557+
SET plpgsql.extra_foo_warnings = true; -- now, it's an error
558+
ERROR: invalid configuration parameter name "plpgsql.extra_foo_warnings"
559+
DETAIL: "plpgsql" is a reserved prefix.
560+
SHOW plpgsql.extra_foo_warnings;
561+
ERROR: unrecognized configuration parameter "plpgsql.extra_foo_warnings"
551562
--
552563
-- Test DISCARD TEMP
553564
--

‎src/test/regress/sql/guc.sql

+7
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,13 @@ SHOW custom."bad-guc";
163163
SET special."weird name" = 'foo'; -- could be allowed, but we choose not to
164164
SHOW special."weird name";
165165

166+
-- Check what happens when you try to set a "custom" GUC within the
167+
-- namespace of an extension.
168+
SET plpgsql.extra_foo_warnings = true; -- allowed if plpgsql is not loaded yet
169+
LOAD 'plpgsql'; -- this will throw a warning and delete the variable
170+
SET plpgsql.extra_foo_warnings = true; -- now, it's an error
171+
SHOW plpgsql.extra_foo_warnings;
172+
166173
--
167174
-- Test DISCARD TEMP
168175
--

0 commit comments

Comments
 (0)