Clarify or fix SIGINT handling in data checksums launcher

Hi Fujii-san,

On Sat, Jul 11, 2026 at 11:14 PM Fujii Masao <masao.fujii@gmail.com> wrote:

In datachecksum_state.c, launcher_cancel_handler() currently says:

* Internal routine for reacting to SIGINT and flagging the worker to
abort.
* The worker won't be interrupted immediately but will check for
abort flag
* between each block in a relation.
*/

This reads to me as if canceling the launcher is expected to cause
the currently running worker to abort. However, the flag set by
launcher_cancel_handler() is process-local to the launcher.
Since the worker has its own process-local flag, it will never
observe the launcher's flag.

As a result, if the launcher receives SIGINT, the worker is not canceled.
Moreover, the launcher waits for the worker to exit before it can
terminate itself. If the worker runs for a long time, for example,
while processing a large database or waiting for a long-running session
holding temporary tables, the launcher's cancellation can be delayed
for a long time as well.

+1

Is this intentional?

I think so; it shouldn't be.

If not, I'd like to propose the attached 0001 patch. It updates the
launcher
to cancel the current worker by calling TerminateBackgroundWorker()
when it receives SIGINT. It also adds a data-checksums-specific helper
for waiting for worker startup or shutdown, to handle the cases where
SIGINT arrives while the launcher is waiting for a worker to start or exit.

I have looked into the 0001 patch; it's LGTM \o/

--
Thanks :)
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/

On 11 Jul 2026, at 19:44, Fujii Masao <masao.fujii@gmail.com> wrote:

Is this intentional?

If so, I think the comment above launcher_cancel_handler() should be
updated so that it does not imply that canceling the launcher directly
causes the worker to abort.

If not, I'd like to propose the attached 0001 patch. It updates the launcher
to cancel the current worker by calling TerminateBackgroundWorker()
when it receives SIGINT. It also adds a data-checksums-specific helper
for waiting for worker startup or shutdown, to handle the cases where
SIGINT arrives while the launcher is waiting for a worker to start or exit.

Early on it was the intentional behaviour, when the worker didn't have a safe
way to cancel processing. That has since been changed but the code didn't
reflect that, so +1 on this patch.

+ if (abort_requested)
+ result = DATACHECKSUMSWORKER_ABORTED;

Unless I'm missing something, this would need a CHECK_FOR_WORKER_ABORT_REQUEST
as well to make sure the abort_requested flag is up to date.

-	status = WaitForBackgroundWorkerStartup(bgw_handle, &pid);
+	status = WaitForDataChecksumsWorkerState(bgw_handle, true, &pid,
+						WAIT_EVENT_BGWORKER_STARTUP);
 	if (status == BGWH_STOPPED)
 	{

Clearly not the fault of this patch, but we should probably add a comment why a
wait for bgworker startup immediately checks for BGWH_STOPPED. Future readers
of this code might find that odd.

-	status = WaitForBackgroundWorkerShutdown(bgw_handle);
+	status = WaitForDataChecksumsWorkerState(bgw_handle, false, NULL,
+						 WAIT_EVENT_BGWORKER_SHUTDOWN);
 	if (status == BGWH_POSTMASTER_DIED)

Again, not the fault of this patch but I wonder if we should add an Assert on
status after the check for BGWH_POSTMASTER_DIED that we have the expected
status.

While reading this patch I started wondering if the check for worker and
launcher abort requests should call pgstat_progress_update_param and set the
phase to a new PROGRESS_DATACHECKSUMS_PHASE_ABORTING? It will likely be a
fairly small window during which it will be visible but it would be the correct
state to show. What do you think?

I wonder if it's worth adding a test for this? Making sure a worker is stuck
on an injection point sleep, siginting the launcher, and then unpausing the
worker to ensure it gets cancelled. It would be tricky to ensure that it got
aborted rather than finished on it's own accord though given that the worker in
a test setup will finish very quickly (and the abort patch leaves few trails to
match a test against).

While working on this, I also found that two local function prototypes
are missing and that the prototype order does not match the function
definition order. This is basically harmless, but I'm tempted to propose
the attached 0002 patch which cleans that up for consistency and readability.

I don't have strong opinions on this, feel free to go ahead with this.

--
Daniel Gustafsson