Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions

Ian Barwick <ian.barwick@2ndquadrant.com> writes:
> On 8/3/19 7:27 AM, Tom Lane wrote:
>> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>>> The main issue however is that no code was written yet.

>> Seems like it ought to be relatively simple ... but I didn't look.

> The patch I originally sent does exactly this.

Ah, you did send a patch, but that tries to maintain the existing behavior
of replacing the last occurrence in-place.  I think it's simpler and more
sensible to just make a sweep to delete all matches, and then append the
new setting (if any) at the end, as attached.

A more aggressive patch would try to de-duplicate the entire list, not
just the current target entry ... but I'm not really excited about doing
that in a back-patchable bug fix.

I looked at the TAP test you proposed and couldn't quite convince myself
that it was worth the trouble.  A new test within an existing suite
would likely be fine, but a whole new src/test/ subdirectory just for
pg.auto.conf seems a bit much.  (Note that the buildfarm and possibly
the MSVC scripts would have to be taught about each such subdirectory.)
At the same time, we lack any better place to put such a test :-(.
Maybe it's time for a "miscellaneous TAP tests" subdirectory?

            regards, tom lane

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fc46360..12abbb2 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7850,40 +7850,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)
     {
+        next = item->next;
         if (strcmp(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;