[krylosov-aa]

Related to #2181

What does this PR do?

Statement and lock timeouts can queue duplicate KeyboardInterrupt callbacks, interrupting Python exception handling.

Keep at most one cancellation callback queued in Cloudberry's PL/Python hook. Ignore stale callbacks after PostgreSQL has processed the cancellation, so they cannot affect a later PL/Python call. Clear the queue flag when the callback runs to allow subsequent cancellation requests

Type of Change

• Bug fix (non-breaking change)

Test Plan

Passed cancel_plpython with both planners on Linux ARM64 / Python 3.10.

Verified client cancellation, repeated cancellation, and statement/lock timeouts.

• Integration tests added/updated

Checklist

• Followed contribution guide
• Requested review from cloudberry committers

[leborchuk]

It's not good to change postgres-based code. After that we will have issues with rebasing it.

Instead of this, consider change pl/python and fix double signal here.

Here the explanation how it works in postgres:

In postgres the duplicate signal delivery exists upstream, deliberately, and has never been fixed because for upstream it isn't a bug. Verified against
REL_17_STABLE and current master (github.com/postgres/postgres):

1. The cancel path delivers SIGINT twice — same as Cloudberry.
   Upstream processCancelRequest calls signal_child(bp->pid, SIGINT), and upstream signal_child (postmaster.c) does exactly what Cloudberry's does:

  if (kill(pid, signal) < 0)          /* signal the child directly    */
      elog(DEBUG3, ...);
  #ifdef HAVE_SETSID
      ...
      if (kill(-pid, signal) < 0)     /* …and its whole process group */
          elog(DEBUG3, ...);
  #endif

2. The timeout handlers double-signal — identical to Cloudberry's old code.

  /* try to signal whole process group */
  kill(-MyProcPid, sig);
  ...
  kill(MyProcPid, sig);               /* sent unconditionally too */

3. Upstream explicitly documents this as intentional (master postmaster.c:3504):

   │ There is a race condition for recently-forked children: they might not have executed setsid() yet. So we signal the child directly as well as the
   │ group… We also assume that signaling the child twice will not cause any problems.

   Why the assumption holds upstream but breaks in Cloudberry

   For plain C code, a duplicate SIGINT is invisible: StatementCancelHandler just sets QueryCancelPending = true, an idempotent boolean. Twice the signal,
   same result. Nothing to fix.

   The break comes from PL/Python's asynchronous cancellation, which is not an upstream feature. I traced it in Cloudberry's git history: commit
   bfa232e — "Support plpython cancellation. (#4988)" by Hubert Zhang, Greenplum, May 2018 — added the Py_AddPendingCall hook so SIGINT queues a Python
   callback raising KeyboardInterrupt. Upstream plpython (checked plpy_main.c, plpy_exec.c, plpy_spi.c on master) has no such machinery — a tight while
   True: loop or time.sleep() in upstream PL/Python is simply not interruptible at all; cancellation only happens when execution returns to C code.

   That mechanism changes the meaning of a second SIGINT from "set an already-set flag" to "queue a second callback" → double KeyboardInterrupt.

My suggestion to change it (It should also be examined. This is merely a proposal based on LLM, outlining how to rectify the issue.):

Every duplicate delivery — postmaster cancel (signal_child does kill(pid) + kill(-pid)), statement/lock timeout (group + direct) — arrives as repeated
SIGINTs into StatementCancelHandler, which calls the single cancel_pending_hook() (postgres.c:3933). That hook is PLy_handle_cancel_interrupt in
plpy_main.c — a file that is already permanently fork-divergent (it carries the "For GPDB Use" cancel machinery from GPDB #4988). Rebase cost there:
zero extra.

The fix replaces the upstream-file changes with queue-point deduplication:

  static volatile sig_atomic_t plpy_cancel_callback_queued = false;

  /* in PLy_handle_cancel_interrupt (signal-handler context): */
  if (PLy_enter_python_intepreter && !plpy_cancel_callback_queued)
  {
      if (Py_AddPendingCall(PLy_python_cancel_handler, NULL) == 0)
          plpy_cancel_callback_queued = true;
  }

  /* in PLy_python_cancel_handler (normal context): */
  plpy_cancel_callback_queued = false;
  if (!QueryCancelPending)   /* stale callback from an already-handled cancel */
      return 0;

Only the second delivery is dropped; the flag clears when the callback runs, so independent later cancels still work. sig_atomic_t keeps it
async-signal-safe (same safety class as the existing Py_AddPendingCall usage).

Resulting PR shape

   src/pl/plpython/plpy_main.c                    | 22 ++++++   (the fix — fork-divergent file)
   src/test/isolation2/sql/cancel_plpython.sql   | 43 +++++    (new test)
   src/test/isolation2/expected/cancel_plpython.out | 36 +++   (new expected output)

postmaster.c and postinit.c are byte-identical to upstream — future PG merges touch nothing here.

[krylosov-aa]

I'll update the PR as follows:

• Keep the fix in plpy_main.c: add the plpy_cancel_callback_queued flag as you suggested, and keep the QueryCancelPending check for stale callbacks.
• Revert postinit.c. The flag alone handles statement and lock timeouts: both kill() calls happen inside the SIGALRM handler, before Python can run the pending call, so the second SIGINT always finds the flag set.
• Revert postmaster.c.

The cancel-request path is already fixed upstream. In PG 18, cancel handling moved from processCancelRequest()/signal_child() to SendCancelRequest() in procsignal.c, which signals the backend's process group only once (REL_18_STABLE). Until then, our code keeps a narrow race on this path. If the backend runs the Python callback between the postmaster's kill(pid) and kill(-pid), the flag is already cleared, and the second SIGINT can still queue another KeyboardInterrupt.

I see two options for that:

1. A follow-up PR that makes processCancelRequest() signal the group exactly the way PG 18 SendCancelRequest() does. Since PG 18 replaces this function, the future rebase would resolve it by taking the upstream code.
2. Leave it as is and wait for the rebase onto PG 18.

Which would you prefer?