Horton

Thread

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.

  1. Allow GUC settings in CREATE SUBSCRIPTION CONNECTION to take effect

    Fujii Masao <masao.fujii@gmail.com> — 2025-11-18T15:59:00Z

    Hi,

In logical replication, any GUC settings specified in the CONNECTION clause of
CREATE SUBSCRIPTION are currently ignored. For example:

        CREATE SUBSCRIPTION mysub
            CONNECTION 'options=''-c wal_sender_timeout=1000'''
            PUBLICATION ...

The wal_sender_timeout value here has no effect.

This is inconvenient when different logical replication walsenders need
different settings - e.g., a small wal_sender_timeout for walsender
connecting to a nearby subscriber and a larger one for walsender
connecting to a distant subscriber. Right now, it's not easy for users
to control such per-connection behavior.

The reason of thid limitation is that libpqrcv_connect() always overwrites
the options connection parameter as follows:

        keys[++i] = "options";
        vals[i] = "-c datestyle=ISO -c intervalstyle=postgres -c
extra_float_digits=3";

This wipes out any user-specified GUCs in the CONNECTION string.
Physical replication does not have this problem because it does not overwrite
options, so GUCs in primary_conninfo are honored.

To remove this restriction, how about switching to issuing SET commands for
datestyle, intervalstyle, and extra_float_digits after the connection
is established,
similar to what postgres_fdw does, instead of forcing them into options?
That would allow user-specified GUC settings in CREATE SUBSCRIPTION to
take effect.

This overwrite behavior was introduced in commit f3d4019da5d and chosen mainly
to avoid extra network round trips according to the discussion [1].
While SET commands would add a round trip, it only happens at
connection startup,
which is infrequent - so the overhead seems negligible.

Thoughts?

Regards,

[1] https://postgr.es/m/CAFF0-CF=D7pc6st-3A9f1JnOt0qmc+BcBPVzD6fLYisKyAjkGA@mail.gmail.com

-- 
Fujii Masao

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-11-19T06:22:28Z

    On Wed, Nov 19, 2025 at 12:59 AM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> Hi,
>
> In logical replication, any GUC settings specified in the CONNECTION clause of
> CREATE SUBSCRIPTION are currently ignored. For example:
>
>         CREATE SUBSCRIPTION mysub
>             CONNECTION 'options=''-c wal_sender_timeout=1000'''
>             PUBLICATION ...
>
> The wal_sender_timeout value here has no effect.
>
> This is inconvenient when different logical replication walsenders need
> different settings - e.g., a small wal_sender_timeout for walsender
> connecting to a nearby subscriber and a larger one for walsender
> connecting to a distant subscriber. Right now, it's not easy for users
> to control such per-connection behavior.
>
> The reason of thid limitation is that libpqrcv_connect() always overwrites
> the options connection parameter as follows:
>
>         keys[++i] = "options";
>         vals[i] = "-c datestyle=ISO -c intervalstyle=postgres -c
> extra_float_digits=3";
>
> This wipes out any user-specified GUCs in the CONNECTION string.
> Physical replication does not have this problem because it does not overwrite
> options, so GUCs in primary_conninfo are honored.
>
> To remove this restriction, how about switching to issuing SET commands for
> datestyle, intervalstyle, and extra_float_digits after the connection
> is established,
> similar to what postgres_fdw does, instead of forcing them into options?
> That would allow user-specified GUC settings in CREATE SUBSCRIPTION to
> take effect.
>
> This overwrite behavior was introduced in commit f3d4019da5d and chosen mainly
> to avoid extra network round trips according to the discussion [1].
> While SET commands would add a round trip, it only happens at
> connection startup,
> which is infrequent - so the overhead seems negligible.
>
> Thoughts?

Attached is a patch implementing this idea.
I've also added it to the next CommitFest.

Regards,

-- 
Fujii Masao

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-11-19T10:38:08Z

    On Wed, Nov 19, 2025 at 3:22 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> Attached is a patch implementing this idea.
> I've also added it to the next CommitFest.

I've fixed the compiler warning issue.
Attached is an updated version of the patch.

Regards,

-- 
Fujii Masao

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

    Chao Li <li.evan.chao@gmail.com> — 2025-11-20T06:53:49Z

    
> On Nov 18, 2025, at 23:59, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
> Hi,
> 
> In logical replication, any GUC settings specified in the CONNECTION clause of
> CREATE SUBSCRIPTION are currently ignored. For example:
> 
>        CREATE SUBSCRIPTION mysub
>            CONNECTION 'options=''-c wal_sender_timeout=1000'''
>            PUBLICATION ...
> 
> The wal_sender_timeout value here has no effect.
> 
> This is inconvenient when different logical replication walsenders need
> different settings - e.g., a small wal_sender_timeout for walsender
> connecting to a nearby subscriber and a larger one for walsender
> connecting to a distant subscriber. Right now, it's not easy for users
> to control such per-connection behavior.
> 
> The reason of thid limitation is that libpqrcv_connect() always overwrites
> the options connection parameter as follows:
> 
>        keys[++i] = "options";
>        vals[i] = "-c datestyle=ISO -c intervalstyle=postgres -c
> extra_float_digits=3";
> 
> This wipes out any user-specified GUCs in the CONNECTION string.
> Physical replication does not have this problem because it does not overwrite
> options, so GUCs in primary_conninfo are honored.
> 
> To remove this restriction, how about switching to issuing SET commands for
> datestyle, intervalstyle, and extra_float_digits after the connection
> is established,
> similar to what postgres_fdw does, instead of forcing them into options?
> That would allow user-specified GUC settings in CREATE SUBSCRIPTION to
> take effect.
> 
> This overwrite behavior was introduced in commit f3d4019da5d and chosen mainly
> to avoid extra network round trips according to the discussion [1].
> While SET commands would add a round trip, it only happens at
> connection startup,
> which is infrequent - so the overhead seems negligible.
> 
> Thoughts?

Before this patch, all user specified options are silently discarded, now all user specified options expect the 3 will be kept, will that expose a hold where user badly specifies some option that may break logical replication? If that’s true, then we need to parse user specified options and do some verifications.

I just reviewed v2, and got some comments:

1.
```
+			char		sql[100];
```

Hardcode 100 here doesn’t look good. If you decide to keep, I won’t have a strong objection.

2
```
+		const char *params[] =
+		{"datestyle", "intervalstyle", "extra_float_digits", NULL};
+		const char *values[] = {"ISO", "postgres", "3", NULL};
```

Nit: we don’t need to have a NULL terminator element. We can use lengthof() macro to get array length. lengthof() is defined in c.h.

3. To minimize the network round-trip, maybe we can combine the 3 set into a single statement?

4. The commit message:
```
This commit removes the restriction by changing how logical replication
connections are established so that GUC settings in the CONNECTION string
are properly passed through to and uesd by the walsender. This enables
```

This is a little bit inaccurate, all user specified settings expected the 3 ones being overwritten will be honored.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-11-21T07:47:05Z

    On Thu, Nov 20, 2025 at 3:54 PM Chao Li <li.evan.chao@gmail.com> wrote:
> Before this patch, all user specified options are silently discarded,

The GUC settings in CREATE SUBSCRIPTION were honored up through v14;
the behavior changed in commit f3d4019da5d, so some might view this
as a regression.


> now all user specified options expect the 3 will be kept, will that expose a hold where user badly specifies some option that may break logical replication? If that’s true, then we need to parse user specified options and do some verifications.
>

Yeah, I agree that if certain GUCs can break logical replication,
we should enforce "safe" values, just as we currently do for datestyle.
And if any other GUCs can cause the issue, they could affect
postgres_fdw etc, so the fix would need to be broader.


> I just reviewed v2, and got some comments:

Thanks for the review!


>
> 1.
> ```
> +                       char            sql[100];
> ```
>
> Hardcode 100 here doesn’t look good. If you decide to keep, I won’t have a strong objection.

I think hardcoding 100 here is sufficient, since the queries built on
that buffer are fixed and clearly fit within that limit.


>
> 2
> ```
> +               const char *params[] =
> +               {"datestyle", "intervalstyle", "extra_float_digits", NULL};
> +               const char *values[] = {"ISO", "postgres", "3", NULL};
> ```
>
> Nit: we don’t need to have a NULL terminator element. We can use lengthof() macro to get array length. lengthof() is defined in c.h.

Okay, I'll adjust the patch accordingly.


>
> 3. To minimize the network round-trip, maybe we can combine the 3 set into a single statement?

As for the extra network round trip, I still doubt it will matter
in practice given that it happens only at replication connection startup.


>
> 4. The commit message:
> ```
> This commit removes the restriction by changing how logical replication
> connections are established so that GUC settings in the CONNECTION string
> are properly passed through to and uesd by the walsender. This enables
> ```
>
> This is a little bit inaccurate, all user specified settings expected the 3 ones being overwritten will be honored.

Are you suggesting that, because datestyle and the other two parameters
specified in CONNECTION aren(t actually applied by the walsender,
the commit message should explicitly mention that not all parameters
from CONNECTION are used?

Regards,

-- 
Fujii Masao

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

    Chao Li <li.evan.chao@gmail.com> — 2025-11-21T09:23:10Z

    
> On Nov 21, 2025, at 15:47, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
>> 
>> 4. The commit message:
>> ```
>> This commit removes the restriction by changing how logical replication
>> connections are established so that GUC settings in the CONNECTION string
>> are properly passed through to and uesd by the walsender. This enables
>> ```
>> 
>> This is a little bit inaccurate, all user specified settings expected the 3 ones being overwritten will be honored.
> 
> Are you suggesting that, because datestyle and the other two parameters
> specified in CONNECTION aren(t actually applied by the walsender,
> the commit message should explicitly mention that not all parameters
> from CONNECTION are used?

No, what I was thinking is that, we could combine the three set statement into one, like:

```
Set a = 1; set b = 2; set c = 3;
```
So that sends a single statement to publisher server, that reduces round-trip from 3 times to one time.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-11-21T16:14:04Z

    On Fri, Nov 21, 2025 at 6:24 PM Chao Li <li.evan.chao@gmail.com> wrote:
> No, what I was thinking is that, we could combine the three set statement into one, like:
>
> ```
> Set a = 1; set b = 2; set c = 3;
> ```
> So that sends a single statement to publisher server, that reduces round-trip from 3 times to one time.

I see the point about combining the three SET commands to reduce round trips,
but I think the current approach in the patch (i.e., issuing a separate
SET command for each parameter) is sufficient. I still don't think
the additional round trip during replication connection startup is
a real concern. This approach is also consistent with what postgres_fdw
and pg_dump already do.

Regards,

-- 
Fujii Masao

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

    Chao Li <li.evan.chao@gmail.com> — 2025-11-22T01:31:06Z

    
> On Nov 22, 2025, at 00:14, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
> On Fri, Nov 21, 2025 at 6:24 PM Chao Li <li.evan.chao@gmail.com> wrote:
>> No, what I was thinking is that, we could combine the three set statement into one, like:
>> 
>> ```
>> Set a = 1; set b = 2; set c = 3;
>> ```
>> So that sends a single statement to publisher server, that reduces round-trip from 3 times to one time.
> 
> I see the point about combining the three SET commands to reduce round trips,
> but I think the current approach in the patch (i.e., issuing a separate
> SET command for each parameter) is sufficient. I still don't think
> the additional round trip during replication connection startup is
> a real concern. This approach is also consistent with what postgres_fdw
> and pg_dump already do.
> 

No problem. I don’t have a strong option here. I just saw you mentioned overhead of round trip and thought to improve. But I agree that overhead is tiny, not a real concern.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-11-22T14:14:27Z

    On Sat, Nov 22, 2025 at 10:31 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Nov 22, 2025, at 00:14, Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > On Fri, Nov 21, 2025 at 6:24 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >> No, what I was thinking is that, we could combine the three set statement into one, like:
> >>
> >> ```
> >> Set a = 1; set b = 2; set c = 3;
> >> ```
> >> So that sends a single statement to publisher server, that reduces round-trip from 3 times to one time.
> >
> > I see the point about combining the three SET commands to reduce round trips,
> > but I think the current approach in the patch (i.e., issuing a separate
> > SET command for each parameter) is sufficient. I still don't think
> > the additional round trip during replication connection startup is
> > a real concern. This approach is also consistent with what postgres_fdw
> > and pg_dump already do.
> >
>
> No problem. I don’t have a strong option here. I just saw you mentioned overhead of round trip and thought to improve. But I agree that overhead is tiny, not a real concern.

Okay, thanks!

I've updated the patch to use lengthof() as you suggested.
The revised version is attached.

Regards,

-- 
Fujii Masao

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

    Chao Li <li.evan.chao@gmail.com> — 2025-11-24T03:51:58Z

    
> On Nov 22, 2025, at 22:14, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
> On Sat, Nov 22, 2025 at 10:31 AM Chao Li <li.evan.chao@gmail.com> wrote:
>> 
>> 
>> 
>>> On Nov 22, 2025, at 00:14, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> 
>>> On Fri, Nov 21, 2025 at 6:24 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>>> No, what I was thinking is that, we could combine the three set statement into one, like:
>>>> 
>>>> ```
>>>> Set a = 1; set b = 2; set c = 3;
>>>> ```
>>>> So that sends a single statement to publisher server, that reduces round-trip from 3 times to one time.
>>> 
>>> I see the point about combining the three SET commands to reduce round trips,
>>> but I think the current approach in the patch (i.e., issuing a separate
>>> SET command for each parameter) is sufficient. I still don't think
>>> the additional round trip during replication connection startup is
>>> a real concern. This approach is also consistent with what postgres_fdw
>>> and pg_dump already do.
>>> 
>> 
>> No problem. I don’t have a strong option here. I just saw you mentioned overhead of round trip and thought to improve. But I agree that overhead is tiny, not a real concern.
> 
> Okay, thanks!
> 
> I've updated the patch to use lengthof() as you suggested.
> The revised version is attached.
> 
> Regards,
> 
> -- 
> Fujii Masao
> <v3-0001-Honor-GUC-settings-specified-in-CREATE-SUBSCRIPTI.patch>

V3 looks good to me.


>> now all user specified options expect the 3 will be kept, will that expose a hold where user badly specifies some option that may break logical replication? If that’s true, then we need to parse user specified options and do some verifications.
>> 
> 
> Yeah, I agree that if certain GUCs can break logical replication,
> we should enforce "safe" values, just as we currently do for datestyle.
> And if any other GUCs can cause the issue, they could affect
> postgres_fdw etc, so the fix would need to be broader.

Just want to clarify if you mean you will handle this in a future patch?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-11-24T05:54:54Z

    On Fri, Nov 21, 2025, 00:47 Fujii Masao <masao.fujii@gmail.com> wrote:

> On Thu, Nov 20, 2025 at 3:54 PM Chao Li <li.evan.chao@gmail.com> wrote:
> > Before this patch, all user specified options are silently discarded,
>
> The GUC settings in CREATE SUBSCRIPTION were honored up through v14;
> the behavior changed in commit f3d4019da5d, so some might view this
> as a regression.
>

FWIW, I definitely view it as a regression. I used this in citus to make
the logical replication sender of the shard rebalancer use a higher CPU
priority[1]. I had no clue, until now, that that logic got completely
broken in PG15 (which we coincidentally added support for in the same
release).

I'm not entirely sure if it's worth a backpatch. This citus feature
probably isn't the most critical. So if that's the only usecase in the wild
that got broken, then that might be fine. But I at least wanted to share
that others (i.e. me) have used this feature.

[1]:
https://github.com/citusdata/citus/blame/662b7248db2146d33a1a21227271b839355a970a/src/backend/distributed/replication/multi_logical_replication.c#L1510

>

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-11-25T11:30:35Z

    On Mon, Nov 24, 2025 at 12:52 PM Chao Li <li.evan.chao@gmail.com> wrote:
> V3 looks good to me.

Thanks for reviewing the patch!

> > Yeah, I agree that if certain GUCs can break logical replication,
> > we should enforce "safe" values, just as we currently do for datestyle.
> > And if any other GUCs can cause the issue, they could affect
> > postgres_fdw etc, so the fix would need to be broader.
>
> Just want to clarify if you mean you will handle this in a future patch?

I don't currently know of any other parameters that must be forced for
logical replication or postgres_fdw. But if we identify any, I'm happy to
review a patch that adds the necessary handling.

Regards,

-- 
Fujii Masao

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-11-25T11:32:51Z

    On Mon, Nov 24, 2025 at 2:55 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> On Fri, Nov 21, 2025, 00:47 Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> On Thu, Nov 20, 2025 at 3:54 PM Chao Li <li.evan.chao@gmail.com> wrote:
>> > Before this patch, all user specified options are silently discarded,
>>
>> The GUC settings in CREATE SUBSCRIPTION were honored up through v14;
>> the behavior changed in commit f3d4019da5d, so some might view this
>> as a regression.
>
>
> FWIW, I definitely view it as a regression. I used this in citus to make the logical replication sender of the shard rebalancer use a higher CPU priority[1]. I had no clue, until now, that that logic got completely broken in PG15 (which we coincidentally added support for in the same release).

Thanks for sharing this!


> I'm not entirely sure if it's worth a backpatch. This citus feature probably isn't the most critical. So if that's the only usecase in the wild that got broken, then that might be fine. But I at least wanted to share that others (i.e. me) have used this feature.

I found the following description in logical replication docs, which makes me
start thinking that the patch would need to be backpatched.

-----------------
If the role does not trust all table owners, include options=-crow_security=off
in the connection string
https://www.postgresql.org/docs/devel/logical-replication-security.html
-----------------

Regards,

-- 
Fujii Masao

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-11-26T16:33:25Z

    On Tue, Nov 25, 2025 at 8:32 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> I found the following description in logical replication docs, which makes me
> start thinking that the patch would need to be backpatched.

I've prepared patches for the older branches as well and attached them.

Regards,

-- 
Fujii Masao

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

    Kirill Reshke <reshkekirill@gmail.com> — 2025-11-26T17:37:39Z

    On Sat, 22 Nov 2025 at 17:14, Fujii Masao <masao.fujii@gmail.com> wrote:
>
>
> Okay, thanks!
>
> I've updated the patch to use lengthof() as you suggested.
> The revised version is attached.
>
> Regards,
>
> --
> Fujii Masao

Looking at v3 raises two questions for me.

First is if we should have a doc notion of which variables ought to be
set to what.
Second, how do we actually test that subscription connection options
are applied on the subscriber side? Can we have TAP for this  (is is
worth the troubles)?

-- 
Best regards,
Kirill Reshke

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-11-27T02:46:10Z

    On Thu, Nov 27, 2025 at 2:37 AM Kirill Reshke <reshkekirill@gmail.com> wrote:
> Looking at v3 raises two questions for me.
>
> First is if we should have a doc notion of which variables ought to be
> set to what.

Are you suggesting that we document which GUC parameters should be set,
and to what values, for logical replication? We already have a section on this
in logical-replication.sgml. Is that sufficient?


> Second, how do we actually test that subscription connection options
> are applied on the subscriber side? Can we have TAP for this  (is is
> worth the troubles)?

+1 on adding a test. One idea is to enable log_replication_commands via
the CONNECTION option and then check that the publisher’s log contains
the message "received replication command: IDENTIFY_SYSTEM".
There may be a cleaner way to test this, though.

Regards,

-- 
Fujii Masao

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-11-27T05:17:15Z

    On Thu, Nov 27, 2025 at 11:46 AM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Thu, Nov 27, 2025 at 2:37 AM Kirill Reshke <reshkekirill@gmail.com> wrote:
> > Looking at v3 raises two questions for me.
> >
> > First is if we should have a doc notion of which variables ought to be
> > set to what.
>
> Are you suggesting that we document which GUC parameters should be set,
> and to what values, for logical replication? We already have a section on this
> in logical-replication.sgml. Is that sufficient?
>
>
> > Second, how do we actually test that subscription connection options
> > are applied on the subscriber side? Can we have TAP for this  (is is
> > worth the troubles)?
>
> +1 on adding a test. One idea is to enable log_replication_commands via
> the CONNECTION option and then check that the publisher’s log contains
> the message "received replication command: IDENTIFY_SYSTEM".
> There may be a cleaner way to test this, though.

I've added the test and attached it as patch v4-0002.

The test enables log_disconnections via the CONNECTION string and then checks
that the publisher's log contains the expected disconnection message after
the logical replication connection is reestablished.

Regards,

-- 
Fujii Masao

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-12-02T06:02:28Z

    On Thu, Nov 27, 2025 at 2:17 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> I've added the test and attached it as patch v4-0002.
>
> The test enables log_disconnections via the CONNECTION string and then checks
> that the publisher's log contains the expected disconnection message after
> the logical replication connection is reestablished.

Barring any objections, I will commit the v4-001 patch and backpatch it to v15.

Regards,

-- 
Fujii Masao

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

    Kirill Reshke <reshkekirill@gmail.com> — 2025-12-02T06:37:08Z

    On Tue, 2 Dec 2025 at 11:02, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Thu, Nov 27, 2025 at 2:17 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> > I've added the test and attached it as patch v4-0002.
> >
> > The test enables log_disconnections via the CONNECTION string and then checks
> > that the publisher's log contains the expected disconnection message after
> > the logical replication connection is reestablished.
>
> Barring any objections, I will commit the v4-001 patch and backpatch it to v15.
>
> Regards,
>
> --
> Fujii Masao

Sure lgtm. I actually checked v4-0001, but did not got any objections

-- 
Best regards,
Kirill Reshke

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

    Amit Kapila <amit.kapila16@gmail.com> — 2025-12-02T12:08:03Z

    On Tue, Nov 18, 2025 at 9:29 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> In logical replication, any GUC settings specified in the CONNECTION clause of
> CREATE SUBSCRIPTION are currently ignored. For example:
>
>         CREATE SUBSCRIPTION mysub
>             CONNECTION 'options=''-c wal_sender_timeout=1000'''
>             PUBLICATION ...
>
> The wal_sender_timeout value here has no effect.
>
> This is inconvenient when different logical replication walsenders need
> different settings - e.g., a small wal_sender_timeout for walsender
> connecting to a nearby subscriber and a larger one for walsender
> connecting to a distant subscriber. Right now, it's not easy for users
> to control such per-connection behavior.
>
> The reason of thid limitation is that libpqrcv_connect() always overwrites
> the options connection parameter as follows:
>
>         keys[++i] = "options";
>         vals[i] = "-c datestyle=ISO -c intervalstyle=postgres -c
> extra_float_digits=3";
>
> This wipes out any user-specified GUCs in the CONNECTION string.
> Physical replication does not have this problem because it does not overwrite
> options, so GUCs in primary_conninfo are honored.
>
> To remove this restriction, how about switching to issuing SET commands for
> datestyle, intervalstyle, and extra_float_digits after the connection
> is established,
> similar to what postgres_fdw does, instead of forcing them into options?
> That would allow user-specified GUC settings in CREATE SUBSCRIPTION to
> take effect.
>

Is it possible that we append the predefined options to the options
given by the user to avoid extra round-trip?

-- 
With Regards,
Amit Kapila.

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-12-02T14:59:55Z

    On Tue, Dec 2, 2025 at 9:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Is it possible that we append the predefined options to the options
> given by the user to avoid extra round-trip?

One idea is to add a function, similar to libpqrcv_get_dbname_from_conninfo()
in libpqwalreceiver.c, that extracts the options string from the conninfo,
to append the required fixed settings, and then to use the combined string as
the value of the options parameter. Do you think implementing this is worthwhile
to avoid the extra round trip?

Regards,

-- 
Fujii Masao

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

    Amit Kapila <amit.kapila16@gmail.com> — 2025-12-03T05:45:06Z

    On Tue, Dec 2, 2025 at 8:30 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Tue, Dec 2, 2025 at 9:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > Is it possible that we append the predefined options to the options
> > given by the user to avoid extra round-trip?
>
> One idea is to add a function, similar to libpqrcv_get_dbname_from_conninfo()
> in libpqwalreceiver.c, that extracts the options string from the conninfo,
> to append the required fixed settings, and then to use the combined string as
> the value of the options parameter.
>

Yes, but libpqrcv_get_dbname_from_conninfo() is an exposed function,
we can implement something internal for libpqwalreceiver.c like
conninfo_add_defaults() present in fe-connect.c.

> Do you think implementing this is worthwhile
> to avoid the extra round trip?
>

I think so. Today, we have three variables, tomorrow there could be
more such variables as well and apart from avoiding extra round trips,
the idea to append options to provided options (if any) sounds more
logical to me.

-- 
With Regards,
Amit Kapila.

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-12-19T07:55:19Z

    On Wed, Dec 3, 2025 at 2:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Dec 2, 2025 at 8:30 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > On Tue, Dec 2, 2025 at 9:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > Is it possible that we append the predefined options to the options
> > > given by the user to avoid extra round-trip?
> >
> > One idea is to add a function, similar to libpqrcv_get_dbname_from_conninfo()
> > in libpqwalreceiver.c, that extracts the options string from the conninfo,
> > to append the required fixed settings, and then to use the combined string as
> > the value of the options parameter.
> >
>
> Yes, but libpqrcv_get_dbname_from_conninfo() is an exposed function,
> we can implement something internal for libpqwalreceiver.c like
> conninfo_add_defaults() present in fe-connect.c.
>
> > Do you think implementing this is worthwhile
> > to avoid the extra round trip?
> >
>
> I think so. Today, we have three variables, tomorrow there could be
> more such variables as well and apart from avoiding extra round trips,
> the idea to append options to provided options (if any) sounds more
> logical to me.

OK, I've implemented the idea discussed upthread. The patch updates
libpqrcv_connect() so that, for logical replication, it extracts the options
string from the conninfo, appends the required fixed settings, and passes
the combined string as the options parameter to libpq.

For example, if the conninfo specifies -c wal_sender_timeout=10s,
the resulting options value becomes:

        -c wal_sender_timeout=10s -c datestyle=ISO -c
intervalstyle=postgres -c extra_float_digits=3.

Patch attached.

Regards,

-- 
Fujii Masao

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

    Japin Li <japinli@hotmail.com> — 2025-12-19T10:07:24Z

    On Fri, 19 Dec 2025 at 16:55, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Wed, Dec 3, 2025 at 2:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Tue, Dec 2, 2025 at 8:30 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>> >
>> > On Tue, Dec 2, 2025 at 9:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> > > Is it possible that we append the predefined options to the options
>> > > given by the user to avoid extra round-trip?
>> >
>> > One idea is to add a function, similar to libpqrcv_get_dbname_from_conninfo()
>> > in libpqwalreceiver.c, that extracts the options string from the conninfo,
>> > to append the required fixed settings, and then to use the combined string as
>> > the value of the options parameter.
>> >
>>
>> Yes, but libpqrcv_get_dbname_from_conninfo() is an exposed function,
>> we can implement something internal for libpqwalreceiver.c like
>> conninfo_add_defaults() present in fe-connect.c.
>>
>> > Do you think implementing this is worthwhile
>> > to avoid the extra round trip?
>> >
>>
>> I think so. Today, we have three variables, tomorrow there could be
>> more such variables as well and apart from avoiding extra round trips,
>> the idea to append options to provided options (if any) sounds more
>> logical to me.
>
> OK, I've implemented the idea discussed upthread. The patch updates
> libpqrcv_connect() so that, for logical replication, it extracts the options
> string from the conninfo, appends the required fixed settings, and passes
> the combined string as the options parameter to libpq.
>
> For example, if the conninfo specifies -c wal_sender_timeout=10s,
> the resulting options value becomes:
>
>         -c wal_sender_timeout=10s -c datestyle=ISO -c
> intervalstyle=postgres -c extra_float_digits=3.
>
> Patch attached.

Thanks for the patch — that was my oversight.

LGTM with one small suggestion:

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.

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

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-12-19T11:05:31Z

    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

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

    Kirill Reshke <reshkekirill@gmail.com> — 2025-12-19T12:30:36Z

    On Fri, 19 Dec 2025 at 16: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


Hi!
I checked the new TAP test 0002 changes. I am wondering, why are
connection options validated so late in this test? I mean,  we do
ALTER PUBLICATION, then we restart publisher, wait for catchup, check
alter publication, and etc, and only then we look if connection
options are indeed applied?

-- 
Best regards,
Kirill Reshke

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-12-19T14:42:14Z

    On Fri, Dec 19, 2025 at 9:30 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
> I checked the new TAP test 0002 changes. I am wondering, why are
> connection options validated so late in this test? I mean,  we do
> ALTER PUBLICATION, then we restart publisher, wait for catchup, check
> alter publication, and etc, and only then we look if connection
> options are indeed applied?

Are you suggesting testing whether the conninfo setting is applied earlier,
for example, right after both running ALTER SUBSCRIPTION CONNECTION and
confirming that the logical replication connection is re-established?
Yeah, that might be better and would also make the test easier to read.

Regards,

-- 
Fujii Masao

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

    Kirill Reshke <reshkekirill@gmail.com> — 2025-12-19T15:48:39Z

    On Fri, 19 Dec 2025 at 19:42, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Fri, Dec 19, 2025 at 9:30 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
> > I checked the new TAP test 0002 changes. I am wondering, why are
> > connection options validated so late in this test? I mean,  we do
> > ALTER PUBLICATION, then we restart publisher, wait for catchup, check
> > alter publication, and etc, and only then we look if connection
> > options are indeed applied?
>
> Are you suggesting testing whether the conninfo setting is applied earlier,
> for example, right after both running ALTER SUBSCRIPTION CONNECTION and
> confirming that the logical replication connection is re-established?
> Yeah, that might be better and would also make the test easier to read.
>
> Regards,
>
> --
> Fujii Masao

Yes, exactly


-- 
Best regards,
Kirill Reshke

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

    Amit Kapila <amit.kapila16@gmail.com> — 2025-12-23T05:05:32Z

    On Fri, Dec 19, 2025 at 1:25 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Wed, Dec 3, 2025 at 2:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Dec 2, 2025 at 8:30 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> > >
> > > On Tue, Dec 2, 2025 at 9:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > Is it possible that we append the predefined options to the options
> > > > given by the user to avoid extra round-trip?
> > >
> > > One idea is to add a function, similar to libpqrcv_get_dbname_from_conninfo()
> > > in libpqwalreceiver.c, that extracts the options string from the conninfo,
> > > to append the required fixed settings, and then to use the combined string as
> > > the value of the options parameter.
> > >
> >
> > Yes, but libpqrcv_get_dbname_from_conninfo() is an exposed function,
> > we can implement something internal for libpqwalreceiver.c like
> > conninfo_add_defaults() present in fe-connect.c.
> >
> > > Do you think implementing this is worthwhile
> > > to avoid the extra round trip?
> > >
> >
> > I think so. Today, we have three variables, tomorrow there could be
> > more such variables as well and apart from avoiding extra round trips,
> > the idea to append options to provided options (if any) sounds more
> > logical to me.
>
> OK, I've implemented the idea discussed upthread. The patch updates
> libpqrcv_connect() so that, for logical replication, it extracts the options
> string from the conninfo, appends the required fixed settings, and passes
> the combined string as the options parameter to libpq.
>
> For example, if the conninfo specifies -c wal_sender_timeout=10s,
> the resulting options value becomes:
>
>         -c wal_sender_timeout=10s -c datestyle=ISO -c
> intervalstyle=postgres -c extra_float_digits=3.
>
> Patch attached.
>

Thanks, the idea looks good now. One minor comment:
  }
+
  /*

This appears to be an unnecessary addition in the patch.

-- 
With Regards,
Amit Kapila.

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

    Chao Li <li.evan.chao@gmail.com> — 2025-12-23T06:17:34Z

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

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

    Japin Li <japinli@hotmail.com> — 2025-12-23T08:13:06Z

    On Tue, 23 Dec 2025 at 14:17, Chao Li <li.evan.chao@gmail.com> wrote:
>> 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.
>

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.

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

    Chao Li <li.evan.chao@gmail.com> — 2025-12-23T08:45:56Z

    
> On Dec 23, 2025, at 16:13, Japin Li <japinli@hotmail.com> wrote:
> 
> On Tue, 23 Dec 2025 at 14:17, Chao Li <li.evan.chao@gmail.com> wrote:
>>> 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.
>> 
> 
> 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/

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-12-25T07:00:04Z

    On Sat, Dec 20, 2025 at 12:48 AM Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> On Fri, 19 Dec 2025 at 19:42, Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > On Fri, Dec 19, 2025 at 9:30 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
> > > I checked the new TAP test 0002 changes. I am wondering, why are
> > > connection options validated so late in this test? I mean,  we do
> > > ALTER PUBLICATION, then we restart publisher, wait for catchup, check
> > > alter publication, and etc, and only then we look if connection
> > > options are indeed applied?
> >
> > Are you suggesting testing whether the conninfo setting is applied earlier,
> > for example, right after both running ALTER SUBSCRIPTION CONNECTION and
> > confirming that the logical replication connection is re-established?
> > Yeah, that might be better and would also make the test easier to read.
> >
> > Regards,
> >
> > --
> > Fujii Masao
>
> Yes, exactly

OK, I've updated the 0002 patch accordingly.

Regards,

-- 
Fujii Masao

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

    Fujii Masao <masao.fujii@gmail.com> — 2025-12-25T07:01:28Z

    On Tue, Dec 23, 2025 at 2:05 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Thanks, the idea looks good now. One minor comment:

Thanks for the review!

>   }
> +
>   /*
>
> This appears to be an unnecessary addition in the patch.

The added blank line is due to pgindent, so I didn't exclude it from the patch.
I'm not sure what the preferred practice is for handling pgindent changes
in older branches, but in general it seems better to keep the code clean
and pgindent-compliant even there. Thoughts?

Regards,

-- 
Fujii Masao