Re: Allow GUC settings in CREATE SUBSCRIPTION CONNECTION to take effect

> On Dec 23, 2025, at 16:13, Japin Li <japinli@hotmail.com> wrote:
>
> 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:
>>>
>>> On Fri, Dec 19, 2025 at 7:07 PM Japin Li <japinli@hotmail.com> wrote:
>>>> Thanks for the patch — that was my oversight.
>>>>
>>>> LGTM with one small suggestion:
>>>
>>> Thanks for the review!
>>>
>>>> The comment says: "If the option is not found in connInfo, return NULL value."
>>>>
>>>> Since the parameter is named `keyword`, I'd suggest: "If the keyword is not found in connInfo, return NULL."
>>>>
>>>> This keeps terminology consistent with the function signature.
>>>
>>> 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...
>>>
>>>
>>> Regarding 0002 patch, I found that it caused a CI failure, so I’ve updated
>>> the patch to fix that. The revised patch is attached.
>>>
>>> Regards,
>>>
>>> --
>>> 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-Honor-GUC-settings-specified-in-CREATE-SUBSCRIPTI.patch>
>>
>> A few more comments on v6:
>>
>> 1 - 0001
>> ```
>> + if (opt != NULL)
>> + pfree(opt);
>> ```
>>
>> From what I learned from previous reviews, pfree() is safe to handle NULL, we can omit the NULL check, which makes
thecode simpler. This comment applies to multiple places. 
>>
>
> 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.
>

Ahh… You are right. I just traced a backend pfree(), and it ends up calling GetMemoryChunkMethodID(const void *point),
andthe function doesn’t handle NULL pointer. 

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