Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: Allow GUC settings in CREATE SUBSCRIPTION CONNECTION to take
 effect
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAHGQGwFQZoKd-d3WjODmEgemPqPmehGdRiTQx7pez2R9_UKCQg@mail.gmail.com>
Date: Tue, 23 Dec 2025 14:17:34 +0800
Cc: Japin Li <japinli@hotmail.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <0C48758A-25F2-40BD-A75F-9E4C2B3C2996@gmail.com>
References: 
 <CAHGQGwGYV+-abbKwdrM2UHUe-JYOFWmsrs6=QicyJO-j+-Widw@mail.gmail.com>
 <CAA4eK1KR8-0DC-+bdSEzfw7oiYn=M6D3PpNjbaohwpFRRZ9y3A@mail.gmail.com>
 <CAHGQGwFPejWhp3SOA17aipbHjD3fZaj9sn6JJdWtKdWR-pT8HA@mail.gmail.com>
 <CAA4eK1JBg=k_4_0BX3p4usPs-a8-7FbPuPW8B2Rf1eUs5i_WLg@mail.gmail.com>
 <CAHGQGwEs08VO8SFFbp+4d=ktqh0K66rXWmqL4ea59iSn24ykLg@mail.gmail.com>
 <MEAPR01MB3031860450BC597D178643FBB6A9A@MEAPR01MB3031.ausprd01.prod.outlook.com>
 <CAHGQGwFQZoKd-d3WjODmEgemPqPmehGdRiTQx7pez2R9_UKCQg@mail.gmail.com>
To: Fujii Masao <masao.fujii@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/0C48758A-25F2-40BD-A75F-9E4C2B3C2996%40gmail.com>
Precedence: bulk


> On Dec 19, 2025, at 19:05, Fujii Masao <masao.fujii@gmail.com> wrote:
>=20
> On Fri, Dec 19, 2025 at 7:07=E2=80=AFPM Japin Li <japinli@hotmail.com> =
wrote:
>> Thanks for the patch =E2=80=94 that was my oversight.
>>=20
>> LGTM with one small suggestion:
>=20
> Thanks for the review!
>=20
>> The comment says: "If the option is not found in connInfo, return =
NULL value."
>>=20
>> Since the parameter is named `keyword`, I'd suggest: "If the keyword =
is not found in connInfo, return NULL."
>>=20
>> This keeps terminology consistent with the function signature.
>=20
> I think "the option with the given keyword" is more precise than just
> "the keyword".
> That said, simply using "the option" also seems sufficient in this =
context...
>=20
>=20
> Regarding 0002 patch, I found that it caused a CI failure, so I=E2=80=99=
ve updated
> the patch to fix that. The revised patch is attached.
>=20
> Regards,
>=20
> --=20
> Fujii Masao
> =
<v6-0001-PG15-PG16-Honor-GUC-settings-specified-in-CREATE-SUBSCRIPTI.txt><=
v6-0002-Add-TAP-test-for-GUC-settings-passed-via-CONNECTI.patch><v6-0001-H=
onor-GUC-settings-specified-in-CREATE-SUBSCRIPTI.patch>

A few more comments on v6:

1 - 0001
```
+			if (opt !=3D NULL)
+				pfree(opt);
```

=46rom what I learned from previous reviews, pfree() is safe to handle =
NULL, we can omit the NULL check, which makes the code simpler. This =
comment applies to multiple places.

2. I still think we should verify options extracted from conninfo. In =
the 0002=E2=80=99s tap script, if I make the following change:
```
 $node_subscriber->safe_psql('postgres',
-       "ALTER SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr =
options=3D''-c log_statement_stats=3Don'''"
+       "ALTER SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr =
options=3D''-c log_statement_stats=3Don-c wal_sender_timeout=3D1000'=E2=80=
=99'"
```

Notice, there is no space between the second =E2=80=9C-c=E2=80=9D and =
=E2=80=9Con=E2=80=9D, meaning that a user passes an invalid options. =
Then the test will get stuck forever, which would never happen before =
this patch.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/