> On Dec 23, 2025, at 16:13, Japin Li <[email protected]> wrote:
>
> On Tue, 23 Dec 2025 at 14:17, Chao Li <[email protected]> wrote:
>>> On Dec 19, 2025, at 19:05, Fujii Masao <[email protected]> wrote:
>>>
>>> On Fri, Dec 19, 2025 at 7:07 PM Japin Li <[email protected]> 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/
