> 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/
