Horton

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

Chao Li <li.evan.chao@gmail.com>

View thread
From: Chao Li <li.evan.chao@gmail.com>
To: Fujii Masao <masao.fujii@gmail.com>
Cc: Japin Li <japinli@hotmail.com>, Amit Kapila <amit.kapila16@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2025-12-23T06:17:34Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add TAP test for GUC settings passed via CONNECTION in logical replication.

  2. Honor GUC settings specified in CREATE SUBSCRIPTION CONNECTION.

  3. Ensure consistent logical replication of datetime and float8 values.


> On Dec 19, 2025, at 19:05, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
> On Fri, Dec 19, 2025 at 7:07 PM Japin Li <japinli@hotmail.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.

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/