Copilot commented on code in PR #1855:
URL: https://github.com/apache/cloudberry/pull/1855#discussion_r3740119064
##########
src/test/modules/parallel_customscan/Makefile:
##########
@@ -0,0 +1,29 @@
+# src/test/modules/parallel_customscan/Makefile
+
+MODULE_big = parallel_customscan
+OBJS = \
+ $(WIN32RES) \
+ parallel_customscan.o
+
+EXTENSION = parallel_customscan
+DATA = parallel_customscan--1.0.sql
+PGFILEDESC = "parallel_customscan - exercise parallel CustomScan dispatch"
+
+REGRESS = parallel_customscan
+
+# Run against an existing cluster (gpdemo). The cluster must have
+# 'parallel_customscan' in shared_preload_libraries so segment backends
+# and parallel workers have the CustomScan methods registered:
+# gpconfig -c shared_preload_libraries -v "'time_series,parallel_customscan'"
+# gpstop -ra
Review Comment:
The regression test requires the module to be present in
`shared_preload_libraries` (as noted here) so that segment backends / parallel
workers have the CustomScan methods registered. However, this Makefile doesn't
currently set `REGRESS_OPTS = --temp-config ...` (like `worker_spi` /
`test_slru`) or otherwise ensure that configuration during `make installcheck`,
so the test can fail unless the environment was manually preconfigured.
Consider adding a temp-config file that sets `shared_preload_libraries =
parallel_customscan` and wiring it via `REGRESS_OPTS`, or marking the module
`NO_INSTALLCHECK = 1` if it must run only against preconfigured clusters.
##########
src/test/modules/parallel_customscan/parallel_customscan.c:
##########
@@ -0,0 +1,396 @@
+#include "postgres.h"
Review Comment:
This new test module C file doesn't follow the common src/test/modules
convention of starting with a header comment block (file purpose +
IDENTIFICATION). Most other modules include that metadata before the first
`#include`, which helps with provenance and tooling that relies on
IDENTIFICATION paths.
##########
src/test/modules/parallel_customscan/parallel_customscan.c:
##########
@@ -0,0 +1,396 @@
+#include "postgres.h"
+
+#include "access/relscan.h"
+#include "access/tableam.h"
+#include "catalog/pg_type.h"
+#include "executor/executor.h"
+#include "fmgr.h"
+#include "funcapi.h"
+#include "miscadmin.h"
+#include "nodes/extensible.h"
+#include "nodes/makefuncs.h"
+#include "optimizer/optimizer.h"
+#include "cdb/cdbpathlocus.h"
+#include "optimizer/pathnode.h"
+#include "optimizer/paths.h"
+#include "optimizer/restrictinfo.h"
+#include "port/atomics.h"
+#include "storage/shm_toc.h"
+#include "utils/builtins.h"
+#include "utils/guc.h"
+#include "utils/rel.h"
+
+PG_MODULE_MAGIC;
+
+void _PG_init(void);
+void _PG_fini(void);
+
+PG_FUNCTION_INFO_V1(pcs_get_hook_calls);
+
+/* GUC */
+static bool pcs_enabled = false;
+
+/*
+ * Per-process hook-call counters. Reset in EstimateDSMCustomScan (which is
+ * called once per parallel-mode invocation in the leader). The cross-worker
+ * InitializeWorkerCustomScan count is aggregated into the DSM atomic and
+ * harvested back into the leader-local counter in ShutdownCustomScan.
+ */
+static int64 pcs_n_estimate = 0;
+static int64 pcs_n_init_dsm = 0;
+static int64 pcs_n_reinit_dsm = 0;
+static int64 pcs_n_init_worker = 0;
+static int64 pcs_n_shutdown = 0;
+
+/*
+ * DSM header. The child SeqScan owns the parallel table scan descriptor, so
+ * the wrapper only needs a tiny shared area to count InitializeWorker calls
+ * across workers.
+ */
+typedef struct PcsDSM
+{
+ pg_atomic_uint32 init_worker_calls;
+} PcsDSM;
+
+typedef struct PcsState
+{
+ CustomScanState csstate;
+ PcsDSM *dsm; /* set in InitDSM /
InitWorker */
+} PcsState;
+
+static set_rel_pathlist_hook_type prev_pathlist_hook = NULL;
+
+static Plan *pcs_plan_path(PlannerInfo *root, RelOptInfo *rel,
+ CustomPath *best_path, List
*tlist,
+ List *clauses, List
*custom_plans);
+static Node *pcs_create_state(CustomScan *cscan);
+static void pcs_begin(CustomScanState *node, EState *estate, int eflags);
+static TupleTableSlot *pcs_exec(CustomScanState *node);
+static void pcs_end(CustomScanState *node);
+static void pcs_rescan(CustomScanState *node);
+static Size pcs_estimate_dsm(CustomScanState *node, ParallelContext *pcxt);
+static void pcs_init_dsm(CustomScanState *node, ParallelContext *pcxt,
+ void *coord);
+static void pcs_reinit_dsm(CustomScanState *node, ParallelContext *pcxt,
+ void *coord);
+static void pcs_init_worker(CustomScanState *node, shm_toc *toc, void *coord);
+static void pcs_shutdown(CustomScanState *node);
+
+static const CustomPathMethods pcs_path_methods =
+{
+ .CustomName = "ParallelCustomScan",
+ .PlanCustomPath = pcs_plan_path,
+};
+
+static const CustomScanMethods pcs_scan_methods =
+{
+ .CustomName = "ParallelCustomScan",
+ .CreateCustomScanState = pcs_create_state,
+};
+
+static const CustomExecMethods pcs_exec_methods =
+{
+ .CustomName = "ParallelCustomScan",
+ .BeginCustomScan = pcs_begin,
+ .ExecCustomScan = pcs_exec,
+ .EndCustomScan = pcs_end,
+ .ReScanCustomScan = pcs_rescan,
+ .EstimateDSMCustomScan = pcs_estimate_dsm,
+ .InitializeDSMCustomScan = pcs_init_dsm,
+ .ReInitializeDSMCustomScan = pcs_reinit_dsm,
+ .InitializeWorkerCustomScan = pcs_init_worker,
+ .ShutdownCustomScan = pcs_shutdown,
+};
+
+static Plan *
+pcs_plan_path(PlannerInfo *root, RelOptInfo *rel, CustomPath *best_path,
+ List *tlist, List *clauses, List *custom_plans)
+{
+ CustomScan *cs = makeNode(CustomScan);
+
+ cs->scan.plan.targetlist = tlist;
+ /*
+ * The child SeqScan does the filtering (create_scan_plan attaches the
+ * base restrictions to it), so the wrapper itself carries no qual.
+ */
+ cs->scan.plan.qual = NIL;
+ cs->scan.plan.parallel_aware = best_path->path.parallel_aware;
+ cs->scan.plan.parallel_safe = best_path->path.parallel_safe;
+ /*
+ * The wrapper delegates scanning to its child, so it is not itself a
+ * base-relation scan: scanrelid = 0. custom_scan_tlist must then
+ * describe the scan tuple we hand upward; we mirror the child's output
+ * targetlist, and set_customscan_references() rewrites our own
+ * targetlist to reference it via INDEX_VAR. This keeps projection
+ * correct for multi-column relations -- e.g. the catalog scans
+ * (pg_class) that ANALYZE issues internally, which a fixed
+ * base-relation descriptor would mis-deform.
+ */
+ cs->scan.scanrelid = 0;
+ cs->flags = best_path->flags;
+ cs->custom_plans = custom_plans;
+ cs->custom_exprs = NIL;
+ cs->custom_private = NIL;
+ cs->custom_scan_tlist =
+ copyObject(((Plan *) linitial(custom_plans))->targetlist);
+ cs->methods = &pcs_scan_methods;
+
+ return (Plan *) cs;
+}
+
+static Node *
+pcs_create_state(CustomScan *cscan)
+{
+ PcsState *st = (PcsState *) newNode(sizeof(PcsState),
T_CustomScanState);
+
+ st->csstate.methods = &pcs_exec_methods;
+ return (Node *) st;
+}
+
+static void
+pcs_begin(CustomScanState *node, EState *estate, int eflags)
+{
+ CustomScan *cscan = (CustomScan *) node->ss.ps.plan;
+ Plan *childplan = (Plan *) linitial(cscan->custom_plans);
+
+ /*
+ * ExecInitCustomScan has already opened the scan relation and set up
our
+ * scan/result slots and projection. All we add is the child plan
state,
+ * which becomes our sole custom_ps entry. That child performs the
actual
+ * (parallel) heap scan and is the node the MPP planstate walkers
recurse
+ * into via the T_CustomScanState arm.
+ */
+ node->custom_ps = list_make1(ExecInitNode(childplan, estate, eflags));
+}
+
+static TupleTableSlot *
+pcs_child_next(CustomScanState *node)
+{
+ PlanState *child = (PlanState *) linitial(node->custom_ps);
+ TupleTableSlot *childslot = ExecProcNode(child);
+
+ if (TupIsNull(childslot))
+ return NULL;
Review Comment:
`pcs_child_next()` returns `NULL` when the child scan is exhausted. While
`TupIsNull()` treats `NULL` as empty, returning a null slot pointer can bypass
`ExecScan`'s empty-slot normalization when there is no qual/projection, and it
is inconsistent with other `ExecScan` access methods which always return a
(possibly-cleared) slot with a valid tuple descriptor. Return a cleared scan
slot instead to ensure callers always receive a non-NULL `TupleTableSlot *`.
--
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]
