Copilot commented on code in PR #1762:
URL: https://github.com/apache/cloudberry/pull/1762#discussion_r3407895797
##########
src/include/utils/guc.h:
##########
@@ -247,6 +247,14 @@ typedef enum
#define GUC_GPDB_NEED_SYNC 0x00400000 /* guc value is synced between
master and primary */
#define GUC_GPDB_NO_SYNC 0x00800000 /* guc value is not synced between
master and primary */
+/*
+ * FIXEM: we have bad logic in get_explain_guc_options
+ * Even GUCs have no GUC_EXPLAIN flag, explain(verbose) still show them.
+ * It's a bug. However, there would be much more plan diffs if we fix it now.
+ * So introduce a temp fix flag to workaround for new added GUCs which are not
showed in explain.
+ */
+#define GUC_NO_EXPLAIN 0x01000000 /* guc value is not synced between
master and primary */
Review Comment:
The comment introducing GUC_NO_EXPLAIN contains a typo ("FIXEM") and the
macro comment is a copy/paste mismatch (it says "not synced" rather than "not
shown in EXPLAIN"). This makes it harder to understand what the flag actually
does.
##########
src/backend/utils/misc/guc_gp.c:
##########
@@ -1883,8 +1893,7 @@ struct config_bool ConfigureNamesBool_gp[] =
{
{"gp_eager_two_phase_agg", PGC_USERSET, QUERY_TUNING_METHOD,
gettext_noop("Eager two stage agg."),
- NULL,
- GUC_NO_SHOW_ALL | GUC_NOT_IN_SAMPLE
+ NULL
},
Review Comment:
gp_eager_two_phase_agg no longer has the GUC_NO_SHOW_ALL | GUC_NOT_IN_SAMPLE
flags, unlike nearby internal tuning GUCs. This changes user-visible behavior
(SHOW ALL / sample configs) and is inconsistent with the surrounding
conventions.
##########
src/backend/optimizer/util/clauses.c:
##########
@@ -849,7 +849,7 @@ max_parallel_hazard_walker(Node *node,
max_parallel_hazard_context *context)
List *save_safe_param_ids;
if (!subplan->parallel_safe &&
- max_parallel_hazard_test(PROPARALLEL_RESTRICTED,
context))
+ max_parallel_hazard_test(PROPARALLEL_SAFE, context))
return true;
Review Comment:
In max_parallel_hazard_walker(), this check now uses PROPARALLEL_SAFE, which
flips the meaning: it will report a hazard only when the caller is scanning for
SAFE hazards, instead of when scanning for RESTRICTED hazards. This can let
non-parallel-safe SubPlans be treated as safe/restricted incorrectly during
planning.
--
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]
