Skip to content

Commit 2594cf0

Browse files
tglsfdctglsfdc
committed
Revise the API for GUC variable assign hooks.
The previous functions of assign hooks are now split between check hooks and assign hooks, where the former can fail but the latter shouldn't. Aside from being conceptually clearer, this approach exposes the "canonicalized" form of the variable value to guc.c without having to do an actual assignment. And that lets us fix the problem recently noted by Bernd Helmle that the auto-tune patch for wal_buffers resulted in bogus log messages about "parameter "wal_buffers" cannot be changed without restarting the server". There may be some speed advantage too, because this design lets hook functions avoid re-parsing variable values when restoring a previous state after a rollback (they can store a pre-parsed representation of the value instead). This patch also resolves a longstanding annoyance about custom error messages from variable assign hooks: they should modify, not appear separately from, guc.c's own message about "invalid parameter value".
1 parent 5d0e462 commit 2594cf0

File tree

35 files changed

+2611
-1854
lines changed

35 files changed

+2611
-1854
lines changed

‎contrib/auth_delay/auth_delay.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ auth_delay_checks(Port *port, int status)
5353
void
5454
_PG_init(void)
5555
{
56-
/* Define custome GUC variables */
56+
/* Define custom GUC variables */
5757
DefineCustomIntVariable("auth_delay.milliseconds",
5858
"Milliseconds to delay before reporting authentication failure",
5959
NULL,
@@ -63,6 +63,7 @@ _PG_init(void)
6363
PGC_SIGHUP,
6464
GUC_UNIT_MS,
6565
NULL,
66+
NULL,
6667
NULL);
6768
/* Install Hooks */
6869
original_client_auth_hook = ClientAuthentication_hook;

‎contrib/auto_explain/auto_explain.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ _PG_init(void)
7474
PGC_SUSET,
7575
GUC_UNIT_MS,
7676
NULL,
77+
NULL,
7778
NULL);
7879

7980
DefineCustomBoolVariable("auto_explain.log_analyze",
@@ -84,6 +85,7 @@ _PG_init(void)
8485
PGC_SUSET,
8586
0,
8687
NULL,
88+
NULL,
8789
NULL);
8890

8991
DefineCustomBoolVariable("auto_explain.log_verbose",
@@ -94,6 +96,7 @@ _PG_init(void)
9496
PGC_SUSET,
9597
0,
9698
NULL,
99+
NULL,
97100
NULL);
98101

99102
DefineCustomBoolVariable("auto_explain.log_buffers",
@@ -104,6 +107,7 @@ _PG_init(void)
104107
PGC_SUSET,
105108
0,
106109
NULL,
110+
NULL,
107111
NULL);
108112

109113
DefineCustomEnumVariable("auto_explain.log_format",
@@ -115,6 +119,7 @@ _PG_init(void)
115119
PGC_SUSET,
116120
0,
117121
NULL,
122+
NULL,
118123
NULL);
119124

120125
DefineCustomBoolVariable("auto_explain.log_nested_statements",
@@ -125,6 +130,7 @@ _PG_init(void)
125130
PGC_SUSET,
126131
0,
127132
NULL,
133+
NULL,
128134
NULL);
129135

130136
EmitWarningsOnPlaceholders("auto_explain");

‎contrib/pg_stat_statements/pg_stat_statements.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,7 @@ _PG_init(void)
219219
PGC_POSTMASTER,
220220
0,
221221
NULL,
222+
NULL,
222223
NULL);
223224

224225
DefineCustomEnumVariable("pg_stat_statements.track",
@@ -230,6 +231,7 @@ _PG_init(void)
230231
PGC_SUSET,
231232
0,
232233
NULL,
234+
NULL,
233235
NULL);
234236

235237
DefineCustomBoolVariable("pg_stat_statements.track_utility",
@@ -240,6 +242,7 @@ _PG_init(void)
240242
PGC_SUSET,
241243
0,
242244
NULL,
245+
NULL,
243246
NULL);
244247

245248
DefineCustomBoolVariable("pg_stat_statements.save",
@@ -250,6 +253,7 @@ _PG_init(void)
250253
PGC_SIGHUP,
251254
0,
252255
NULL,
256+
NULL,
253257
NULL);
254258

255259
EmitWarningsOnPlaceholders("pg_stat_statements");

‎contrib/sepgsql/hooks.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,7 @@ _PG_init(void)
394394
PGC_SIGHUP,
395395
GUC_NOT_IN_SAMPLE,
396396
NULL,
397+
NULL,
397398
NULL);
398399

399400
/*
@@ -412,6 +413,7 @@ _PG_init(void)
412413
PGC_USERSET,
413414
GUC_NOT_IN_SAMPLE,
414415
NULL,
416+
NULL,
415417
NULL);
416418

417419
/*

‎src/backend/access/transam/xlog.c

Lines changed: 63 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -4887,36 +4887,60 @@ GetSystemIdentifier(void)
48874887
/*
48884888
* Auto-tune the number of XLOG buffers.
48894889
*
4890-
* If the user-set value of wal_buffers is -1, we auto-tune to about 3% of
4891-
* shared_buffers, with a maximum of one XLOG segment and a minimum of 8
4892-
* blocks (8 was the default value prior to PostgreSQL 9.1, when auto-tuning
4893-
* was added). We also clamp manually-set values to at least 4 blocks; prior
4894-
* to PostgreSQL 9.1, a minimum of 4 was enforced by guc.c, but since that
4895-
* is no longer possible, we just silently treat such values as a request for
4896-
* the minimum.
4890+
* The preferred setting for wal_buffers is about 3% of shared_buffers, with
4891+
* a maximum of one XLOG segment (there is little reason to think that more
4892+
* is helpful, at least so long as we force an fsync when switching log files)
4893+
* and a minimum of 8 blocks (which was the default value prior to PostgreSQL
4894+
* 9.1, when auto-tuning was added).
4895+
*
4896+
* This should not be called until NBuffers has received its final value.
48974897
*/
4898-
static void
4899-
XLOGTuneNumBuffers(void)
4898+
static int
4899+
XLOGChooseNumBuffers(void)
49004900
{
4901-
int xbuffers = XLOGbuffers;
4902-
char buf[32];
4901+
int xbuffers;
49034902

4904-
if (xbuffers == -1)
4905-
{
4906-
xbuffers = NBuffers / 32;
4907-
if (xbuffers > XLOG_SEG_SIZE / XLOG_BLCKSZ)
4908-
xbuffers = XLOG_SEG_SIZE / XLOG_BLCKSZ;
4909-
if (xbuffers < 8)
4910-
xbuffers = 8;
4911-
}
4912-
else if (xbuffers < 4)
4913-
xbuffers = 4;
4903+
xbuffers = NBuffers / 32;
4904+
if (xbuffers > XLOG_SEG_SIZE / XLOG_BLCKSZ)
4905+
xbuffers = XLOG_SEG_SIZE / XLOG_BLCKSZ;
4906+
if (xbuffers < 8)
4907+
xbuffers = 8;
4908+
return xbuffers;
4909+
}
49144910

4915-
if (xbuffers != XLOGbuffers)
4911+
/*
4912+
* GUC check_hook for wal_buffers
4913+
*/
4914+
bool
4915+
check_wal_buffers(int *newval, void **extra, GucSource source)
4916+
{
4917+
/*
4918+
* -1 indicates a request for auto-tune.
4919+
*/
4920+
if (*newval == -1)
49164921
{
4917-
snprintf(buf, sizeof(buf), "%d", xbuffers);
4918-
SetConfigOption("wal_buffers", buf, PGC_POSTMASTER, PGC_S_OVERRIDE);
4922+
/*
4923+
* If we haven't yet changed the boot_val default of -1, just let it
4924+
* be. We'll fix it when XLOGShmemSize is called.
4925+
*/
4926+
if (XLOGbuffers == -1)
4927+
return true;
4928+
4929+
/* Otherwise, substitute the auto-tune value */
4930+
*newval = XLOGChooseNumBuffers();
49194931
}
4932+
4933+
/*
4934+
* We clamp manually-set values to at least 4 blocks. Prior to PostgreSQL
4935+
* 9.1, a minimum of 4 was enforced by guc.c, but since that is no longer
4936+
* the case, we just silently treat such values as a request for the
4937+
* minimum. (We could throw an error instead, but that doesn't seem very
4938+
* helpful.)
4939+
*/
4940+
if (*newval < 4)
4941+
*newval = 4;
4942+
4943+
return true;
49204944
}
49214945

49224946
/*
@@ -4927,8 +4951,19 @@ XLOGShmemSize(void)
49274951
{
49284952
Size size;
49294953

4930-
/* Figure out how many XLOG buffers we need. */
4931-
XLOGTuneNumBuffers();
4954+
/*
4955+
* If the value of wal_buffers is -1, use the preferred auto-tune value.
4956+
* This isn't an amazingly clean place to do this, but we must wait till
4957+
* NBuffers has received its final value, and must do it before using
4958+
* the value of XLOGbuffers to do anything important.
4959+
*/
4960+
if (XLOGbuffers == -1)
4961+
{
4962+
char buf[32];
4963+
4964+
snprintf(buf, sizeof(buf), "%d", XLOGChooseNumBuffers());
4965+
SetConfigOption("wal_buffers", buf, PGC_POSTMASTER, PGC_S_OVERRIDE);
4966+
}
49324967
Assert(XLOGbuffers > 0);
49334968

49344969
/* XLogCtl */
@@ -8653,12 +8688,9 @@ get_sync_bit(int method)
86538688
/*
86548689
* GUC support
86558690
*/
8656-
bool
8657-
assign_xlog_sync_method(int new_sync_method, bool doit, GucSource source)
8691+
void
8692+
assign_xlog_sync_method(int new_sync_method, void *extra)
86588693
{
8659-
if (!doit)
8660-
return true;
8661-
86628694
if (sync_method != new_sync_method)
86638695
{
86648696
/*
@@ -8678,8 +8710,6 @@ assign_xlog_sync_method(int new_sync_method, bool doit, GucSource source)
86788710
XLogFileClose();
86798711
}
86808712
}
8681-
8682-
return true;
86838713
}
86848714

86858715

‎src/backend/catalog/namespace.c

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3468,31 +3468,33 @@ ResetTempTableNamespace(void)
34683468
* Routines for handling the GUC variable 'search_path'.
34693469
*/
34703470

3471-
/* assign_hook: validate new search_path, do extra actions as needed */
3472-
const char *
3473-
assign_search_path(const char *newval, bool doit, GucSource source)
3471+
/* check_hook: validate new search_path, if possible */
3472+
bool
3473+
check_search_path(char **newval, void **extra, GucSource source)
34743474
{
3475+
bool result = true;
34753476
char *rawname;
34763477
List *namelist;
34773478
ListCell *l;
34783479

34793480
/* Need a modifiable copy of string */
3480-
rawname = pstrdup(newval);
3481+
rawname = pstrdup(*newval);
34813482

34823483
/* Parse string into list of identifiers */
34833484
if (!SplitIdentifierString(rawname, ',', &namelist))
34843485
{
34853486
/* syntax error in name list */
3487+
GUC_check_errdetail("List syntax is invalid.");
34863488
pfree(rawname);
34873489
list_free(namelist);
3488-
return NULL;
3490+
return false;
34893491
}
34903492

34913493
/*
34923494
* If we aren't inside a transaction, we cannot do database access so
34933495
* cannot verify the individual names. Must accept the list on faith.
34943496
*/
3495-
if (source >= PGC_S_INTERACTIVE && IsTransactionState())
3497+
if (IsTransactionState())
34963498
{
34973499
/*
34983500
* Verify that all the names are either valid namespace names or
@@ -3504,7 +3506,7 @@ assign_search_path(const char *newval, bool doit, GucSource source)
35043506
* DATABASE SET or ALTER USER SET command. It could be that the
35053507
* intended use of the search path is for some other database, so we
35063508
* should not error out if it mentions schemas not present in the
3507-
* current database. We reduce the message to NOTICE instead.
3509+
* current database. We issue a NOTICE instead.
35083510
*/
35093511
foreach(l, namelist)
35103512
{
@@ -3516,24 +3518,37 @@ assign_search_path(const char *newval, bool doit, GucSource source)
35163518
continue;
35173519
if (!SearchSysCacheExists1(NAMESPACENAME,
35183520
CStringGetDatum(curname)))
3519-
ereport((source == PGC_S_TEST) ? NOTICE : ERROR,
3520-
(errcode(ERRCODE_UNDEFINED_SCHEMA),
3521-
errmsg("schema \"%s\" does not exist", curname)));
3521+
{
3522+
if (source == PGC_S_TEST)
3523+
ereport(NOTICE,
3524+
(errcode(ERRCODE_UNDEFINED_SCHEMA),
3525+
errmsg("schema \"%s\" does not exist", curname)));
3526+
else
3527+
{
3528+
GUC_check_errdetail("schema \"%s\" does not exist", curname);
3529+
result = false;
3530+
break;
3531+
}
3532+
}
35223533
}
35233534
}
35243535

35253536
pfree(rawname);
35263537
list_free(namelist);
35273538

3539+
return result;
3540+
}
3541+
3542+
/* assign_hook: do extra actions as needed */
3543+
void
3544+
assign_search_path(const char *newval, void *extra)
3545+
{
35283546
/*
35293547
* We mark the path as needing recomputation, but don't do anything until
35303548
* it's needed. This avoids trying to do database access during GUC
3531-
* initialization.
3549+
* initialization, or outside a transaction.
35323550
*/
3533-
if (doit)
3534-
baseSearchPathValid = false;
3535-
3536-
return newval;
3551+
baseSearchPathValid = false;
35373552
}
35383553

35393554
/*

0 commit comments

Comments
 (0)