Horton
Menu

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 regression test for background worker restart after crash.

  2. Fix background worker not restarting after crash-and-restart cycle.

  3. Refactor code to handle death of a backend or bgworker in postmaster

  1. Logical replication launcher did not automatically restart when got SIGKILL

    cca5507 <cca5507@qq.com> — 2025-07-15T09:25:58Z

    Hi, hackers


I found the $SUBJECT, the main reason is that RegisteredBgWorker::rw_pid has not been cleaned.


Attach a patch to fix it.


--
Regards,
ChangAo Chen

  2. Re: Logical replication launcher did not automatically restart when got SIGKILL

    shveta malik <shveta.malik@gmail.com> — 2025-07-15T10:34:26Z

    On Tue, Jul 15, 2025 at 2:56 PM cca5507 <cca5507@qq.com> wrote:
>
> Hi, hackers
>
> I found the $SUBJECT, the main reason is that RegisteredBgWorker::rw_pid has not been cleaned.
>
> Attach a patch to fix it.
>

Thank You for reporting this. The problem exists and the patch works
as expected.

In the patch, we are resetting the PID during shared memory
initialization. Is there a better place to handle PID reset in the
case of a SIGKILL, possibly within a cleanup flow? For example, during
a regular shutdown, we reset the launcher (background worker) PID in
CleanupBackend(). Or is this the only possibility?

thanks
Shveta

  3. Re: Logical replication launcher did not automatically restart when got SIGKILL

    cca5507 <cca5507@qq.com> — 2025-07-15T10:46:42Z

    Hi,

Reset the PID in&nbsp;ResetBackgroundWorkerCrashTimes() may also works, but I'm not sure which is better.


--
Regards,
ChangAo Chen

  4. Re: Logical replication launcher did not automatically restart when got SIGKILL

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-15T15:08:13Z

    
On 2025/07/15 19:34, shveta malik wrote:
> On Tue, Jul 15, 2025 at 2:56 PM cca5507 <cca5507@qq.com> wrote:
>>
>> Hi, hackers
>>
>> I found the $SUBJECT, the main reason is that RegisteredBgWorker::rw_pid has not been cleaned.
>>
>> Attach a patch to fix it.

Thanks for the report!

This issue appears to have been introduced by commit 28a520c0b77. As a result,
not only the logical replication launcher but also other background workers
(like autoprewarm) may fail to restart after a server crash.


> Thank You for reporting this. The problem exists and the patch works
> as expected.
> 
> In the patch, we are resetting the PID during shared memory
> initialization. Is there a better place to handle PID reset in the
> case of a SIGKILL, possibly within a cleanup flow? For example, during
> a regular shutdown, we reset the launcher (background worker) PID in
> CleanupBackend(). Or is this the only possibility?

 From a quick look at the code, it seems that the second half of CleanupBackend()
is responsible for cleaning up background workers and resetting rw_pid to 0.
However, in the crash case, the function exits immediately after calling
HandleChildCrash(), skipping that cleanup:

	if (crashed)
	{
		HandleChildCrash(bp_pid, exitstatus, procname);
		return;
	}

This could be the problem? Shouldn't the background worker cleanup still
happen even in the crash case?

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

  5. Re: Logical replication launcher did not automatically restart when got SIGKILL

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-15T15:20:05Z

    
On 2025/07/16 0:08, Fujii Masao wrote:
> 
> 
> On 2025/07/15 19:34, shveta malik wrote:
>> On Tue, Jul 15, 2025 at 2:56 PM cca5507 <cca5507@qq.com> wrote:
>>>
>>> Hi, hackers
>>>
>>> I found the $SUBJECT, the main reason is that RegisteredBgWorker::rw_pid has not been cleaned.
>>>
>>> Attach a patch to fix it.
> 
> Thanks for the report!
> 
> This issue appears to have been introduced by commit 28a520c0b77. As a result,
> not only the logical replication launcher but also other background workers
> (like autoprewarm) may fail to restart after a server crash.

I found that the same issue was previously reported here [1],
and a patch has been added to the current commitfest [2].

Regards,

[1] https://www.postgresql.org/message-id/flat/CAF6JsWiO=i24qYitWe6ns1sXqcL86rYxdyU+pNYk-WueKPSySg@mail.gmail.com
[2] https://commitfest.postgresql.org/patch/5844/

-- 
Fujii Masao
NTT DATA Japan Corporation

  6. Re: Logical replication launcher did not automatically restart when got SIGKILL

    cca5507 <cca5507@qq.com> — 2025-07-16T03:21:10Z

    Hi,

The v1-0002 in [1] will call ReportBackgroundWorkerExit() which will send SIGUSR1 to 'bgw_notify_pid', but it may already exit in HandleChildCrash(), is this ok?


[1]
https://commitfest.postgresql.org/patch/5844/


--
Regards,
ChangAo Chen

  7. Re: Logical replication launcher did not automatically restart when got SIGKILL

    shveta malik <shveta.malik@gmail.com> — 2025-07-17T09:57:41Z

    On Wed, Jul 16, 2025 at 8:51 AM cca5507 <cca5507@qq.com> wrote:
>
> Hi,
>
> The v1-0002 in [1] will call ReportBackgroundWorkerExit() which will send SIGUSR1 to 'bgw_notify_pid', but it may already exit in HandleChildCrash(), is this ok?
>

Shall ReportBackgroundWorkerExit() be skipped for 'crashed' background worker?

If we look at code prior to commit 28a520c0b77, there we were setting
'rw_crashed_at' in CleanupBackgroundWorker() and then
HandleChildCrash() was resetting the pid and exiting with no
additional processing.

thanks
Shveta

  8. Re: Logical replication launcher did not automatically restart when got SIGKILL

    Fujii Masao <masao.fujii@gmail.com> — 2025-07-24T09:09:01Z

    On Thu, Jul 17, 2025 at 6:58 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Wed, Jul 16, 2025 at 8:51 AM cca5507 <cca5507@qq.com> wrote:
> >
> > Hi,
> >
> > The v1-0002 in [1] will call ReportBackgroundWorkerExit() which will send SIGUSR1 to 'bgw_notify_pid', but it may already exit in HandleChildCrash(), is this ok?
> >
>
> Shall ReportBackgroundWorkerExit() be skipped for 'crashed' background worker?
>
> If we look at code prior to commit 28a520c0b77, there we were setting
> 'rw_crashed_at' in CleanupBackgroundWorker() and then
> HandleChildCrash() was resetting the pid and exiting with no
> additional processing.

It seems we don't need to set rw_crashed_at in crash cases,
since it's always reset to 0 by ResetBackgroundWorkerCrashTimes()
in restart-after-crash code. So, the only additional step we need may be
resetting rw_pid to 0.

Instead of modifying CleanupBackend() to do this, another option
could be to reset rw_pid during restart-after-crash code, for example,
inside ResetBackgroundWorkerCrashTimes(). Thought?

Regards,

-- 
Fujii Masao

  9. Re: Logical replication launcher did not automatically restart when got SIGKILL

    shveta malik <shveta.malik@gmail.com> — 2025-07-24T09:46:08Z

    On Thu, Jul 24, 2025 at 2:39 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Thu, Jul 17, 2025 at 6:58 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Wed, Jul 16, 2025 at 8:51 AM cca5507 <cca5507@qq.com> wrote:
> > >
> > > Hi,
> > >
> > > The v1-0002 in [1] will call ReportBackgroundWorkerExit() which will send SIGUSR1 to 'bgw_notify_pid', but it may already exit in HandleChildCrash(), is this ok?
> > >
> >
> > Shall ReportBackgroundWorkerExit() be skipped for 'crashed' background worker?
> >
> > If we look at code prior to commit 28a520c0b77, there we were setting
> > 'rw_crashed_at' in CleanupBackgroundWorker() and then
> > HandleChildCrash() was resetting the pid and exiting with no
> > additional processing.
>
> It seems we don't need to set rw_crashed_at in crash cases,
> since it's always reset to 0 by ResetBackgroundWorkerCrashTimes()
> in restart-after-crash code.

Yes, that seems the case,

> So, the only additional step we need may be
> resetting rw_pid to 0.
>

I agree.

> Instead of modifying CleanupBackend() to do this, another option
> could be to reset rw_pid during restart-after-crash code, for example,
> inside ResetBackgroundWorkerCrashTimes(). Thought?
>

Sounds reasonable.
Thinking out loud, when cleaning up after a backend or background
worker crash, process_pm_child_exit() is invoked, which subsequently
calls both CleanupBackend() and HandleChildCrash(). After the cleanup
completes, process_pm_child_exit() calls PostmasterStateMachine() to
move to the next state. As part of that, PostmasterStateMachine()
invokes ResetBackgroundWorkerCrashTimes() (only in crash
scenarios/FatalError), to reset a few things. Since it also resets
rw_worker.bgw_notify_pid, it seems reasonable to reset the rw_pid as
well there.

thanks
Shveta

  10. Re: Logical replication launcher did not automatically restart when got SIGKILL

    Fujii Masao <masao.fujii@gmail.com> — 2025-07-25T01:47:18Z

    On Thu, Jul 24, 2025 at 6:46 PM shveta malik <shveta.malik@gmail.com> wrote:
> Sounds reasonable.
> Thinking out loud, when cleaning up after a backend or background
> worker crash, process_pm_child_exit() is invoked, which subsequently
> calls both CleanupBackend() and HandleChildCrash(). After the cleanup
> completes, process_pm_child_exit() calls PostmasterStateMachine() to
> move to the next state. As part of that, PostmasterStateMachine()
> invokes ResetBackgroundWorkerCrashTimes() (only in crash
> scenarios/FatalError), to reset a few things. Since it also resets
> rw_worker.bgw_notify_pid, it seems reasonable to reset the rw_pid as
> well there.

Thanks!
Attached is a patch that fixes the issue by resetting rw_pid in
ResetBackgroundWorkerCrashTimes().

We should probably add a regression test for this case,
but I'd prefer to commit the fix first and work on the test separately.
Andrey Rudometov proposed a test patch in thread [1],
which we might use as a starting point.

Regards,

[1] https://postgr.es/m/CAF6JsWiO=i24qYitWe6ns1sXqcL86rYxdyU+pNYk-WueKPSySg@mail.gmail.com

-- 
Fujii Masao

  11. Re: Logical replication launcher did not automatically restart when got SIGKILL

    cca5507 <cca5507@qq.com> — 2025-07-25T03:30:40Z

    Hi,

The v2-0001 LGTM!


--
Regards,
ChangAo Chen

  12. Re: Logical replication launcher did not automatically restart when got SIGKILL

    shveta malik <shveta.malik@gmail.com> — 2025-07-25T08:25:03Z

    On Fri, Jul 25, 2025 at 7:17 AM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Thu, Jul 24, 2025 at 6:46 PM shveta malik <shveta.malik@gmail.com> wrote:
> > Sounds reasonable.
> > Thinking out loud, when cleaning up after a backend or background
> > worker crash, process_pm_child_exit() is invoked, which subsequently
> > calls both CleanupBackend() and HandleChildCrash(). After the cleanup
> > completes, process_pm_child_exit() calls PostmasterStateMachine() to
> > move to the next state. As part of that, PostmasterStateMachine()
> > invokes ResetBackgroundWorkerCrashTimes() (only in crash
> > scenarios/FatalError), to reset a few things. Since it also resets
> > rw_worker.bgw_notify_pid, it seems reasonable to reset the rw_pid as
> > well there.
>
> Thanks!
> Attached is a patch that fixes the issue by resetting rw_pid in
> ResetBackgroundWorkerCrashTimes().
>

The patch LGTM.

> We should probably add a regression test for this case,
> but I'd prefer to commit the fix first and work on the test separately.
> Andrey Rudometov proposed a test patch in thread [1],
> which we might use as a starting point.

Sounds good.

thanks
Shveta

  13. Re: Logical replication launcher did not automatically restart when got SIGKILL

    Fujii Masao <masao.fujii@gmail.com> — 2025-07-25T09:44:35Z

    On Fri, Jul 25, 2025 at 5:25 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Jul 25, 2025 at 7:17 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > On Thu, Jul 24, 2025 at 6:46 PM shveta malik <shveta.malik@gmail.com> wrote:
> > > Sounds reasonable.
> > > Thinking out loud, when cleaning up after a backend or background
> > > worker crash, process_pm_child_exit() is invoked, which subsequently
> > > calls both CleanupBackend() and HandleChildCrash(). After the cleanup
> > > completes, process_pm_child_exit() calls PostmasterStateMachine() to
> > > move to the next state. As part of that, PostmasterStateMachine()
> > > invokes ResetBackgroundWorkerCrashTimes() (only in crash
> > > scenarios/FatalError), to reset a few things. Since it also resets
> > > rw_worker.bgw_notify_pid, it seems reasonable to reset the rw_pid as
> > > well there.
> >
> > Thanks!
> > Attached is a patch that fixes the issue by resetting rw_pid in
> > ResetBackgroundWorkerCrashTimes().
> >
>
> The patch LGTM.

Thanks to both ChangAo and Shveta for the review!
I've pushed the patch and back-patched it to v18.


> > We should probably add a regression test for this case,
> > but I'd prefer to commit the fix first and work on the test separately.
> > Andrey Rudometov proposed a test patch in thread [1],
> > which we might use as a starting point.
>
> Sounds good.

This proposed patch adds a new regression test file to verify background
worker restarts when restart_after_crash is enabled. However, since
we already have 013_crash_restart.pl for testing that scenario,
I’m thinking it might be sufficient to check that the logical replication
launcher, i.e., the background worker, restarts properly there.

For example, we could add a check like the following to 013_crash_restart.pl.
Thoughts?

-----------------
is($node->safe_psql('postgres',
"SELECT count(*) = 1 FROM pg_stat_activity WHERE backend_type =
'logical replication launcher'"),
't',
'logical replication launcher is running after crash');
-----------------

Regards,

-- 
Fujii Masao

  14. Re: Logical replication launcher did not automatically restart when got SIGKILL

    cca5507 <cca5507@qq.com> — 2025-07-25T13:53:00Z

    &gt; For example, we could add a check like the following to 013_crash_restart.pl.
&gt; Thoughts?
&gt;&nbsp;
&gt; -----------------
&gt; is($node-&gt;safe_psql('postgres',
&gt; "SELECT count(*) = 1 FROM pg_stat_activity WHERE backend_type =
&gt; 'logical replication launcher'"),
&gt; 't',
&gt; 'logical replication launcher is running after crash');
&gt; -----------------


Agree, but we need note that the logical replication launcher won't be registered in some case. (see ApplyLauncherRegister())


--
Regards,
ChangAo Chen

  15. Re: Logical replication launcher did not automatically restart when got SIGKILL

    Fujii Masao <masao.fujii@gmail.com> — 2025-07-25T15:30:02Z

    On Fri, Jul 25, 2025 at 10:53 PM cca5507 <cca5507@qq.com> wrote:
>
> > For example, we could add a check like the following to 013_crash_restart.pl.
> > Thoughts?
> >
> > -----------------
> > is($node->safe_psql('postgres',
> > "SELECT count(*) = 1 FROM pg_stat_activity WHERE backend_type =
> > 'logical replication launcher'"),
> > 't',
> > 'logical replication launcher is running after crash');
> > -----------------

Patch attached.
I confirmed that the test fails when applied to a version before
commit b5d084c5353 (i.e., before the bug was fixed), and it passes
on HEAD with the patch applied.

> Agree, but we need note that the logical replication launcher won't be registered in some case. (see ApplyLauncherRegister())

Since 013_crash_restart.pl doesn't set any parameters that would
prevent the logical replication launcher from starting, I think
we don't need to worry about that. No??

Regards,

-- 
Fujii Masao

  16. Re: Logical replication launcher did not automatically restart when got SIGKILL

    cca5507 <cca5507@qq.com> — 2025-07-26T09:27:15Z

    Hi,


The test case seems to have a problem:


We cannot ensure that the SELECT happens after the pg_stat_activity can show&nbsp;the logical replication launcher.


With the following patch the test will fail (without the patch it may happen very rarely):


diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index 742d9ba68e9..1e155587c55 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -1162,6 +1162,7 @@ ApplyLauncherMain(Datum main_arg)
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;* Establish connection to nailed catalogs (we only ever access
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;* pg_subscription).
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;*/
+&nbsp; &nbsp; &nbsp; &nbsp;sleep(100);
&nbsp; &nbsp; &nbsp; &nbsp; BackgroundWorkerInitializeConnection(NULL, NULL, 0);
&nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; /*


--
Regards,
ChangAo Chen

  17. Re: Logical replication launcher did not automatically restart when got SIGKILL

    Fujii Masao <masao.fujii@gmail.com> — 2025-07-27T05:06:56Z

    On Sat, Jul 26, 2025 at 6:27 PM cca5507 <cca5507@qq.com> wrote:
>
> Hi,
>
> The test case seems to have a problem:
>
> We cannot ensure that the SELECT happens after the pg_stat_activity can show the logical replication launcher.

Thanks for the review! You're right. We should use poll_query_until()
instead of safe_psql() to handle this properly.
I've attached an updated patch with that change.

Regards,

-- 
Fujii Masao

  18. Re: Logical replication launcher did not automatically restart when got SIGKILL

    cca5507 <cca5507@qq.com> — 2025-07-27T06:13:24Z

    &gt; I've attached an updated patch with that change.


LGTM!


--
Regards,
ChangAo Chen

  19. Re: Logical replication launcher did not automatically restart when got SIGKILL

    Fujii Masao <masao.fujii@gmail.com> — 2025-07-29T10:45:08Z

    On Sun, Jul 27, 2025 at 3:13 PM cca5507 <cca5507@qq.com> wrote:
>
> > I've attached an updated patch with that change.
>
> LGTM!

I've pushed the patch. Thanks!

Regards,

-- 
Fujii Masao