public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Japin Li <[email protected]>
Cc: Fujii Masao <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Allow GUC settings in CREATE SUBSCRIPTION CONNECTION to take effect
Date: Tue, 23 Dec 2025 16:45:56 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <SYAPR01MB3038EFC51A8FD90FACAE203AB6B5A@SYAPR01MB3038.ausprd01.prod.outlook.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>
<[email protected]>
<SYAPR01MB3038EFC51A8FD90FACAE203AB6B5A@SYAPR01MB3038.ausprd01.prod.outlook.com>
> 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/
view thread (37+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Allow GUC settings in CREATE SUBSCRIPTION CONNECTION to take effect
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox