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

On Tue, 23 Dec 2025 at 14:17, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>> On Dec 19, 2025, at 19:05, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>
>> On Fri, Dec 19, 2025 at 7:07 PM Japin Li <japinli(at)hotmail(dot)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.

> 2. I still think we should verify options extracted from conninfo. In the 0002’s tap script, if I make the following change:
> ```
> $node_subscriber->safe_psql('postgres',
> - "ALTER SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr options=''-c log_statement_stats=on'''"
> + "ALTER SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr options=''-c log_statement_stats=on-c wal_sender_timeout=1000'’'"
> ```
>
> Notice, there is no space between the second “-c” and “on”, meaning that a user passes an invalid options. Then the test will get stuck forever, which would never happen before this patch.
>

How to verify the options? Checking for -c isn't enough because valid flags
can still carry invalid values. For example, -c log_statement_stats=aa is
syntactically correct but will fail at runtime.

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.