Conversation
Adds a signal-based runtime query-state facility to the gp_stats_collector extension. It lets a session inspect the live execution state of another running backend on demand - walking its active plan tree across the QD and all QEs - without waiting for the query to finish, pushing batches to the UDS(unix domain socket). New SQL API (extension v1.2, schema gpsc): gpsc.pg_query_state(pid, trace_id) - fan out a poll to the query running on pid; each participating backend walks its plan tree and logs a per-node snapshot. gpsc.pg_query_state_backends(pid) - list the (segid, pid) QE backends taking part in that query. cbdb_mpp_query_state(gpsc.gp_segment_pid[], trace_id) - QE-side dispatch target. The extension embeds the pg_query_state signal layer, which depends on three PostgreSQL core changes folded directly into the tree (configure enables the extension by default, so the tree must build without a manual patch step): custom ProcSignal handlers (procsignal.c/.h, postgres.c); end-of-node instrumentation flag readable mid-run (instrument.c/.h); runtime EXPLAIN entry points (explain.c/.h). --------- Co-authored-by: Dianjin Wang <wangdianjin@gmail.com>
The pg_query_state feature (ad77767) was cherry-picked from REL_2_STABLE and applied verbatim: apart from the LICENSE hunk and one getopt character, every hunk is identical to the PG 14.9 original. The tree builds, but the PG15/16 deltas inside the functions the patch rewrites were never accounted for. MERGE (new in PG15) show_modifytable_info() has two InstrEndLoop() call sites on PG16. The patch guarded the ON CONFLICT one with !es->runtime and left the CMD_MERGE one bare. Against a running MERGE that either raises "InstrEndLoop called on running node" (losing the plan document) or, when the outer node is between tuples, succeeds and destroys the live query's instrumentation, so the user's own EXPLAIN ANALYZE output comes out wrong. The adjacent Assert(skipped_path >= 0) also fires, since mid-flight ntuples excludes the in-progress loop. Guard the call and report only the action counters in runtime mode; non-runtime output is unchanged. filter_query() likewise listed only SELECT/INSERT/UPDATE/DELETE, which was exhaustive on PG14. On PG16 MERGE fell through and was never instrumented -- adding it is what makes the fix above reachable. Bugs carried over from the original patch ExplainNode() lost its es->workers_state test when the patch rewrote the per-worker buffer/WAL condition. That field is NULL whenever per-worker detail is hidden (es->hide_workers), and ExplainOpenWorker() requires it, so a plain EXPLAIN (ANALYZE, VERBOSE) under debug_parallel_query= regress dereferenced NULL. Restore the original condition and add only the !es->runtime term the patch actually needed. show_instrumentation_count() was rewritten to divide by nloops before the text-mode suppression test. Upstream already handles nloops == 0 -- exactly the runtime-mode case -- so the rewrite bought nothing while changing stock EXPLAIN output and leaving a dead second assignment to nloops. Reverted. qs_planstate_walker() did not descend into SequenceState->subplans[]. Those children are unreachable via outerPlan/innerPlan, so every node beneath a Sequence -- emitted for partitioned and dynamic-scan plans -- was silently absent from per-node batches. qs_debug_node_sample() used %lu/%ld for uint64_t/int64_t; use UINT64_FORMAT/INT64_FORMAT so the build is clean off Linux x86-64. Port fidelity The LICENSE hunk reconstructed a gpcontrib/yezzey/* entry from the REL_2_STABLE context. Neither that directory nor licenses/LICENSE-yezzey.txt exists on main, so drop it and keep only the pg_query_state attribution. Drop the -Z hunk in process_postgres_switches() entirely. It is unrelated to this feature, its comment ("for consistency with the postmaster") is false -- postmaster.c has no -Z in either branch -- and nothing in the tree passes the option. The port had also silently changed it from "Z" to "Z:". Also port ef0b024, an ExplainNode() NULL-planstate guard that landed on REL_2_STABLE only. With alien elimination on (execute_pruned_plan), a QE leaves the child of a receiving Motion and any subplan unreachable from its local slice uninitialized, so outerPlanState() and SubPlanState.planstate can be NULL while the corresponding Plan is not. auto_explain runs on QEs and walks the local plan tree, which crashes without this. build_plan_doc() now enforces its QD-only restriction itself rather than leaving it to the caller. Finally, document that InstrAggNode() deliberately does not merge the new Instrumentation.eof field: every caller has already run InstrEndLoop() on the source, which clears it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The review found signal-state, interrupt-cleanup, authorization, null-input, and C++ exception-handling issues that can cause missed polls, backend instability, or unauthorized collection.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Ports runtime query-progress collection to PG16, integrating core signal/instrumentation support with gp_stats_collector, UDS protobuf reporting, tests, and CI.
Changes:
- Adds custom process signals and runtime instrumentation.
- Adds
pg_query_stateAPIs and UDS/protobuf emitters. - Adds regression, isolation, crash, and CI coverage.
File summaries
| File | Description |
|---|---|
src/include/storage/procsignal.h |
Custom signal API |
src/include/executor/instrument.h |
Runtime EOF instrumentation |
src/include/commands/explain.h |
Runtime explain flag |
src/backend/tcop/postgres.c |
Custom signal processing |
src/backend/storage/ipc/procsignal.c |
Custom signal dispatch |
src/backend/executor/instrument.c |
EOF tracking |
src/backend/commands/explain.c |
Runtime explain output |
pom.xml |
RAT exclusions |
licenses/LICENSE-pg_query_state.txt |
Third-party license |
LICENSE |
License inventory entry |
gpcontrib/gp_stats_collector/test/sql/gpsc_pg_query_state.sql |
SQL API tests |
gpcontrib/gp_stats_collector/test/Makefile |
Regression test build |
gpcontrib/gp_stats_collector/test/isolation2/sql/setup.sql |
Isolation setup |
gpcontrib/gp_stats_collector/test/isolation2/sql/gpsc_pqs_seg_count.sql |
Segment-count test |
gpcontrib/gp_stats_collector/test/isolation2/sql/gpsc_pqs_running.sql |
Running-query test |
gpcontrib/gp_stats_collector/test/isolation2/sql/gpsc_pqs_perms.sql |
Permission tests |
gpcontrib/gp_stats_collector/test/isolation2/sql/gpsc_pqs_disabled.sql |
Disabled-feature test |
gpcontrib/gp_stats_collector/test/isolation2/sql/gpsc_pqs_backends.sql |
Idle-backend test |
gpcontrib/gp_stats_collector/test/isolation2/Makefile |
Isolation test build |
gpcontrib/gp_stats_collector/test/isolation2/isolation2_schedule |
Isolation schedule |
gpcontrib/gp_stats_collector/test/isolation2/expected/setup.out |
Setup expected output |
gpcontrib/gp_stats_collector/test/isolation2/expected/gpsc_pqs_seg_count.out |
Segment-count expected output |
gpcontrib/gp_stats_collector/test/isolation2/expected/gpsc_pqs_running.out |
Running-query expected output |
gpcontrib/gp_stats_collector/test/isolation2/expected/gpsc_pqs_perms.out |
Permission expected output |
gpcontrib/gp_stats_collector/test/isolation2/expected/gpsc_pqs_disabled.out |
Disabled-feature expected output |
gpcontrib/gp_stats_collector/test/isolation2/expected/gpsc_pqs_backends.out |
Idle-backend expected output |
gpcontrib/gp_stats_collector/test/isolation2/.gitignore |
Test artifacts |
gpcontrib/gp_stats_collector/test/expected/gpsc_pg_query_state.out |
SQL test expected output |
gpcontrib/gp_stats_collector/test/crash/uds_drain.py |
UDS test sink |
gpcontrib/gp_stats_collector/test/crash/README.md |
Crash-test documentation |
gpcontrib/gp_stats_collector/test/crash/poller.py |
Runtime query poller |
gpcontrib/gp_stats_collector/test/crash/extract_failures.sh |
Failure extraction |
gpcontrib/gp_stats_collector/test/crash/crash_scan.sh |
Crash health checks |
gpcontrib/gp_stats_collector/src/UDSConnector.h |
Extended UDS API |
gpcontrib/gp_stats_collector/src/UDSConnector.cpp |
Extended UDS transport |
gpcontrib/gp_stats_collector/src/PlanNodeEmitter.h |
Emitter interface |
gpcontrib/gp_stats_collector/src/PlanNodeEmitter.cpp |
Protobuf emission |
gpcontrib/gp_stats_collector/src/pg_query_state/signal_handler.c |
Query-state collection |
gpcontrib/gp_stats_collector/src/pg_query_state/README.md |
Query-state design |
gpcontrib/gp_stats_collector/src/pg_query_state/qs_types.h |
Sample data types |
gpcontrib/gp_stats_collector/src/pg_query_state/pg_query_state.h |
Query-state declarations |
gpcontrib/gp_stats_collector/src/pg_query_state/pg_query_state.c |
APIs and executor hooks |
gpcontrib/gp_stats_collector/src/gp_stats_collector.c |
Module initialization |
gpcontrib/gp_stats_collector/README.md |
User-facing documentation |
gpcontrib/gp_stats_collector/protos/yagpcc_set_per_node.proto |
Per-node protobuf schema |
gpcontrib/gp_stats_collector/protos/yagpcc_plan.proto |
Plan protobuf schema |
gpcontrib/gp_stats_collector/protos/yagpcc_metrics.proto |
Metrics protobuf schema |
gpcontrib/gp_stats_collector/Makefile |
Protobuf and object build |
gpcontrib/gp_stats_collector/gp_stats_collector.control |
Version 1.2 |
gpcontrib/gp_stats_collector/gp_stats_collector--1.2.sql |
New extension objects |
gpcontrib/gp_stats_collector/gp_stats_collector--1.1--1.2.sql |
Upgrade objects |
gpcontrib/gp_stats_collector/docs/pg_query_state_dataflow.puml |
Data-flow diagram |
.github/workflows/gpsc-crash-test.yaml |
Crash-test workflow |
.github/workflows/build-deb-cloudberry.yml |
Debian test integration |
.github/workflows/build-cloudberry.yml |
Main CI test integration |
Review details
- Files reviewed: 55/55 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Every plan node was reported as "unknown" by the collector on a real PG16 cluster. BatchNode.node_type carried nodeTag(plan) verbatim. NodeTag numbering is an internal PostgreSQL detail with no cross-version stability guarantee, and PG16 generates the enum with gen_node_support.pl (src/include/nodes/nodetags.h) rather than maintaining it by hand. That renumbered all 547 tags shared with PG14 -- T_SeqScan 27 -> 395, T_HashJoin 53 -> 424, T_Motion 71 -> 443 -- so a receiver holding a PG14-era numeric table missed on every node. The feature was ported from REL_2_STABLE with this field untouched, so the breakage only appeared once it ran against a PG16 backend. Introduce a PlanNodeType enum owned by the protocol and map NodeTag onto it in qs_map_node_type(), mirroring the QsNodeStatus -> PlanNodeStatus mapping already used for node_status. The 58 members cover exactly the node types ExplainNode() can name in this tree, including T_SplitMerge, which is new in PG16 and has no PG14 equivalent. Unmapped tags report UNSPECIFIED and log at DEBUG1 instead of leaking a raw tag. The wire encoding is unchanged -- proto3 enums are varints, like the int32 they replace -- but the values are not, so the yagpcc receiver must switch to PlanNodeType in lockstep. In exchange, set_node_type() is now typed, so a raw NodeTag can no longer reach the wire by accident. yagpcc_plan.proto's PlanNode.node_type is left as-is: that message is unused by the extension, so changing it would desync a definition nothing here can exercise. Its comment now warns to convert it before use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
leborchuk
marked this pull request as ready for review
September 21, 2026 16:24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is PG16 port changes from #1934
together with fixes found out in manual testing:
Stop shipping raw NodeTag values in per-node samples
Harden pg_query_state against non-local exits and unauthorized polling