leborchuk opened a new pull request, #2038: URL: https://github.com/apache/cloudberry/pull/2038
Port fixes from https://github.com/apache/cloudberry/pull/2029 back to REL_2_STABLE branch Five issues from review of the PG16 port. Three share a root cause worth stating plainly: the collection path runs inside PG_TRY() from a signal handler, mixes C and C++ frames, and dismisses its own errors. Every non-local exit that skips cleanup therefore leaves damage behind in a backend that then keeps serving. 1. Leaked interrupt holdoff -- longjmp over RESUME_INTERRUPTS SendQueryState() holds interrupts for the whole collection, and build_plan_doc() took a second, nested hold. ExplainPrintPlan() can raise an error, and the longjmp out of it skips the inner RESUME_INTERRUPTS(); the PG_CATCH path that calls elog_dismiss(WARNING) then resumes only the caller's hold. InterruptHoldoffCount stays at 1 for the rest of the session and CHECK_FOR_INTERRUPTS() becomes a no-op, so the backend can no longer be cancelled or terminated. The re-throw path self-heals, because transaction abort resets the count -- the dismiss path, which is the common one here, does not. build_plan_doc() has exactly one caller, so drop the redundant pair rather than wrap it in PG_FINALLY, and assert the precondition so the coupling is checkable instead of implicit. 2. C++ exceptions escaping extern "C" gpsc_qs_sync_config(), gpsc_emit_node_batch() and gpsc_emit_query_plan() are reached from C on the signal path with no exception boundary. Config::sync(), protobuf construction and serialization, and UDSConnector::report_extended() -- which has no try/catch of its own and uses std::string plus an RAII socket guard -- can all throw. An exception crossing an extern "C" frame is undefined behaviour and in practice calls std::terminate(), taking the backend down. Add qs_emit_guard() and route all three entry points through it. cpp_call() in hook_wrappers.cpp cannot be reused as-is: it is a file-static template shaped for member-function pointers. It also differs deliberately in what it does once it has caught something. cpp_call() raises a PostgreSQL error, but an ereport(ERROR) longjmp from here would unwind C++ frames without running their destructors -- leaking the socket guard's fd -- and abandon the rest of the snapshot. This is best-effort telemetry taken while somebody else's query is mid-flight, so report at WARNING and return; a missing batch is just a missed sample. 3. Missing STRICT on the trace-bearing functions pg_query_state(), cbdb_mpp_query_state() and pg_query_state_backends() were declared without STRICT while every other function in both scripts has it. PG_GETARG_BYTEA_P() and PG_GETARG_ARRAYTYPE_P() do not consult isnull; they detoast a zero Datum, so passing NULL dereferences a null pointer instead of raising a controlled error. Mark all three STRICT in the fresh-install and the 1.1-to-1.2 script. 4. Unauthorized polling through the PUBLIC QE entry point cbdb_mpp_query_state() is granted to PUBLIC, carries no EXECUTE ON clause so it also runs on the coordinator, and selects targets by comparing the caller-supplied segid against GpIdentity.segindex -- which is -1 on the QD. An unprivileged session could therefore name (-1, victim_pid) directly and have another role's live plan collected and pushed to the UDS sink, bypassing the superuser-or-owner gate that pg_query_state() applies on the coordinator. It cannot simply be revoked, because CdbDispatchCommand() runs it on the QEs as the session user. Apply the same per-target check inside the function instead. This cannot reject a legitimate dispatch: the coordinator has already authorized the caller, and a query's QEs run under the same role as its coordinator backend. 5. Pending custom-signal flags not volatile sig_atomic_t CustomSignalPendings is written by procsignal_sigusr1_handler() and read by CheckAndHandleCustomSignals() in normal code -- the same contract as pss_signalFlags in the same file, which is volatile sig_atomic_t. As a plain bool array the compiler may keep a stale copy across the read and silently drop notifications. CustomSignalProcessing (a recursion guard) and CustomInterruptHandlers (set once at _PG_init) are never touched from the handler and stay ordinary variables; say so in a comment. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
