Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitf0c2a5b

Browse files
committed
Avoid leaking memory in RestoreGUCState(), and improve comments.
RestoreGUCState applied InitializeOneGUCOption to already-liveGUC entries, causing any malloc'd subsidiary data to be forgotten.We do want the effect of resetting the GUC to its compiled-indefault, and InitializeOneGUCOption seems like the best way to dothat, so add code to free any existing subsidiary data beforehand.The interaction between can_skip_gucvar, SerializeGUCState, andRestoreGUCState is way more subtle than their opaque commentswould suggest to an unwary reader. Rewrite and enlarge thecomments to try to make it clearer what's happening.Remove a long-obsolete assertion in read_nondefault_variables: thebehavior of set_config_option hasn't depended on IsInitProcessingModesincef5d9698 installed a better way of controlling it.Although this is fixing a clear memory leak, the leak is quite unlikelyto involve any large amount of data, and it can only happen once in thelifetime of a worker process. So it seems unnecessary to take anyrisk of back-patching.Discussion:https://postgr.es/m/4105247.1616174862@sss.pgh.pa.us
1 parent61752af commitf0c2a5b

File tree

1 file changed

+140
-30
lines changed
  • src/backend/utils/misc

1 file changed

+140
-30
lines changed

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

Lines changed: 140 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -7121,6 +7121,10 @@ parse_and_validate_value(struct config_generic *record,
71217121
* its standard choice of ereport level. However some callers need to be
71227122
* able to override that choice; they should pass the ereport level to use.
71237123
*
7124+
* is_reload should be true only when called from read_nondefault_variables()
7125+
* or RestoreGUCState(), where we are trying to load some other process's
7126+
* GUC settings into a new process.
7127+
*
71247128
* Return value:
71257129
*+1: the value is valid and was successfully applied.
71267130
*0:the name or value is invalid (but see below).
@@ -10258,12 +10262,6 @@ read_nondefault_variables(void)
1025810262
GucSourcevarsource;
1025910263
GucContextvarscontext;
1026010264

10261-
/*
10262-
* Assert that PGC_BACKEND/PGC_SU_BACKEND case in set_config_option() will
10263-
* do the right thing.
10264-
*/
10265-
Assert(IsInitProcessingMode());
10266-
1026710265
/*
1026810266
* Open file
1026910267
*/
@@ -10317,30 +10315,43 @@ read_nondefault_variables(void)
1031710315

1031810316
/*
1031910317
* can_skip_gucvar:
10320-
* When serializing, determine whether to skip this GUC. When restoring, the
10321-
* negation of this test determines whether to restore the compiled-in default
10322-
* value before processing serialized values.
10318+
* Decide whether SerializeGUCState can skip sending this GUC variable,
10319+
* or whether RestoreGUCState can skip resetting this GUC to default.
1032310320
*
10324-
* A PGC_S_DEFAULT setting on the serialize side will typically match new
10325-
* postmaster children, but that can be false when got_SIGHUP == true and the
10326-
* pending configuration change modifies this setting. Nonetheless, we omit
10327-
* PGC_S_DEFAULT settings from serialization and make up for that by restoring
10328-
* defaults before applying serialized values.
10329-
*
10330-
* PGC_POSTMASTER variables always have the same value in every child of a
10331-
* particular postmaster. Most PGC_INTERNAL variables are compile-time
10332-
* constants; a few, like server_encoding and lc_ctype, are handled specially
10333-
* outside the serialize/restore procedure. Therefore, SerializeGUCState()
10334-
* never sends these, and RestoreGUCState() never changes them.
10335-
*
10336-
* Role is a special variable in the sense that its current value can be an
10337-
* invalid value and there are multiple ways by which that can happen (like
10338-
* after setting the role, someone drops it). So we handle it outside of
10339-
* serialize/restore machinery.
10321+
* It is somewhat magical and fragile that the same test works for both cases.
10322+
* Realize in particular that we are very likely selecting different sets of
10323+
* GUCs on the leader and worker sides! Be sure you've understood the
10324+
* comments here and in RestoreGUCState thoroughly before changing this.
1034010325
*/
1034110326
staticbool
1034210327
can_skip_gucvar(structconfig_generic*gconf)
1034310328
{
10329+
/*
10330+
* We can skip GUCs that are guaranteed to have the same values in leaders
10331+
* and workers. (Note it is critical that the leader and worker have the
10332+
* same idea of which GUCs fall into this category. It's okay to consider
10333+
* context and name for this purpose, since those are unchanging
10334+
* properties of a GUC.)
10335+
*
10336+
* PGC_POSTMASTER variables always have the same value in every child of a
10337+
* particular postmaster, so the worker will certainly have the right
10338+
* value already. Likewise, PGC_INTERNAL variables are set by special
10339+
* mechanisms (if indeed they aren't compile-time constants). So we may
10340+
* always skip these.
10341+
*
10342+
* Role must be handled specially because its current value can be an
10343+
* invalid value (for instance, if someone dropped the role since we set
10344+
* it). So if we tried to serialize it normally, we might get a failure.
10345+
* We skip it here, and use another mechanism to ensure the worker has the
10346+
* right value.
10347+
*
10348+
* For all other GUCs, we skip if the GUC has its compiled-in default
10349+
* value (i.e., source == PGC_S_DEFAULT). On the leader side, this means
10350+
* we don't send GUCs that have their default values, which typically
10351+
* saves lots of work. On the worker side, this means we don't need to
10352+
* reset the GUC to default because it already has that value. See
10353+
* comments in RestoreGUCState for more info.
10354+
*/
1034410355
returngconf->context==PGC_POSTMASTER||
1034510356
gconf->context==PGC_INTERNAL||gconf->source==PGC_S_DEFAULT||
1034610357
strcmp(gconf->name,"role")==0;
@@ -10358,6 +10369,7 @@ estimate_variable_size(struct config_generic *gconf)
1035810369
Sizesize;
1035910370
Sizevalsize=0;
1036010371

10372+
/* Skippable GUCs consume zero space. */
1036110373
if (can_skip_gucvar(gconf))
1036210374
return0;
1036310375

@@ -10522,6 +10534,7 @@ static void
1052210534
serialize_variable(char**destptr,Size*maxbytes,
1052310535
structconfig_generic*gconf)
1052410536
{
10537+
/* Ignore skippable GUCs. */
1052510538
if (can_skip_gucvar(gconf))
1052610539
return;
1052710540

@@ -10669,8 +10682,14 @@ guc_restore_error_context_callback(void *arg)
1066910682

1067010683
/*
1067110684
* RestoreGUCState:
10672-
* Reads the GUC state at the specified address and updates the GUCs with the
10673-
* values read from the GUC state.
10685+
* Reads the GUC state at the specified address and sets this process's
10686+
* GUCs to match.
10687+
*
10688+
* Note that this provides the worker with only a very shallow view of the
10689+
* leader's GUC state: we'll know about the currently active values, but not
10690+
* about stacked or reset values. That's fine since the worker is just
10691+
* executing one part of a query, within which the active values won't change
10692+
* and the stacked values are invisible.
1067410693
*/
1067510694
void
1067610695
RestoreGUCState(void*gucstate)
@@ -10687,10 +10706,100 @@ RestoreGUCState(void *gucstate)
1068710706
inti;
1068810707
ErrorContextCallbackerror_context_callback;
1068910708

10690-
/* See comment at can_skip_gucvar(). */
10709+
/*
10710+
* First, ensure that all potentially-shippable GUCs are reset to their
10711+
* default values. We must not touch those GUCs that the leader will
10712+
* never ship, while there is no need to touch those that are shippable
10713+
* but already have their default values. Thus, this ends up being the
10714+
* same test that SerializeGUCState uses, even though the sets of
10715+
* variables involved may well be different since the leader's set of
10716+
* variables-not-at-default-values can differ from the set that are
10717+
* not-default in this freshly started worker.
10718+
*
10719+
* Once we have set all the potentially-shippable GUCs to default values,
10720+
* restoring the GUCs that the leader sent (because they had non-default
10721+
* values over there) leads us to exactly the set of GUC values that the
10722+
* leader has. This is true even though the worker may have initially
10723+
* absorbed postgresql.conf settings that the leader hasn't yet seen, or
10724+
* ALTER USER/DATABASE SET settings that were established after the leader
10725+
* started.
10726+
*
10727+
* Note that ensuring all the potential target GUCs are at PGC_S_DEFAULT
10728+
* also ensures that set_config_option won't refuse to set them because of
10729+
* source-priority comparisons.
10730+
*/
1069110731
for (i=0;i<num_guc_variables;i++)
10692-
if (!can_skip_gucvar(guc_variables[i]))
10693-
InitializeOneGUCOption(guc_variables[i]);
10732+
{
10733+
structconfig_generic*gconf=guc_variables[i];
10734+
10735+
/* Do nothing if non-shippable or if already at PGC_S_DEFAULT. */
10736+
if (can_skip_gucvar(gconf))
10737+
continue;
10738+
10739+
/*
10740+
* We can use InitializeOneGUCOption to reset the GUC to default, but
10741+
* first we must free any existing subsidiary data to avoid leaking
10742+
* memory. The stack must be empty, but we have to clean up all other
10743+
* fields. Beware that there might be duplicate value or "extra"
10744+
* pointers.
10745+
*/
10746+
Assert(gconf->stack==NULL);
10747+
if (gconf->extra)
10748+
free(gconf->extra);
10749+
if (gconf->last_reported)/* probably can't happen */
10750+
free(gconf->last_reported);
10751+
if (gconf->sourcefile)
10752+
free(gconf->sourcefile);
10753+
switch (gconf->vartype)
10754+
{
10755+
casePGC_BOOL:
10756+
{
10757+
structconfig_bool*conf= (structconfig_bool*)gconf;
10758+
10759+
if (conf->reset_extra&&conf->reset_extra!=gconf->extra)
10760+
free(conf->reset_extra);
10761+
break;
10762+
}
10763+
casePGC_INT:
10764+
{
10765+
structconfig_int*conf= (structconfig_int*)gconf;
10766+
10767+
if (conf->reset_extra&&conf->reset_extra!=gconf->extra)
10768+
free(conf->reset_extra);
10769+
break;
10770+
}
10771+
casePGC_REAL:
10772+
{
10773+
structconfig_real*conf= (structconfig_real*)gconf;
10774+
10775+
if (conf->reset_extra&&conf->reset_extra!=gconf->extra)
10776+
free(conf->reset_extra);
10777+
break;
10778+
}
10779+
casePGC_STRING:
10780+
{
10781+
structconfig_string*conf= (structconfig_string*)gconf;
10782+
10783+
if (*conf->variable)
10784+
free(*conf->variable);
10785+
if (conf->reset_val&&conf->reset_val!=*conf->variable)
10786+
free(conf->reset_val);
10787+
if (conf->reset_extra&&conf->reset_extra!=gconf->extra)
10788+
free(conf->reset_extra);
10789+
break;
10790+
}
10791+
casePGC_ENUM:
10792+
{
10793+
structconfig_enum*conf= (structconfig_enum*)gconf;
10794+
10795+
if (conf->reset_extra&&conf->reset_extra!=gconf->extra)
10796+
free(conf->reset_extra);
10797+
break;
10798+
}
10799+
}
10800+
/* Now we can reset the struct to PGS_S_DEFAULT state. */
10801+
InitializeOneGUCOption(gconf);
10802+
}
1069410803

1069510804
/* First item is the length of the subsequent data */
1069610805
memcpy(&len,gucstate,sizeof(len));
@@ -10704,6 +10813,7 @@ RestoreGUCState(void *gucstate)
1070410813
error_context_callback.arg=NULL;
1070510814
error_context_stack=&error_context_callback;
1070610815

10816+
/* Restore all the listed GUCs. */
1070710817
while (srcptr<srcend)
1070810818
{
1070910819
intresult;

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp