From: Tom Lane <tgl@sss.pgh.pa.us>
To: Ian Barwick <ian.barwick@2ndquadrant.com>
cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>,
        Amit Kapila <amit.kapila16@gmail.com>,
        Stephen Frost <sfrost@snowman.net>,
        Robert Haas <robertmhaas@gmail.com>,
        "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions
In-reply-to: <c87c6599-8f60-754f-620d-c30faa0dceea@2ndquadrant.com>
References: 
 <CA+TgmobVjrvVQB_gVytgBcPRmBvptN3wHQy5T6=dYCDZ5n5MGA@mail.gmail.com>
 <20372.1561134638@sss.pgh.pa.us> <20190621164028.GO2480@tamriel.snowman.net>
 <21353.1561136112@sss.pgh.pa.us>
 <CA+TgmoZVx-vco-39EN9MxO9BaG-oVY+NuDeZPGdS=_bX5VH70w@mail.gmail.com>
 <20190624185647.GW2480@tamriel.snowman.net> <15032.1561403529@sss.pgh.pa.us>
 <20190624192014.GB2480@tamriel.snowman.net>
 <56aff4f3-dec8-f462-7feb-0938411b04a6@2ndquadrant.com>
 <CAA4eK1+JMm=tvx720y6jhD8pdzt=HdS1-TwQwkARib2pwK8EvQ@mail.gmail.com>
 <20190802222254.qsvqjtkwte464lu3@development>
 <31697.1564784856@sss.pgh.pa.us>
 <c87c6599-8f60-754f-620d-c30faa0dceea@2ndquadrant.com>
Comments: In-reply-to Ian Barwick <ian.barwick@2ndquadrant.com>
	message dated "Sat, 03 Aug 2019 07:39:27 +0900"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <16080.1564859244.0@sss.pgh.pa.us>
Date: Sat, 03 Aug 2019 15:13:48 -0400
Message-ID: <16337.1564859628@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <16080.1564859244.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="alter-system-remove-dup-entries-1.patch";
	charset="us-ascii"
Content-ID: <16080.1564859244.2@sss.pgh.pa.us>
Content-Description: alter-system-remove-dup-entries-1.patch
Content-Transfer-Encoding: quoted-printable

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 =3D NULL;
 =

-	/* Search the list for an existing match (we assume there's only one) */
-	for (item =3D *head_p; item !=3D NULL; item =3D item->next)
+	/*
+	 * Remove any existing match(es) for "name".  Normally there'd be at mos=
t
+	 * one, but if external tools have modified the config file, there could
+	 * be more.
+	 */
+	for (item =3D *head_p; item !=3D NULL; item =3D next)
 	{
+		next =3D item->next;
 		if (strcmp(item->name, name) =3D=3D 0)
 		{
-			/* found a match, replace it */
-			pfree(item->value);
-			if (value !=3D NULL)
-			{
-				/* update the parameter value */
-				item->value =3D pstrdup(value);
-			}
+			/* found a match, delete it */
+			if (prev)
+				prev->next =3D next;
 			else
-			{
-				/* delete the configuration parameter from list */
-				if (*head_p =3D=3D item)
-					*head_p =3D item->next;
-				else
-					prev->next =3D item->next;
-				if (*tail_p =3D=3D item)
-					*tail_p =3D prev;
+				*head_p =3D next;
+			if (next =3D=3D NULL)
+				*tail_p =3D prev;
 =

-				pfree(item->name);
-				pfree(item->filename);
-				pfree(item);
-			}
-			return;
+			pfree(item->value);
+			pfree(item->name);
+			pfree(item->filename);
+			pfree(item);
 		}
-		prev =3D item;
+		else
+			prev =3D item;
 	}
 =

-	/* Not there; no work if we're trying to delete it */
+	/* Done if we're trying to delete it */
 	if (value =3D=3D NULL)
 		return;
 =


------- =_aaaaaaaaaa0--