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: 
 <SYAPR01MB3038EFC51A8FD90FACAE203AB6B5A@SYAPR01MB3038.ausprd01.prod.outlook.com>
Date: Tue, 23 Dec 2025 16:45:56 +0800
Cc: Fujii Masao <masao.fujii@gmail.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <35BC6A31-9816-4DC7-AE90-8D4C073C20D6@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>
 <0C48758A-25F2-40BD-A75F-9E4C2B3C2996@gmail.com>
 <SYAPR01MB3038EFC51A8FD90FACAE203AB6B5A@SYAPR01MB3038.ausprd01.prod.outlook.com>
To: Japin Li <japinli@hotmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/35BC6A31-9816-4DC7-AE90-8D4C073C20D6%40gmail.com>
Precedence: bulk


> On Dec 23, 2025, at 16:13, Japin Li <japinli@hotmail.com> wrote:
>=20
> On Tue, 23 Dec 2025 at 14:17, Chao Li <li.evan.chao@gmail.com> wrote:
>>> 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>
>>=20
>> A few more comments on v6:
>>=20
>> 1 - 0001
>> ```
>> + if (opt !=3D NULL)
>> + pfree(opt);
>> ```
>>=20
>> =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.
>>=20
>=20
> I think we cannot omit it here. We have two pfree()s, one for frontend =
and
> one for backend. IIUC, the frontend pfree() is safe to handle NULL, =
but the
> backend pfree() is not.
>=20

Ahh=E2=80=A6 You are right. I just traced a backend pfree(), and it ends =
up calling GetMemoryChunkMethodID(const void *point), and the function =
doesn=E2=80=99t handle NULL pointer.

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