On 8/6/19 11:16 AM, Stephen Frost wrote:
> Greetings,
>
> * Ian Barwick ([email protected]) wrote:
>> On 8/6/19 9:52 AM, Stephen Frost wrote:> Greetings,
>>> * Tom Lane ([email protected]) wrote:
>>>>
>>>> + <para>
>>>> + External tools might also
>>>> + modify <filename>postgresql.auto.conf</filename>, typically by
appending
>>>> + new settings to the end. It is not recommended to do this while the
>>>> + server is running, since a concurrent <command>ALTER SYSTEM</command>
>>>> + command could overwrite such changes.
>>>> </para>
>>>
>>> Alternatively, or maybe also here, we could say "note that appending to
>>> the file as a mechanism for setting a new value by an external tool is
>>> acceptable even though it will cause duplicates- PostgreSQL will always
>>> use the last value set and other tools should as well. Duplicates and
>>> comments may be removed when rewriting the file
>>
>> FWIW, as the file is rewritten each time, *all* comments are removed
>> anyway (the first two comment lines in the file with the warning
>> are added when the new version of the file is written().
>
> Whoah- the file is *not* rewritten each time. It's only rewritten each
> time by *ALTER SYSTEM*, but that it not the only thing that's modifying
> the file. That mistaken assumption is part of what got us into this
> mess...
Ah, got it, I thought you were talking about the ALTER SYSTEM behaviour.
>>> and parameters may be
>>> lower-cased." (istr that last bit being true too but I haven't checked
>>> lately).
>>
>> Ho-hum, they won't be lower-cased, instead currently they just won't be
>> overwritten if they're present in pg.auto.conf, which is a slight
>> eccentricity, but not actually a problem with the current code as the new
value
>> will be written last. E.g.:
>>
>> $ cat postgresql.auto.conf
>> # Do not edit this file manually!
>> # It will be overwritten by the ALTER SYSTEM command.
>> DEFAULT_TABLESPACE = 'space_1'
>>
>> postgres=# ALTER SYSTEM SET default_tablespace ='pg_default';
>> ALTER SYSTEM
>>
>> $ cat postgresql.auto.conf
>> # Do not edit this file manually!
>> # It will be overwritten by the ALTER SYSTEM command.
>> DEFAULT_TABLESPACE = 'space_1'
>> default_tablespace = 'pg_default'
>>
>> I don't think that's worth worrying about now.
>
> Erm, those are duplicates though and we're saying that ALTER SYSTEM
> removes those... Seems like we should be normalizing the file to be
> consistent in this regard too.
True. (Switches brain on)... Ah yes, with the patch previously provided
by Tom, it seems to be just a case of replacing "strcmp" with "guc_name_compare"
to match the existing string; the name will be rewritten with the value provided
to ALTER SYSTEM, which will be normalized to lower case anyway.
Tweaked version attached.
>> My suggestion for the paragaph in question:
>>
>> <para>
>> External tools which need to write configuration settings (e.g. for
replication)
>> where it's essential to ensure these are read last (to override
versions
>> of these settings present in other configuration files), may append
settings to
>> <filename>postgresql.auto.conf</filename>. It is not recommended to do
this while
>> the server is running, since a concurrent <command>ALTER
SYSTEM</command>
>> command could overwrite such changes. Note that a subsequent
>> <command>ALTER SYSTEM</command> will cause
<filename>postgresql.auto.conf</filename>,
>> to be rewritten, removing any duplicate versions of the setting
altered, and also
>> any comment lines present.
>> </para>
>
> I dislike the special-casing of ALTER SYSTEM here, where we're basically
> saying that only ALTER SYSTEM is allowed to do this cleanup and that if
> such cleanup is wanted then ALTER SYSTEM must be run.
This is just saying what ALTER SYSTEM will do, which IMHO we should describe
somewhere. Initially when I stated working with pg.auto.conf I had
my application append a comment line to show where the entries came from,
but not having any idea how pg.auto.conf was modified at that point, I was
wondering why the comment subsequently disappeared. Perusing the source code has
explained that for me, but would be mighty useful to document that.
> What I was trying to get at is a definition of what transformations are
> allowed and to make it clear that anything using/modifying the file
> needs to be prepared for and work with those transformations. I don't
> think we want people assuming that if they don't run ALTER SYSTEM then
> they can depend on duplicates being preserved and such..
OK, then we should be saying something like:
- pg.auto.conf may be rewritten at any point and duplicates/comments removed
- the rewrite will occur whenever ALTER SYSTEM is run, removing duplicates
of the parameter being modified and any comments
- external utilities may also rewrite it; they may, if they wish, remove
duplicates and comments
> and, yes, I
> know that's a stretch, but if we ever want anything other than ALTER
> SYSTEM to be able to make such changes (and I feel pretty confident that
> we will...) then we shouldn't document things specifically about when
> that command runs.
But at this point ALTER SYSTEM is the only thing which is known to modify
the file, with known effects. If in a future release something else is
added, the documentation can be updated as appropriate.
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 3051c9cf8e..0093e9096a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7281,40 +7281,37 @@ replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p,
const char *name, const char *value)
{
ConfigVariable *item,
+ *next,
*prev = NULL;
- /* Search the list for an existing match (we assume there's only one) */
- for (item = *head_p; item != NULL; item = item->next)
+ /*
+ * Remove any existing match(es) for "name". Normally there'd be at most
+ * one, but if external tools have modified the config file, there could
+ * be more.
+ */
+ for (item = *head_p; item != NULL; item = next)
{
- if (strcmp(item->name, name) == 0)
+ next = item->next;
+ if (guc_name_compare(item->name, name) == 0)
{
- /* found a match, replace it */
- pfree(item->value);
- if (value != NULL)
- {
- /* update the parameter value */
- item->value = pstrdup(value);
- }
+ /* found a match, delete it */
+ if (prev)
+ prev->next = next;
else
- {
- /* delete the configuration parameter from list */
- if (*head_p == item)
- *head_p = item->next;
- else
- prev->next = item->next;
- if (*tail_p == item)
- *tail_p = prev;
+ *head_p = next;
+ if (next == NULL)
+ *tail_p = prev;
- pfree(item->name);
- pfree(item->filename);
- pfree(item);
- }
- return;
+ pfree(item->value);
+ pfree(item->name);
+ pfree(item->filename);
+ pfree(item);
}
- prev = item;
+ else
+ prev = item;
}
- /* Not there; no work if we're trying to delete it */
+ /* Done if we're trying to delete it */
if (value == NULL)
return;
