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.
Comment on lines
+127
to
+130
| CREATE FUNCTION gpsc.pg_query_state(pid int, trace_id bytea) | ||
| RETURNS SETOF void | ||
| AS 'MODULE_PATHNAME', 'pg_query_state' | ||
| LANGUAGE C VOLATILE EXECUTE ON COORDINATOR; |
Comment on lines
+84
to
+88
| extern "C" void | ||
| gpsc_emit_node_batch(GpscNodeSample **nodes, int count, const char *trace_id) | ||
| { | ||
| if (count <= 0) | ||
| return; |
Comment on lines
+1120
to
+1125
| /* Stamp the target's own trace slot before signalling it. */ | ||
| memcpy(qs_trace_slots[proc->backendId], VARDATA_ANY(trace_id), | ||
| GPSC_TRACE_ID_LEN); | ||
|
|
||
| sig_result = SendProcSignal(proc->pid, QueryStatePollReason, | ||
| proc->backendId); |
| if (queryDesc == NULL || Gp_role != GP_ROLE_DISPATCH) | ||
| return NULL; | ||
|
|
||
| HOLD_INTERRUPTS(); |
Comment on lines
+107
to
+109
| static bool CustomSignalPendings[NUM_CUSTOM_PROCSIGNALS]; | ||
| static bool CustomSignalProcessing[NUM_CUSTOM_PROCSIGNALS]; | ||
| static ProcSignalHandler_type CustomInterruptHandlers[NUM_CUSTOM_PROCSIGNALS]; |
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