On Mon, 13 Apr 2026 at 09:04, Yugo Nagata <[email protected]> wrote:
> On Fri, 6 Feb 2026 18:28:06 +1100 > Peter Smith <[email protected]> wrote: > > > On Fri, Feb 6, 2026 at 3:37 PM Yugo Nagata <[email protected]> wrote: > > > > > > On Fri, 6 Feb 2026 09:58:02 +1100 > > > Peter Smith <[email protected]> wrote: > > > > > > > On Thu, Feb 5, 2026 at 7:12 PM Zhijie Hou (Fujitsu) > > > > <[email protected]> wrote: > > > > > > > > > > On Thursday, February 5, 2026 3:47 PM Peter Smith < > [email protected]> wrote: > > > > > > On Thu, Feb 5, 2026 at 12:12 PM Yugo Nagata <[email protected]> > wrote: > > > > > > > > > > > > > > On Wed, 4 Feb 2026 17:26:25 +1100 > > > > > > > Peter Smith <[email protected]> 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
v1-0001-Add-pg_stat_subscription_stats.worker_launch_fail.patch
Description: Binary data
