Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 7ab5b4e

Browse files
committed
Be more careful about GucSource for internally-driven GUC settings.
The original advice for hard-wired SetConfigOption calls was to use PGC_S_OVERRIDE, particularly for PGC_INTERNAL GUCs. However, that's really overkill for PGC_INTERNAL GUCs, since there is no possibility that we need to override a user-provided setting. Instead use PGC_S_DYNAMIC_DEFAULT in most places, so that the value will appear with source = 'default' in pg_settings and thereby not be shown by psql's new \dconfig command. The one exception is that when changing in_hot_standby in a hot-standby session, we still use PGC_S_OVERRIDE, because people felt that seeing that in \dconfig would be a good thing. Similarly use PGC_S_DYNAMIC_DEFAULT for the auto-tune value of wal_buffers (if possible, that is if wal_buffers wasn't explicitly set to -1), and for the typical 2MB value of max_stack_depth. In combination these changes remove four not-very-interesting entries from the typical output of \dconfig, all of which people fingered as "why is that showing up?" in the discussion thread. Discussion: https://postgr.es/m/3118455.1649267333@sss.pgh.pa.us
1 parent abed46a commit 7ab5b4e

File tree

9 files changed

+52
-32
lines changed

9 files changed

+52
-32
lines changed

src/backend/access/transam/xlog.c

+12-3
Original file line numberDiff line numberDiff line change
@@ -4167,7 +4167,7 @@ ReadControlFile(void)
41674167

41684168
snprintf(wal_segsz_str, sizeof(wal_segsz_str), "%d", wal_segment_size);
41694169
SetConfigOption("wal_segment_size", wal_segsz_str, PGC_INTERNAL,
4170-
PGC_S_OVERRIDE);
4170+
PGC_S_DYNAMIC_DEFAULT);
41714171

41724172
/* check and update variables dependent on wal_segment_size */
41734173
if (ConvertToXSegs(min_wal_size_mb, wal_segment_size) < 2)
@@ -4186,7 +4186,7 @@ ReadControlFile(void)
41864186

41874187
/* Make the initdb settings visible as GUC variables, too */
41884188
SetConfigOption("data_checksums", DataChecksumsEnabled() ? "yes" : "no",
4189-
PGC_INTERNAL, PGC_S_OVERRIDE);
4189+
PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
41904190
}
41914191

41924192
/*
@@ -4343,13 +4343,22 @@ XLOGShmemSize(void)
43434343
* This isn't an amazingly clean place to do this, but we must wait till
43444344
* NBuffers has received its final value, and must do it before using the
43454345
* value of XLOGbuffers to do anything important.
4346+
*
4347+
* We prefer to report this value's source as PGC_S_DYNAMIC_DEFAULT.
4348+
* However, if the DBA explicitly set wal_buffers = -1 in the config file,
4349+
* then PGC_S_DYNAMIC_DEFAULT will fail to override that and we must force
4350+
* the matter with PGC_S_OVERRIDE.
43464351
*/
43474352
if (XLOGbuffers == -1)
43484353
{
43494354
char buf[32];
43504355

43514356
snprintf(buf, sizeof(buf), "%d", XLOGChooseNumBuffers());
4352-
SetConfigOption("wal_buffers", buf, PGC_POSTMASTER, PGC_S_OVERRIDE);
4357+
SetConfigOption("wal_buffers", buf, PGC_POSTMASTER,
4358+
PGC_S_DYNAMIC_DEFAULT);
4359+
if (XLOGbuffers == -1) /* failed to apply it? */
4360+
SetConfigOption("wal_buffers", buf, PGC_POSTMASTER,
4361+
PGC_S_OVERRIDE);
43534362
}
43544363
Assert(XLOGbuffers > 0);
43554364

src/backend/bootstrap/bootstrap.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
262262
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
263263
errmsg("-X requires a power of two value between 1 MB and 1 GB")));
264264
SetConfigOption("wal_segment_size", optarg, PGC_INTERNAL,
265-
PGC_S_OVERRIDE);
265+
PGC_S_DYNAMIC_DEFAULT);
266266
}
267267
break;
268268
case 'c':

src/backend/postmaster/postmaster.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -921,7 +921,7 @@ PostmasterMain(int argc, char *argv[])
921921
* useful to show, even if one would only expect at least PANIC. LOG
922922
* entries are hidden.
923923
*/
924-
SetConfigOption("log_min_messages", "FATAL", PGC_INTERNAL,
924+
SetConfigOption("log_min_messages", "FATAL", PGC_SUSET,
925925
PGC_S_OVERRIDE);
926926
}
927927

src/backend/storage/ipc/ipci.c

+4-2
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,8 @@ InitializeShmemGUCs(void)
334334
size_b = CalculateShmemSize(NULL);
335335
size_mb = add_size(size_b, (1024 * 1024) - 1) / (1024 * 1024);
336336
sprintf(buf, "%zu", size_mb);
337-
SetConfigOption("shared_memory_size", buf, PGC_INTERNAL, PGC_S_OVERRIDE);
337+
SetConfigOption("shared_memory_size", buf,
338+
PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
338339

339340
/*
340341
* Calculate the number of huge pages required.
@@ -346,6 +347,7 @@ InitializeShmemGUCs(void)
346347

347348
hp_required = add_size(size_b / hp_size, 1);
348349
sprintf(buf, "%zu", hp_required);
349-
SetConfigOption("shared_memory_size_in_huge_pages", buf, PGC_INTERNAL, PGC_S_OVERRIDE);
350+
SetConfigOption("shared_memory_size_in_huge_pages", buf,
351+
PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
350352
}
351353
}

src/backend/utils/init/miscinit.c

+3-3
Original file line numberDiff line numberDiff line change
@@ -787,7 +787,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
787787
PGC_BACKEND, PGC_S_OVERRIDE);
788788
SetConfigOption("is_superuser",
789789
AuthenticatedUserIsSuperuser ? "on" : "off",
790-
PGC_INTERNAL, PGC_S_OVERRIDE);
790+
PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
791791

792792
ReleaseSysCache(roleTup);
793793
}
@@ -844,7 +844,7 @@ SetSessionAuthorization(Oid userid, bool is_superuser)
844844

845845
SetConfigOption("is_superuser",
846846
is_superuser ? "on" : "off",
847-
PGC_INTERNAL, PGC_S_OVERRIDE);
847+
PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
848848
}
849849

850850
/*
@@ -901,7 +901,7 @@ SetCurrentRoleId(Oid roleid, bool is_superuser)
901901

902902
SetConfigOption("is_superuser",
903903
is_superuser ? "on" : "off",
904-
PGC_INTERNAL, PGC_S_OVERRIDE);
904+
PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
905905
}
906906

907907

src/backend/utils/init/postinit.c

+3-3
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,7 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect
391391
SetDatabaseEncoding(dbform->encoding);
392392
/* Record it as a GUC internal option, too */
393393
SetConfigOption("server_encoding", GetDatabaseEncodingName(),
394-
PGC_INTERNAL, PGC_S_OVERRIDE);
394+
PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
395395
/* If we have no other source of client_encoding, use server encoding */
396396
SetConfigOption("client_encoding", GetDatabaseEncodingName(),
397397
PGC_BACKEND, PGC_S_DYNAMIC_DEFAULT);
@@ -470,8 +470,8 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect
470470
}
471471

472472
/* Make the locale settings visible as GUC variables, too */
473-
SetConfigOption("lc_collate", collate, PGC_INTERNAL, PGC_S_OVERRIDE);
474-
SetConfigOption("lc_ctype", ctype, PGC_INTERNAL, PGC_S_OVERRIDE);
473+
SetConfigOption("lc_collate", collate, PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
474+
SetConfigOption("lc_ctype", ctype, PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
475475

476476
check_strxfrm_bug();
477477

src/backend/utils/misc/guc-file.l

+2-3
Original file line numberDiff line numberDiff line change
@@ -401,9 +401,8 @@ ProcessConfigFileInternal(GucContext context, bool applySettings, int elevel)
401401
* be run only after initialization is complete.
402402
*
403403
* XXX this is an unmaintainable crock, because we have to know how to set
404-
* (or at least what to call to set) every variable that could potentially
405-
* have PGC_S_DYNAMIC_DEFAULT or PGC_S_ENV_VAR source. However, there's no
406-
* time to redesign it for 9.1.
404+
* (or at least what to call to set) every non-PGC_INTERNAL variable that
405+
* could potentially have PGC_S_DYNAMIC_DEFAULT or PGC_S_ENV_VAR source.
407406
*/
408407
if (context == PGC_SIGHUP && applySettings)
409408
{

src/backend/utils/misc/guc.c

+20-14
Original file line numberDiff line numberDiff line change
@@ -5939,6 +5939,8 @@ InitializeGUCOptionsFromEnvironment(void)
59395939
* rlimit isn't exactly an "environment variable", but it behaves about
59405940
* the same. If we can identify the platform stack depth rlimit, increase
59415941
* default stack depth setting up to whatever is safe (but at most 2MB).
5942+
* Report the value's source as PGC_S_DYNAMIC_DEFAULT if it's 2MB, or as
5943+
* PGC_S_ENV_VAR if it's reflecting the rlimit limit.
59425944
*/
59435945
stack_rlimit = get_stack_depth_rlimit();
59445946
if (stack_rlimit > 0)
@@ -5947,12 +5949,19 @@ InitializeGUCOptionsFromEnvironment(void)
59475949

59485950
if (new_limit > 100)
59495951
{
5952+
GucSource source;
59505953
char limbuf[16];
59515954

5952-
new_limit = Min(new_limit, 2048);
5953-
sprintf(limbuf, "%ld", new_limit);
5955+
if (new_limit < 2048)
5956+
source = PGC_S_ENV_VAR;
5957+
else
5958+
{
5959+
new_limit = 2048;
5960+
source = PGC_S_DYNAMIC_DEFAULT;
5961+
}
5962+
snprintf(limbuf, sizeof(limbuf), "%ld", new_limit);
59545963
SetConfigOption("max_stack_depth", limbuf,
5955-
PGC_POSTMASTER, PGC_S_ENV_VAR);
5964+
PGC_POSTMASTER, source);
59565965
}
59575966
}
59585967
}
@@ -6776,12 +6785,16 @@ BeginReportingGUCOptions(void)
67766785
reporting_enabled = true;
67776786

67786787
/*
6779-
* Hack for in_hot_standby: initialize with the value we're about to send.
6788+
* Hack for in_hot_standby: set the GUC value true if appropriate. This
6789+
* is kind of an ugly place to do it, but there's few better options.
6790+
*
67806791
* (This could be out of date by the time we actually send it, in which
67816792
* case the next ReportChangedGUCOptions call will send a duplicate
67826793
* report.)
67836794
*/
6784-
in_hot_standby = RecoveryInProgress();
6795+
if (RecoveryInProgress())
6796+
SetConfigOption("in_hot_standby", "true",
6797+
PGC_INTERNAL, PGC_S_OVERRIDE);
67856798

67866799
/* Transmit initial values of interesting variables */
67876800
for (i = 0; i < num_guc_variables; i++)
@@ -6822,15 +6835,8 @@ ReportChangedGUCOptions(void)
68226835
* transition from false to true.
68236836
*/
68246837
if (in_hot_standby && !RecoveryInProgress())
6825-
{
6826-
struct config_generic *record;
6827-
6828-
record = find_option("in_hot_standby", false, false, ERROR);
6829-
Assert(record != NULL);
6830-
record->status |= GUC_NEEDS_REPORT;
6831-
report_needed = true;
6832-
in_hot_standby = false;
6833-
}
6838+
SetConfigOption("in_hot_standby", "false",
6839+
PGC_INTERNAL, PGC_S_OVERRIDE);
68346840

68356841
/* Quick exit if no values have been changed */
68366842
if (!report_needed)

src/include/utils/guc.h

+6-2
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,7 @@ typedef enum
8383
* override the postmaster command line.) Tracking the source allows us
8484
* to process sources in any convenient order without affecting results.
8585
* Sources <= PGC_S_OVERRIDE will set the default used by RESET, as well
86-
* as the current value. Note that source == PGC_S_OVERRIDE should be
87-
* used when setting a PGC_INTERNAL option.
86+
* as the current value.
8887
*
8988
* PGC_S_INTERACTIVE isn't actually a source value, but is the
9089
* dividing line between "interactive" and "non-interactive" sources for
@@ -99,6 +98,11 @@ typedef enum
9998
* shouldn't throw hard errors in this case, at most NOTICEs, since the
10099
* objects might exist by the time the setting is used for real.
101100
*
101+
* When setting the value of a non-compile-time-constant PGC_INTERNAL option,
102+
* source == PGC_S_DYNAMIC_DEFAULT should typically be used so that the value
103+
* will show as "default" in pg_settings. If there is a specific reason not
104+
* to want that, use source == PGC_S_OVERRIDE.
105+
*
102106
* NB: see GucSource_Names in guc.c if you change this.
103107
*/
104108
typedef enum

0 commit comments

Comments
 (0)