Обсуждение: Make wal_receiver_timeout configurable per subscription

Hi,

When multiple subscribers connect to different publisher servers,
it can be useful to set different wal_receiver_timeout values for
each connection to better detect failures. However, this isn't
currently possible, which limits flexibility in managing subscriptions.

To address this, I'd like to propose making wal_receiver_timeout
configurable per subscription.

One approach is to add wal_receiver_timeout as a parameter to
CREATE SUBSCRIPTION command, storing it in pg_subscription
so each logical replication worker can use its specific value.

Another option is to change the wal_receiver_timeout's GUC context
from PGC_SIGHUP to PGC_USERSET. This would allow setting different
values via ALTER ROLE SET command for each subscription owner -
effectively enabling per-subscription configuration. Since this
approach is simpler and likely sufficient, I'd prefer starting with this.
Thought?

BTW, this could be extended in the future to other GUCs used by
logical replication workers, such as wal_retrieve_retry_interval.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Fri, May 16, 2025 at 9:11 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> When multiple subscribers connect to different publisher servers,
> it can be useful to set different wal_receiver_timeout values for
> each connection to better detect failures. However, this isn't
> currently possible, which limits flexibility in managing subscriptions.
>
> To address this, I'd like to propose making wal_receiver_timeout
> configurable per subscription.
>
> One approach is to add wal_receiver_timeout as a parameter to
> CREATE SUBSCRIPTION command, storing it in pg_subscription
> so each logical replication worker can use its specific value.
>
> Another option is to change the wal_receiver_timeout's GUC context
> from PGC_SIGHUP to PGC_USERSET. This would allow setting different
> values via ALTER ROLE SET command for each subscription owner -
> effectively enabling per-subscription configuration. Since this
> approach is simpler and likely sufficient, I'd prefer starting with this.
> Thought?
>

The GUC wal_receiver_interval is also used for physical replication
and logical launcher, so won't making it userset can impact those
cases as well, but maybe that is okay. However, for the specific case
you are worried about, isn't it better to make it a subscription
option as that won't have a chance to impact any other cases?

IIUC, the reason you are worried is because different publishers can
have different network latencies with subscribers, so they may want
different timing for feedback/keepalive messages.

--
With Regards,
Amit Kapila.

On Mon, May 19, 2025 at 2:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> The GUC wal_receiver_interval is also used for physical replication
> and logical launcher, so won't making it userset can impact those
> cases as well, but maybe that is okay. However, for the specific case
> you are worried about, isn't it better to make it a subscription
> option as that won't have a chance to impact any other cases?

The advantage of Fujii-san's proposal is that it is very simple to
implement. A subscription option would indeed be better, but it would
also be considerably more complex. Why not start simple and if someone
wants to do the work to add something more complicated, that is fine?

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, 20 May 2025 at 03:16, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, May 19, 2025 at 11:19:48AM -0400, Robert Haas wrote:
> > The advantage of Fujii-san's proposal is that it is very simple to
> > implement. A subscription option would indeed be better, but it would
> > also be considerably more complex. Why not start simple and if someone
> > wants to do the work to add something more complicated, that is fine?
>
> Logically, adding that as an option of CREATE SUBSCRIPTION would just
> be a duplication of what a connection strings are already able to do
> with "options='-c foo=fooval'", isn't it?

Although the value is set in the session that creates the
subscription, it will not be used by the apply worker because the
launcher process, which starts the apply worker after subscription
creation, is unaware of session-specific settings.

> It seems to me that the issue of downgrading wal_receiver_timeout to
> become user-settable is if we're OK to allow non-superusers play with
> it in the code path where it's used currently.  Knowing that physical
> WAL receivers are only spawned in a controlled manner by the startup
> process, this does not sound like an issue.

If we set the wal_receiver_timeout configuration using ALTER ROLE for
the subscription owner's role, the apply worker will start with that
value. However, any changes made via ALTER ROLE ... SET
wal_receiver_timeout will not take effect for an already running apply
worker unless the subscription is disabled and re-enabled. In
contrast, this is handled automatically during CREATE SUBSCRIPTION,
where parameter changes are detected.

Regards,
Vignesh

On Tue, May 20, 2025 at 2:13 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 20 May 2025 at 03:16, Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Mon, May 19, 2025 at 11:19:48AM -0400, Robert Haas wrote:
> > > The advantage of Fujii-san's proposal is that it is very simple to
> > > implement. A subscription option would indeed be better, but it would
> > > also be considerably more complex. Why not start simple and if someone
> > > wants to do the work to add something more complicated, that is fine?
> >
> > Logically, adding that as an option of CREATE SUBSCRIPTION would just
> > be a duplication of what a connection strings are already able to do
> > with "options='-c foo=fooval'", isn't it?

I think there is a difference in the point that Vignesh made below;
the worker can detect wal_receiver_timeout change and restart.

>
> > It seems to me that the issue of downgrading wal_receiver_timeout to
> > become user-settable is if we're OK to allow non-superusers play with
> > it in the code path where it's used currently.  Knowing that physical
> > WAL receivers are only spawned in a controlled manner by the startup
> > process, this does not sound like an issue.
>
> If we set the wal_receiver_timeout configuration using ALTER ROLE for
> the subscription owner's role, the apply worker will start with that
> value. However, any changes made via ALTER ROLE ... SET
> wal_receiver_timeout will not take effect for an already running apply
> worker unless the subscription is disabled and re-enabled. In
> contrast, this is handled automatically during CREATE SUBSCRIPTION,
> where parameter changes are detected.

Right. But given changing wal_receiver_timeout doesn't happen
frequently in practice I guess this would not be a big downside of the
proposed idea.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On 2025/05/20 18:13, vignesh C wrote:
> If we set the wal_receiver_timeout configuration using ALTER ROLE for
> the subscription owner's role, the apply worker will start with that
> value. However, any changes made via ALTER ROLE ... SET
> wal_receiver_timeout will not take effect for an already running apply
> worker unless the subscription is disabled and re-enabled. In
> contrast, this is handled automatically during CREATE SUBSCRIPTION,
> where parameter changes are detected.

Yes, this is one of the limitations of the user-settable wal_receiver_timeout
approach. If we want to change the timeout used by the apply worker without
restarting it, storing the value in pg_subscription (similar to how
synchronous_commit is handled) would be a better solution.

In that case, for example, we could set the default value of
pg_subscription.wal_receiver_timeout to -1, meaning the apply worker should
use the global wal_receiver_timeout from postgresql.conf. If the value is 0
or higher, the apply worker would use the value stored in pg_subscription.


On further thought, another downside of the user-settable approach is that
it doesn't work for parameters like wal_retrieve_retry_interval, which is
used by the logical replication launcher not the apply worker. So if we
want to support per-subscription control for non-apply workers, storing
the settings in pg_subscription might be more appropriate.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Wed, May 21, 2025 at 6:04 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> On 2025/05/20 18:13, vignesh C wrote:
> > If we set the wal_receiver_timeout configuration using ALTER ROLE for
> > the subscription owner's role, the apply worker will start with that
> > value. However, any changes made via ALTER ROLE ... SET
> > wal_receiver_timeout will not take effect for an already running apply
> > worker unless the subscription is disabled and re-enabled. In
> > contrast, this is handled automatically during CREATE SUBSCRIPTION,
> > where parameter changes are detected.
>
> Yes, this is one of the limitations of the user-settable wal_receiver_timeout
> approach. If we want to change the timeout used by the apply worker without
> restarting it, storing the value in pg_subscription (similar to how
> synchronous_commit is handled) would be a better solution.
>
> In that case, for example, we could set the default value of
> pg_subscription.wal_receiver_timeout to -1, meaning the apply worker should
> use the global wal_receiver_timeout from postgresql.conf. If the value is 0
> or higher, the apply worker would use the value stored in pg_subscription.
>

Yeah, I had a similar idea in my mind.

>
> On further thought, another downside of the user-settable approach is that
> it doesn't work for parameters like wal_retrieve_retry_interval, which is
> used by the logical replication launcher not the apply worker. So if we
> want to support per-subscription control for non-apply workers, storing
> the settings in pg_subscription might be more appropriate.
>

Yeah, that could be an option, but one might not want to keep such
variables different for each subscription. Do you think one would like
to prefer specifying variables that only apply to the subscriber-node
in a way other than GUC? I always have this question whenever I see
GUCs like max_sync_workers_per_subscription, which are specific to
only subscriber nodes.

--
With Regards,
Amit Kapila.

On 2025/05/28 0:36, Fujii Masao wrote:
> 
> 
> On 2025/05/22 21:21, Amit Kapila wrote:
>> On Wed, May 21, 2025 at 6:04 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>
>>> On 2025/05/20 18:13, vignesh C wrote:
>>>> If we set the wal_receiver_timeout configuration using ALTER ROLE for
>>>> the subscription owner's role, the apply worker will start with that
>>>> value. However, any changes made via ALTER ROLE ... SET
>>>> wal_receiver_timeout will not take effect for an already running apply
>>>> worker unless the subscription is disabled and re-enabled. In
>>>> contrast, this is handled automatically during CREATE SUBSCRIPTION,
>>>> where parameter changes are detected.
>>>
>>> Yes, this is one of the limitations of the user-settable wal_receiver_timeout
>>> approach. If we want to change the timeout used by the apply worker without
>>> restarting it, storing the value in pg_subscription (similar to how
>>> synchronous_commit is handled) would be a better solution.
>>>
>>> In that case, for example, we could set the default value of
>>> pg_subscription.wal_receiver_timeout to -1, meaning the apply worker should
>>> use the global wal_receiver_timeout from postgresql.conf. If the value is 0
>>> or higher, the apply worker would use the value stored in pg_subscription.
>>>
>>
>> Yeah, I had a similar idea in my mind.
> 
> OK, I've implemented two patches:
> 
>    - 0001 makes the wal_receiver_timeout GUC user-settable.
>    - 0002 adds support for setting wal_receiver_timeout per subscription.
>      It depends on the changes in 0001.
> 
> With both patches applied, wal_receiver_timeout can be set per role or
> per database using ALTER ROLE or ALTER DATABASE (from 0001), and also
> per subscription using CREATE SUBSCRIPTION or ALTER SUBSCRIPTION (from 0002).
> The per-subscription value is stored in pg_subscription.subwalrcvtimeout,
> and it overrides the global setting of wal_receiver_timeout for that
> subscription's apply worker. The default is -1, meaning the global setting
> (from server config, command line, role, or database) is used.

I've attached the rebased patches.

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

On Mon, Jul 14, 2025 at 10:27 PM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
> I've attached the rebased patches.

Attached are the rebased versions of the patches.

Regards,

--
Fujii Masao

On Thu, Oct 23, 2025 at 5:19 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Mon, Jul 14, 2025 at 10:27 PM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
> > I've attached the rebased patches.
>
> Attached are the rebased versions of the patches.

I've rebased the patches again.

Regards,

--
Fujii Masao

References: <a1414b64-bf58-43a6-8494-9704975a41e9@oss.nttdata.com>
    <CAA4eK1JtSN2OW4+xPMOoVfYF5LG+ZdBQ8LMAk1h_mCd5SsuCxw@mail.gmail.com>
    <CA+TgmobgPuxLWMbTzBE72yKDQJTXpCnGjtCN3v5N=u_F3uD_nw@mail.gmail.com>
    <aCumuj3V5geOw8YV@paquier.xyz>
    <CALDaNm0Ro-Z0JdsuZxEYRQxqdOOY2U3vRrPtRU=re4CB8Ee-2A@mail.gmail.com>
    <3ed7e711-102f-496d-93b8-8b2619d4d875@oss.nttdata.com>
    <CAA4eK1LSVODWq5aC92Q2PuHRiGqs68bZmumYbC-D7d39MCvukw@mail.gmail.com>
    <5780e93c-7183-4aeb-b3a9-0a5ba0ff7e02@oss.nttdata.com>
    <adf8214d-f2ae-4777-9ba0-33f18ab77e0b@oss.nttdata.com>
    <CAHGQGwG82P4s6tmYK=aEm-T7QfGJBZvXo=WZfckMkffsX6DZjQ@mail.gmail.com>
    <CAHGQGwHq0hP8zZVxaRrvoqD6ZJsWsTO8E_4QqPn5X3bEfEZSMQ@mail.gmail.com>
User-Agent: mu4e 1.12.12; emacs 29.3
Hi, Fujii

Date: Thu, 05 Feb 2026 12:06:45 +0800

On Thu, 05 Feb 2026 at 09:33, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Thu, Oct 23, 2025 at 5:19 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> On Mon, Jul 14, 2025 at 10:27 PM Fujii Masao
>> <masao.fujii@oss.nttdata.com> wrote:
>> > I've attached the rebased patches.
>>
>> Attached are the rebased versions of the patches.
>
> I've rebased the patches again.
>

Thanks for updating the patches.
I have one small comment on v4-0002:

@@ -104,6 +105,7 @@ typedef struct SubOpts
     int32        maxretention;
     char       *origin;
     XLogRecPtr    lsn;
+    char       *wal_receiver_timeout;
 } SubOpts;

According to the comment above the SubOpts struct:

    Structure to hold a bitmap representing the user-provided CREATE/ALTER
        SUBSCRIPTION command options and the parsed/default values of each of them.

Since `wal_receiver_timeout` is a GUC-style interval value (typically stored as
integer milliseconds), wouldn't it be better to use an int32 here instead of a
string?


> Regards,
>
> --
> Fujii Masao
>
> [2. text/x-diff; v4-0001-Make-GUC-wal_receiver_timeout-user-settable.patch]...
>
> [3. text/x-diff; v4-0002-Add-per-subscription-wal_receiver_timeout-setting.patch]...

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

> On Feb 5, 2026, at 08:33, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Thu, Oct 23, 2025 at 5:19 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> On Mon, Jul 14, 2025 at 10:27 PM Fujii Masao
>> <masao.fujii@oss.nttdata.com> wrote:
>>> I've attached the rebased patches.
>>
>> Attached are the rebased versions of the patches.
>
> I've rebased the patches again.
>
> Regards,
>
> --
> Fujii Masao
>
<v4-0001-Make-GUC-wal_receiver_timeout-user-settable.patch><v4-0002-Add-per-subscription-wal_receiver_timeout-setting.patch>

Hi Fujii-san,

I applied the patch locally and played with it a bit. In short, it adds a new subscription option that allows
overridingthe GUC wal_receiver_timeout for a subscription’s apply worker. The changes look solid overall, and the new
optionworked as expected in my manual testing. 

I have only one small comment:
```
+            /*
+             * Test if the given value is valid for wal_receiver_timeeout GUC.
+             * Skip this test if the value is -1, since -1 is allowed for the
+             * wal_receiver_timeout subscription option, but not for the GUC
+             * itself.
+             */
+            parsed = parse_int(opts->wal_receiver_timeout, &val, 0, NULL);
+            if (!parsed || val != -1)
+                (void) set_config_option("wal_receiver_timeout", opts->wal_receiver_timeout,
+                                         PGC_BACKEND, PGC_S_TEST, GUC_ACTION_SET,
+                                         false, 0, false);
```

Here, parse_int() is also from GUC, with flag 0, it will reject any value with units such as “1s” or “7d”. So in
practice,the only purpose of calling parse_int() here is to detect the special value “-1”. 

Given that, I think using atoi() directly may be simpler and easier to read. For example:
```
    if (atoi(opts->wal_receiver_timeout) != -1)
         /* if value is not -1, then test if the given value is valid for wal_receiver_timeeout GUC.
         (void) set_config_option("wal_receiver_timeout", opts->wal_receiver_timeout,
              PGC_BACKEND, PGC_S_TEST, GUC_ACTION_SET,
              false, 0, false);
```

I tried this locally and `make check` still passed.

Similarly, later in set_wal_receiver_timeout(), MySubscription->walrcvtimeout has already been validated, so we could
alsouse atoi() there instead of parse_int(). 

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

On Thu, Feb 5, 2026 at 1:06 PM Japin Li <japinli@hotmail.com> wrote:
> Thanks for updating the patches.
> I have one small comment on v4-0002:

Thanks for the review!


> @@ -104,6 +105,7 @@ typedef struct SubOpts
>         int32           maxretention;
>         char       *origin;
>         XLogRecPtr      lsn;
> +       char       *wal_receiver_timeout;
>  } SubOpts;
>
> According to the comment above the SubOpts struct:
>
>         Structure to hold a bitmap representing the user-provided CREATE/ALTER
>         SUBSCRIPTION command options and the parsed/default values of each of them.
>
> Since `wal_receiver_timeout` is a GUC-style interval value (typically stored as
> integer milliseconds), wouldn't it be better to use an int32 here instead of a
> string?

The wal_receiver_timeout value in CREATE SUBSCRIPTION can include a unit
(for example, 10s), not just a plain integer. Because of that, we can't store it
in an int32, I think.

Regards,

--
Fujii Masao

On Thu, 05 Feb 2026 at 23:40, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Thu, Feb 5, 2026 at 1:06 PM Japin Li <japinli@hotmail.com> wrote:
>> Thanks for updating the patches.
>> I have one small comment on v4-0002:
>
> Thanks for the review!
>
>
>> @@ -104,6 +105,7 @@ typedef struct SubOpts
>>         int32           maxretention;
>>         char       *origin;
>>         XLogRecPtr      lsn;
>> +       char       *wal_receiver_timeout;
>>  } SubOpts;
>>
>> According to the comment above the SubOpts struct:
>>
>>         Structure to hold a bitmap representing the user-provided CREATE/ALTER
>>         SUBSCRIPTION command options and the parsed/default values of each of them.
>>
>> Since `wal_receiver_timeout` is a GUC-style interval value (typically stored as
>> integer milliseconds), wouldn't it be better to use an int32 here instead of a
>> string?
>
> The wal_receiver_timeout value in CREATE SUBSCRIPTION can include a unit
> (for example, 10s), not just a plain integer. Because of that, we can't store it
> in an int32, I think.
>

If we stored it as an integer, an input such as '1min' would be normalized to
60000 (milliseconds) and lose its unit.

That would make it inconsistent with the original user input shown in pg_subscription.
So we keep it as a string, right?

> Regards,
>
> --
> Fujii Masao

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

On Fri, Feb 6, 2026 at 10:50 AM Japin Li <japinli@hotmail.com> wrote:
> If we stored it as an integer, an input such as '1min' would be normalized to
> 60000 (milliseconds) and lose its unit.
>
> That would make it inconsistent with the original user input shown in pg_subscription.
> So we keep it as a string, right?

Yes, I think.

Regards,

--
Fujii Masao

On Thu, 05 Feb 2026 at 16:04, Chao Li <li.evan.chao@gmail.com> wrote:
>> On Feb 5, 2026, at 08:33, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> On Thu, Oct 23, 2025 at 5:19 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>>>
>>> On Mon, Jul 14, 2025 at 10:27 PM Fujii Masao
>>> <masao.fujii@oss.nttdata.com> wrote:
>>>> I've attached the rebased patches.
>>>
>>> Attached are the rebased versions of the patches.
>>
>> I've rebased the patches again.
>>
>> Regards,
>>
>> --
>> Fujii Masao
>>
<v4-0001-Make-GUC-wal_receiver_timeout-user-settable.patch><v4-0002-Add-per-subscription-wal_receiver_timeout-setting.patch>
>
> Hi Fujii-san,
>
> I applied the patch locally and played with it a bit. In short, it adds a new subscription option that allows
overridingthe GUC wal_receiver_timeout for a subscription’s apply worker. The changes look solid overall, and the new
optionworked as expected in my manual testing. 
>
> I have only one small comment:
> ```
> +            /*
> +             * Test if the given value is valid for wal_receiver_timeeout GUC.
> +             * Skip this test if the value is -1, since -1 is allowed for the
> +             * wal_receiver_timeout subscription option, but not for the GUC
> +             * itself.
> +             */
> +            parsed = parse_int(opts->wal_receiver_timeout, &val, 0, NULL);
> +            if (!parsed || val != -1)
> +                (void) set_config_option("wal_receiver_timeout", opts->wal_receiver_timeout,
> +                                         PGC_BACKEND, PGC_S_TEST, GUC_ACTION_SET,
> +                                         false, 0, false);
> ```
>

1.
Typo, s/timeeout/timeout/g.

2.
The comment mentions skipping only "-1".
Since we already use strcmp(... , "-1") later in the code, wouldn't it be
better to use the same check here too?

+    if (strcmp(subinfo->subwalrcvtimeout, "-1") != 0)
+        appendPQExpBuffer(query, ", wal_receiver_timeout = %s", fmtId(subinfo->subwalrcvtimeout));
+

> Here, parse_int() is also from GUC, with flag 0, it will reject any value with units such as “1s” or “7d”. So in
practice,the only purpose of calling parse_int() here is to detect the special value “-1”. 
>
> Given that, I think using atoi() directly may be simpler and easier to read. For example:
> ```
>     if (atoi(opts->wal_receiver_timeout) != -1)
>          /* if value is not -1, then test if the given value is valid for wal_receiver_timeeout GUC.
>          (void) set_config_option("wal_receiver_timeout", opts->wal_receiver_timeout,
>               PGC_BACKEND, PGC_S_TEST, GUC_ACTION_SET,
>               false, 0, false);
> ```
>
> I tried this locally and `make check` still passed.
>
> Similarly, later in set_wal_receiver_timeout(), MySubscription->walrcvtimeout has already been validated, so we could
alsouse atoi() there instead of parse_int(). 
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/

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

On Fri, Feb 6, 2026 at 2:03 PM Chao Li <li.evan.chao@gmail.com> wrote:
> I applied the patch locally and played with it a bit. In short, it adds a new subscription option that allows
overridingthe GUC wal_receiver_timeout for a subscription’s apply worker. The changes look solid overall, and the new
optionworked as expected in my manual testing. 

Thanks for the review!


> I have only one small comment:
> ```
> +                       /*
> +                        * Test if the given value is valid for wal_receiver_timeeout GUC.
> +                        * Skip this test if the value is -1, since -1 is allowed for the
> +                        * wal_receiver_timeout subscription option, but not for the GUC
> +                        * itself.
> +                        */
> +                       parsed = parse_int(opts->wal_receiver_timeout, &val, 0, NULL);
> +                       if (!parsed || val != -1)
> +                               (void) set_config_option("wal_receiver_timeout", opts->wal_receiver_timeout,
> +                                                                                PGC_BACKEND, PGC_S_TEST,
GUC_ACTION_SET,
> +                                                                                false, 0, false);
> ```
>
> Here, parse_int() is also from GUC, with flag 0, it will reject any value with units such as “1s” or “7d”. So in
practice,the only purpose of calling parse_int() here is to detect the special value “-1”. 
>
> Given that, I think using atoi() directly may be simpler and easier to read. For example:

If we use atoi(), a command like CREATE SUBSCRIPTION with an invalid
wal_receiver_timeout value such as '-1invalid' would succeed, since atoi()
interprets it as -1. I don't think that's desirable behavior. So it would be
better to use parse_int() so that such invalid input is properly rejected.

Regards,

--
Fujii Masao

On Fri, Feb 6, 2026 at 3:44 PM Japin Li <japinli@hotmail.com> wrote:
> 1.
> Typo, s/timeeout/timeout/g.

Fixed. Thanks for the review!


> 2.
> The comment mentions skipping only "-1".
> Since we already use strcmp(... , "-1") later in the code, wouldn't it be
> better to use the same check here too?

With this approach, a command like CREATE SUBSCRIPTION using
a wal_receiver_timeout value such as '-1 ' (i.e., -1 followed by whitespace)
would fail, since it would not be interpreted as -1. I don't think that's
desirable behavior. So it would be better to use parse_int() so that
such input is handled correctly.

Regards,

--
Fujii Masao

> On Feb 13, 2026, at 23:51, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Fri, Feb 6, 2026 at 2:03 PM Chao Li <li.evan.chao@gmail.com> wrote:
>> I applied the patch locally and played with it a bit. In short, it adds a new subscription option that allows
overridingthe GUC wal_receiver_timeout for a subscription’s apply worker. The changes look solid overall, and the new
optionworked as expected in my manual testing. 
>
> Thanks for the review!
>
>
>> I have only one small comment:
>> ```
>> +                       /*
>> +                        * Test if the given value is valid for wal_receiver_timeeout GUC.
>> +                        * Skip this test if the value is -1, since -1 is allowed for the
>> +                        * wal_receiver_timeout subscription option, but not for the GUC
>> +                        * itself.
>> +                        */
>> +                       parsed = parse_int(opts->wal_receiver_timeout, &val, 0, NULL);
>> +                       if (!parsed || val != -1)
>> +                               (void) set_config_option("wal_receiver_timeout", opts->wal_receiver_timeout,
>> +                                                                                PGC_BACKEND, PGC_S_TEST,
GUC_ACTION_SET,
>> +                                                                                false, 0, false);
>> ```
>>
>> Here, parse_int() is also from GUC, with flag 0, it will reject any value with units such as “1s” or “7d”. So in
practice,the only purpose of calling parse_int() here is to detect the special value “-1”. 
>>
>> Given that, I think using atoi() directly may be simpler and easier to read. For example:
>
> If we use atoi(), a command like CREATE SUBSCRIPTION with an invalid
> wal_receiver_timeout value such as '-1invalid' would succeed, since atoi()
> interprets it as -1. I don't think that's desirable behavior. So it would be
> better to use parse_int() so that such invalid input is properly rejected.
>
> Regards,
>
> --
> Fujii Masao

I realized atoi(“-1invalid”) would return -1, but I thought that would be an imagined use case. I’m fine if you insist
touse parse_int. Maybe we can enhance the comment. set_config_option does the test and parse_int is used to skip -1. 

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

On Sat, Feb 14, 2026 at 8:37 AM Chao Li <li.evan.chao@gmail.com> wrote:
> I realized atoi(“-1invalid”) would return -1, but I thought that would be an imagined use case. I’m fine if you
insistto use parse_int. Maybe we can enhance the comment. set_config_option does the test and parse_int is used to skip
-1.

IMO the current comment is sufficient, so I left it unchanged.

+ /* 180000 should be changed to 190000 */
+ if (pset.sversion >= 180000)
+ appendPQExpBuffer(&buf,

Since I started the patch before v19dev was created, I temporarily used
180000 and planned to update it to 190000 later, but forgot to do so.
This is now fixed.

+ /*
+ * Log the current wal_receiver_timeout GUC value (in milliseconds) as a
+ * debug message to verify it was set correctly.
+ */
+ elog(DEBUG1, "logical replication worker for subscription \"%s\"
wal_receiver_timeout: %d ms",
+ MySubscription->name, wal_receiver_timeout);

Previously, this debug message was emitted every time set_wal_receiver_timeout()
was called, even when the value had not changed. For example,
ALTER SUBSCRIPTION ... SET (synchronous_commit = true) would trigger
the message, which seemed strange. I updated the code so the message is
logged only when the effective wal_receiver_timeout value used by
the worker actually changes.

Attached are the updated patches.

Regards,

--
Fujii Masao

On Mon, Feb 16, 2026 at 4:48 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> Attached are the updated patches.

I've pushed the patches. Thanks!

Regards,

--
Fujii Masao