| From: | Shubhra Jain <shubhra(dot)jain(at)ksolves(dot)com> |
|---|---|
| To: | Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> |
| Cc: | Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>, Peter Smith <smithpb2250(at)gmail(dot)com>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: Warn when creating or enabling a subscription with max_logical_replication_workers = 0 |
| Date: | 2026-10-06 06:07:41 |
| Message-ID: | CAOh5eDXnKt4z3_0GPMCaWsT+0kgBeoGOw6h4PiVXagBZGS22hA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Yugo,
I reviewed and tested the patch from your first message (Feb 4,
0001-Warn-when-creating-or-enabling-a-subscription-with-l.patch).
Setup: master at d6393fc40a, built with --enable-cassert --enable-debug
on Linux, with a publisher and a subscriber cluster on one machine.
Without the patch, with max_logical_replication_workers = 0 on the
subscriber, CREATE SUBSCRIPTION prints only the slot NOTICE and
succeeds. pg_stat_subscription lists the subscription with an empty pid,
no "logical replication launcher" process exists on the subscriber, no
rows arrive, and the publisher's slot stays inactive. With the setting
at 4, the same subscription works.
With the patch, same setup:
CREATE SUBSCRIPTION sub_a CONNECTION '...' PUBLICATION pub1;
NOTICE: created replication slot "sub_a" on publisher
WARNING: subscription was created, but logical replication is disabled
HINT: To initiate replication, set "max_logical_replication_workers"
to a non zero value.
ALTER SUBSCRIPTION sub_a ENABLE;
WARNING: subscription was enabled, but logical replication is disabled
HINT: (same hint)
There is no warning for ALTER SUBSCRIPTION ... DISABLE, nor for CREATE
SUBSCRIPTION ... WITH (enabled = false). With
max_logical_replication_workers = 4, none of these commands warn.
make -C src/test/subscription check passes (39 test files, 604 tests).
Observations:
1. On an enabled subscription, ALTER SUBSCRIPTION ... REFRESH
PUBLICATION, SET PUBLICATION, CONNECTION and SET (synchronous_commit)
do not warn with the setting at 0. I assume that is intended, since
those commands do not start replication.
2. With the setting at 0, CREATE SUBSCRIPTION ... WITH (enabled = false,
retain_dead_tuples = true) also prints "subscription was created, but
logical replication is disabled". I see from the comment above the
check that the launcher is needed there to create the conflict
detection slot, so a warning seems justified. But since the
subscription itself is not enabled, "To initiate replication" in the
hint may be confusing. Would different wording be better for that
case? Output:
WARNING: deleted rows to detect conflicts would not be removed
until the subscription is enabled
HINT: Consider setting retain_dead_tuples to false.
NOTICE: created replication slot "sub_r" on publisher
WARNING: subscription was created, but logical replication is disabled
HINT: To initiate replication, set "max_logical_replication_workers"
to a non zero value.
3. Minor: the hint says "non zero"; create_subscription.sgml writes
"non-zero".
4. The patch has no test. If you agree, I would be happy to write a TAP
test in src/test/subscription that starts a subscriber with the
setting at 0 and checks for the warning on CREATE and ENABLE, and for
its absence on DISABLE and with enabled = false. Please tell me if you
would rather add it yourself.
On the slot-exhaustion case discussed upthread: with
max_logical_replication_workers = 1, I created a subscription with the
default options. CREATE SUBSCRIPTION printed only the slot NOTICE in the
client session, while the server log repeated this every 5 seconds:
WARNING: out of logical replication worker slots
HINT: You might need to increase "max_logical_replication_workers".
So that case is not visible to the user who ran the command, and the
patch does not change it, which seems consistent with treating it
separately as discussed upthread. I have not tried a check from the
backend side. If there is interest, I could look into a best-effort check
of free worker slots as a separate patch, but I wanted to ask first
whether that direction would be acceptable.
Regards,
Shubhra
On Tue, Oct 6, 2026 at 10:31 AM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>
wrote:
>
>
> On Mon, 13 Apr 2026 at 09:04, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>
>> On Fri, 6 Feb 2026 18:28:06 +1100
>> Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>>
>> > On Fri, Feb 6, 2026 at 3:37 PM Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>> > >
>> > > On Fri, 6 Feb 2026 09:58:02 +1100
>> > > Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>> > >
>> > > > On Thu, Feb 5, 2026 at 7:12 PM Zhijie Hou (Fujitsu)
>> > > > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>> > > > >
>> > > > > On Thursday, February 5, 2026 3:47 PM Peter Smith <
>> smithpb2250(at)gmail(dot)com> wrote:
>> > > > > > On Thu, Feb 5, 2026 at 12:12 PM Yugo Nagata <
>> nagata(at)sraoss(dot)co(dot)jp> wrote:
>> > > > > > >
>> > > > > > > On Wed, 4 Feb 2026 17:26:25 +1100
>> > > > > > > Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>> > > > > > >
>> > > > ...
>> > > > > >
>> > > > > > Oh right, I mistook that you had run out of logical replication
>> "workers", but in
>> > > > > > fact, because max_logical_replication_workers = 0 the main
>> "logical
>> > > > > > replication launcher" process had failed to start, so logical
>> replication was
>> > > > > > entirely disabled.
>> > > > > >
>> > > > > > See code: in backend/replication/logical/launcher.c
>> > > > > >
>> > > > > > ApplyLauncherRegister(void)
>> > > > > > {
>> > > > > > ...
>> > > > > > if (max_logical_replication_workers == 0 || IsBinaryUpgrade)
>> > > > > > return;
>> > > > > >
>> > > > > > ~~~
>> > > > > >
>> > > > > > Given this, I felt that instead of testing the GUC, what you
>> really want to know
>> > > > > > is just whether that "logical replication launcher" is running
>> or not.
>> > > > > >
>> > > > > > And that launcher pid is already tested when the Subscription
>> commands send
>> > > > > > a "kill" to the launcher. e.g. see function ApplyLauncherWakeup.
>> > > > > >
>> > > > > > So, here is a diff patch, of what I tried:
>> > > > > >
>> > > > > > ------
>> > > > > > diff --git a/src/backend/replication/logical/launcher.c
>> > > > > > b/src/backend/replication/logical/launcher.c
>> > > > > > index 3ed86480be2..f880380ce4e 100644
>> > > > > > --- a/src/backend/replication/logical/launcher.c
>> > > > > > +++ b/src/backend/replication/logical/launcher.c
>> > > > > > @@ -1195,6 +1195,13 @@ ApplyLauncherWakeup(void) {
>> > > > > > if (LogicalRepCtx->launcher_pid != 0)
>> > > > > > kill(LogicalRepCtx->launcher_pid, SIGUSR1);
>> > > > > > + else
>> > > > > > + {
>> > > > > > + if (max_logical_replication_workers == 0)
>> > > > > > + ereport(WARNING,
>> > > > > > + errmsg("Logical replication is
>> > > > > > currently disabled"),
>> > > > > > + errhint("\"%s\" is 0.",
>> > > > > > "max_logical_replication_workers"));
>> > > > > > + }
>> > > > > > }
>> > > > > > ------
>> > > > > >
>> > > > > > Thoughts?
>> > > > >
>> > > > > I think this is not the right place to check this issue. The
>> launcher might fail
>> > > > > for some reasons and restart soon (pid will be set to 0), in
>> which case this
>> > > > > warning wouldn't be appropriate.
>> > > >
>> > > > AFAIK, that's not possible. My warning is guarded by checking
>> > > > max_logical_replication_workers == 0. And in that case, the launcher
>> > > > cannot "fail" because it was never registered/started in the first
>> > > > place.
>> > >
>> > > I also initially considered emitting the warning in
>> > > ApplyLauncherWakeup() after checking max_logical_replication_workers
>> == 0.
>> > > However, I think checking pid == 0 is sufficient.
>> > >
>> > > Even when max_logical_replication_workers is non-zero and the
>> launcher is
>> > > normally running, it could still be killed by user action or by the
>> > > OS, although such cases should be rare. In that situation, emitting
>> the
>> > > same warning would not be appropriate.
>> >
>> > Sorry, I did not understand the previous paragraph. If "when
>> > max_logical_replication_workers is non-zero" then the warning cannot
>> > be emitted inappropriately because that warning is guarded by "if
>> > (max_logical_replication_workers == 0)" (??).
>>
>> You're right, I misunderstood that part. Sorry about that.
>>
>> > > I placed the logic in subscriptioncmds.c so that the warning message
>> can
>> > > better reflect the actual situation, while keeping consistency with
>> > > existing messages such as "subscription was created, but is not
>> > > connected".
>> >
>> > In hindsgght your original patch is very similar to what I posted. I
>> > agree, your messages are better because they are more specific. But
>> > there are multiple calls to ApplyLauncherWakeup() -- not just those 2
>> > that you handled - so I was just unsure if there are some remaining
>> > holes.
>>
>> I see your point.
>>
>> There are other places where ApplyLauncherWakeupAtCommit() is called,
>> for example via AlterSubscription() when retain_dead_tuples is set or
>> when the owner is changed. However, these cases are not affected when
>> max_logical_replication_workers = 0 unless the subscription is enabled.
>>
>> For now, I’ll keep the current approach.
>>
>> > >
>> > > > >
>> > > > > Besides, I also think it would make more sense to issue a warning
>> if the
>> > > > > subscription has no remaining workers to start instead of raising
>> a
>> > > > > warning for 0 setting (the latter seems rare).
>> > > > >
>> > > >
>> > > > It might be rare, but by my understanding, the original post
>> described
>> > > > this specific scenario, whereby the user had previously deliberately
>> > > > configured `max_logical_replication_workers` to 0. Then, some time
>> > > > later, when they attempted CREATE/ALTER SUBSCRIPTION, nothing
>> > > > happened, and there was only silence. If they'd forgotten about
>> their
>> > > > `max_logical_replication_workers` setting, then it could be
>> confusing
>> > > > why nothing was happening.
>> > > >
>> > > > OTOH, when max_logical_replication_workers > 0, then the logical
>> > > > replication launcher would be running, and in that case, there are
>> > > > already plenty of warning logs about not enough worker resources.
>> > >
>> > > Yes. In this case, warnings are emitted to the server log, but they
>> do not
>> > > appear as a response to CREATE/ALTER SUBSCRIPTION. There is an option
>> to
>> > > also emit such warnings during the CREATE/ALTER SUBSCRIPTION command,
>> so
>> > > I will update the patch accordingly.
>> > >
>> > > Nevertheless, this seems to be a different situation from the case
>> where
>> > > logical replication is disabled entirely, so I think the warning
>> messages
>> > > should be handled separately.
>> >
>> > +1. I also think that running out of worker resources is a different
>> > scenario from the entire logical replication being disabled, so it
>> > should be handled separately
>>
>> I've investigated whether we can emit a warning during CREATE/ALTER
>> SUBSCRIPTION
>> when there are no available worker slots to start the subscription.
>>
>> I find there is another more warning worthy scenario that should be
> covered too, what if everything is working normally and then someone
> changes max_logical_replication_workers to 0 in conf file and restarts, now
> logical replication fails to continue. In this case since no CREATE or
> ALTER subscription command was issued, the changes discussed upthread would
> not produce a warning. Shouldn't we cover this case also?
>
>> One issue is that, in the current implementation, this condition is only
>> checked
>> when the launcher actually forks a worker. There is no infrastructure to
>> perform
>> this check in advance from a backend process.
>>
>> Adding such infrastructure would likely require acquiring an LWLock (at
>> least in
>> shared mode) to inspect the current worker state. Moreover, even if we
>> implement
>> such a mechanism, the situation may change between the time of the check
>> and the
>> actual worker startup.
>>
> What about an alternate way to overcome this, following the same pattern
> as in apply_error_count for pg_stat_subscription_stats.
> So, a rough sketch of the approach is like this.. Create a new field
> in PgStat_StatSubEntry say worker_failure_count and add a function to
> increment this field. Now, in logicalrep_worker_launch when it hits worker
> == NULL, call this function to increment the worker_failure_count. I have
> added a rough patch to explain the idea better.
>
> Note that this won't be something as emitting a warning or notice but
> rather needs to be explicitly queried.
>
>>
>> Given these limitations, I'm wondering whether introducing this kind of
>> warning
>> is worthwhile.
>>
>> --
> Regards,
> Rafia Sabih
> CYBERTEC PostgreSQL International GmbH
>
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Florin Irion | 2026-10-06 06:12:11 | Re: Proposal: Supporting URI SAN in Certificate Authentication |
| Previous Message | David G. Johnston | 2026-10-06 06:04:38 | Re: should pg_dumpall --clean apply to database in --exclude-database |