> On 11 Jul 2026, at 19:44, Fujii Masao <[email protected]> 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
