public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Fujii Masao <[email protected]>
Cc: Japin Li <[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 14:17:34 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAHGQGwFQZoKd-d3WjODmEgemPqPmehGdRiTQx7pez2R9_UKCQg@mail.gmail.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>
> 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.
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.
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