Copilot commented on code in PR #2038:
URL: https://github.com/apache/cloudberry/pull/2038#discussion_r4064441040
##########
gpcontrib/gp_stats_collector/src/pg_query_state/qs_types.h:
##########
@@ -69,7 +163,8 @@ typedef struct GpscNodeSample
int32_t plan_node_id; /* Plan.plan_node_id */
int32_t parent_plan_node_id; /* parent's plan_node_id, or
*
GPSC_NO_PARENT_PLAN_NODE_ID at the root */
- int32_t node_tag; /* nodeTag(plan) */
+ QsPlanNodeType node_type; /* qs_map_node_type(nodeTag(plan)); a
+ *
protocol value, not a raw NodeTag */
Review Comment:
If `GpscNodeSample` is part of a binary/shared-memory interface, storing
`node_type` as a C `enum` can be less robust than a fixed-width integer: enum
underlying size is compiler/flags dependent (e.g., `-fshort-enums`). To make
the sample struct layout unambiguous, consider storing this as `int32_t`
(protocol code) and casting at the edges, or add a compile-time assertion that
`sizeof(QsPlanNodeType) == sizeof(int32_t)` in the relevant compilation units.
##########
src/backend/commands/explain.c:
##########
@@ -2967,8 +2967,15 @@ ExplainNode(PlanState *planstate, List *ancestors,
if (es->wal && planstate->instrument)
show_wal_usage(es, &planstate->instrument->walusage);
- /* Show worker detail after query execution */
- if (es->analyze && es->verbose && planstate->worker_instrument
+ /*
+ * Prepare per-worker buffer/WAL usage, after query execution.
+ *
+ * es->workers_state is NULL when per-worker detail is hidden (see
+ * es->hide_workers), and ExplainOpenWorker() below requires it, so
testing
+ * it is what keeps this safe -- planstate->worker_instrument alone is
not
+ * enough.
+ */
+ if (es->workers_state && (es->buffers || es->wal) && es->verbose
&& !es->runtime)
Review Comment:
This block no longer checks `planstate->worker_instrument` before
dereferencing/using it (it’s assigned immediately after the condition).
Previously the condition included `planstate->worker_instrument` (per the
removed code), so this change can make EXPLAIN crash if `worker_instrument` is
NULL while `es->workers_state` is non-NULL. Add `planstate->worker_instrument`
back into the `if` condition (or otherwise guard the block) so the subsequent
code can’t run with a NULL worker instrumentation pointer.
##########
src/backend/commands/explain.c:
##########
@@ -4505,16 +4512,27 @@ show_instrumentation_count(const char *qlabel, int
which,
if (!es->analyze || !planstate->instrument)
return;
- nloops = planstate->instrument->nloops;
+
if (which == 2)
- nfiltered = ((nloops > 0) ? planstate->instrument->nfiltered2 /
nloops : 0);
+ nfiltered = planstate->instrument->nfiltered2;
else
- nfiltered = ((nloops > 0) ? planstate->instrument->nfiltered1 /
nloops : 0);
+ nfiltered = planstate->instrument->nfiltered1;
nloops = planstate->instrument->nloops;
- /* In text mode, suppress zero counts; they're not interesting enough */
+ /*
+ * In text mode, suppress zero counts; they're not interesting enough.
+ *
+ * The nloops == 0 case is what runtime mode hits for the whole of the
first
+ * loop, so the counters cannot be averaged there; report 0 rather than
+ * dividing by zero.
+ */
if (nfiltered > 0 || es->format != EXPLAIN_FORMAT_TEXT)
- ExplainPropertyFloat(qlabel, NULL, nfiltered, 0, es);
+ {
+ if (nloops > 0)
+ ExplainPropertyFloat(qlabel, NULL, nfiltered / nloops,
0, es);
+ else
+ ExplainPropertyFloat(qlabel, NULL, 0.0, 0, es);
+ }
Review Comment:
The suppression predicate now uses the raw (non-averaged) `nfiltered`, but
the printed value is `nfiltered / nloops` (or `0.0` when `nloops == 0`). This
can emit a 0.0 property in TEXT format when `nfiltered > 0` but `nloops == 0`,
which contradicts the stated intent to 'suppress zero counts' and is a behavior
change from the prior code (which would suppress in that case). Compute the
value-to-report first (respecting the `nloops == 0` case), then base the
suppression check on the value actually printed.
##########
gpcontrib/gp_stats_collector/src/pg_query_state/signal_handler.c:
##########
@@ -222,13 +222,110 @@ send_msg_by_parts(shm_mq_handle *mqh, Size nbytes, const
void *data)
return MSG_BY_PARTS_SUCCEEDED;
}
+/*
+ * qs_map_node_type -- translate a NodeTag into the wire protocol's stable
+ * QsPlanNodeType.
+ *
+ * NodeTag numbering is a PostgreSQL implementation detail and is not stable
+ * across major versions: PG16 generates it with gen_node_support.pl
+ * (src/include/nodes/nodetags.h), which renumbered every tag relative to PG14
+ * (T_SeqScan 27 -> 395). Sending nodeTag(plan) raw therefore makes every node
+ * unresolvable on a receiver holding a table for the other version. Map
+ * explicitly so the protocol is decoupled from the backend's numbering.
+ *
+ * The cases below mirror the node-name switch in ExplainNode(); anything not
+ * listed is reported as UNSPECIFIED rather than guessed. QS_PLAN_NODE_TYPE_
+ * SPLIT_MERGE has no PostgreSQL 14 counterpart, so it is reserved in the enum
+ * but has no case here.
+ */
+static QsPlanNodeType
+qs_map_node_type(NodeTag tag)
+{
+ switch (tag)
+ {
+ /* control nodes */
+ case T_Result: return
QS_PLAN_NODE_TYPE_RESULT;
+ case T_ProjectSet: return
QS_PLAN_NODE_TYPE_PROJECT_SET;
+ case T_ModifyTable: return
QS_PLAN_NODE_TYPE_MODIFY_TABLE;
+ case T_Append: return
QS_PLAN_NODE_TYPE_APPEND;
+ case T_MergeAppend: return
QS_PLAN_NODE_TYPE_MERGE_APPEND;
+ case T_RecursiveUnion: return
QS_PLAN_NODE_TYPE_RECURSIVE_UNION;
+ case T_BitmapAnd: return
QS_PLAN_NODE_TYPE_BITMAP_AND;
+ case T_BitmapOr: return
QS_PLAN_NODE_TYPE_BITMAP_OR;
+
+ /* scans */
+ case T_SeqScan: return
QS_PLAN_NODE_TYPE_SEQ_SCAN;
+ case T_SampleScan: return
QS_PLAN_NODE_TYPE_SAMPLE_SCAN;
+ case T_IndexScan: return
QS_PLAN_NODE_TYPE_INDEX_SCAN;
+ case T_IndexOnlyScan: return
QS_PLAN_NODE_TYPE_INDEX_ONLY_SCAN;
+ case T_BitmapIndexScan: return
QS_PLAN_NODE_TYPE_BITMAP_INDEX_SCAN;
+ case T_BitmapHeapScan: return
QS_PLAN_NODE_TYPE_BITMAP_HEAP_SCAN;
+ case T_TidScan: return
QS_PLAN_NODE_TYPE_TID_SCAN;
+ case T_TidRangeScan: return
QS_PLAN_NODE_TYPE_TID_RANGE_SCAN;
+ case T_SubqueryScan: return
QS_PLAN_NODE_TYPE_SUBQUERY_SCAN;
+ case T_FunctionScan: return
QS_PLAN_NODE_TYPE_FUNCTION_SCAN;
+ case T_TableFuncScan: return
QS_PLAN_NODE_TYPE_TABLE_FUNC_SCAN;
+ case T_ValuesScan: return
QS_PLAN_NODE_TYPE_VALUES_SCAN;
+ case T_CteScan: return
QS_PLAN_NODE_TYPE_CTE_SCAN;
+ case T_NamedTuplestoreScan: return
QS_PLAN_NODE_TYPE_NAMED_TUPLESTORE_SCAN;
+ case T_WorkTableScan: return
QS_PLAN_NODE_TYPE_WORK_TABLE_SCAN;
+ case T_ForeignScan: return
QS_PLAN_NODE_TYPE_FOREIGN_SCAN;
+ case T_CustomScan: return
QS_PLAN_NODE_TYPE_CUSTOM_SCAN;
+
+ /* joins */
+ case T_NestLoop: return
QS_PLAN_NODE_TYPE_NEST_LOOP;
+ case T_MergeJoin: return
QS_PLAN_NODE_TYPE_MERGE_JOIN;
+ case T_HashJoin: return
QS_PLAN_NODE_TYPE_HASH_JOIN;
+
+ /* materialization, ordering, grouping */
+ case T_Material: return
QS_PLAN_NODE_TYPE_MATERIAL;
+ case T_Memoize: return
QS_PLAN_NODE_TYPE_MEMOIZE;
+ case T_Sort: return
QS_PLAN_NODE_TYPE_SORT;
+ case T_IncrementalSort: return
QS_PLAN_NODE_TYPE_INCREMENTAL_SORT;
+ case T_Group: return
QS_PLAN_NODE_TYPE_GROUP;
+ case T_Agg: return
QS_PLAN_NODE_TYPE_AGG;
+ case T_WindowAgg: return
QS_PLAN_NODE_TYPE_WINDOW_AGG;
+ case T_Unique: return
QS_PLAN_NODE_TYPE_UNIQUE;
+ case T_Hash: return
QS_PLAN_NODE_TYPE_HASH;
+ case T_SetOp: return
QS_PLAN_NODE_TYPE_SET_OP;
+ case T_LockRows: return
QS_PLAN_NODE_TYPE_LOCK_ROWS;
+ case T_Limit: return
QS_PLAN_NODE_TYPE_LIMIT;
+
+ /* intra-node parallelism */
+ case T_Gather: return
QS_PLAN_NODE_TYPE_GATHER;
+ case T_GatherMerge: return
QS_PLAN_NODE_TYPE_GATHER_MERGE;
+
+ /* Cloudberry MPP nodes */
+ case T_Motion: return
QS_PLAN_NODE_TYPE_MOTION;
+ case T_Sequence: return
QS_PLAN_NODE_TYPE_SEQUENCE;
+ case T_ShareInputScan: return
QS_PLAN_NODE_TYPE_SHARE_INPUT_SCAN;
+ case T_SplitUpdate: return
QS_PLAN_NODE_TYPE_SPLIT_UPDATE;
+ case T_AssertOp: return
QS_PLAN_NODE_TYPE_ASSERT_OP;
+ case T_PartitionSelector: return
QS_PLAN_NODE_TYPE_PARTITION_SELECTOR;
+ case T_RuntimeFilter: return
QS_PLAN_NODE_TYPE_RUNTIME_FILTER;
+ case T_TupleSplit: return
QS_PLAN_NODE_TYPE_TUPLE_SPLIT;
+ case T_TableFunctionScan: return
QS_PLAN_NODE_TYPE_TABLE_FUNCTION_SCAN;
+ case T_DynamicSeqScan: return
QS_PLAN_NODE_TYPE_DYNAMIC_SEQ_SCAN;
+ case T_DynamicIndexScan: return
QS_PLAN_NODE_TYPE_DYNAMIC_INDEX_SCAN;
+ case T_DynamicIndexOnlyScan: return
QS_PLAN_NODE_TYPE_DYNAMIC_INDEX_ONLY_SCAN;
+ case T_DynamicBitmapIndexScan: return
QS_PLAN_NODE_TYPE_DYNAMIC_BITMAP_INDEX_SCAN;
+ case T_DynamicBitmapHeapScan: return
QS_PLAN_NODE_TYPE_DYNAMIC_BITMAP_HEAP_SCAN;
+ case T_DynamicForeignScan: return
QS_PLAN_NODE_TYPE_DYNAMIC_FOREIGN_SCAN;
+
+ default:
+ elog(DEBUG1, "pg_query_state: unmapped plan NodeTag
%d", (int) tag);
+ return QS_PLAN_NODE_TYPE_UNSPECIFIED;
Review Comment:
Logging per unmapped node tag can spam logs in environments where DEBUG1 is
enabled, especially since mapping runs per plan node during sampling. Consider
rate-limiting (e.g., log once per `NodeTag` value per process) or removing the
log entirely and relying on `UNSPECIFIED` as a silent fallback.
--
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]
