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 the code 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), and the function doesn’t handle NULL pointer.

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