Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move optimizer_enable_query_parameter GUC to Orca extension #1172

Open
wants to merge 5 commits into
base: feature/ADBDEV-6552
Choose a base branch
from
Open

Move optimizer_enable_query_parameter GUC to Orca extension #1172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4d5dc89
Add choose_custom_plan_hook
whitehawk Dec 25, 2024
583ecc3
Merge branch 'feature/ADBDEV-6552' into ADBDEV-6630
whitehawk Dec 26, 2024
93a84d0
Move GUC optimizer_enable_query_parameter to the extension
whitehawk Dec 26, 2024
2b7632b
Cleanup unsync_guc_name.h
whitehawk Dec 26, 2024
65a944c
Reorder checks in choose_custom_plan()
whitehawk Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .abi-check/7.2.0_arenadata7/postgres.symbols.ignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ optimizer_enable_range_predicate_dpe
optimizer_enable_push_join_below_union_all
optimizer_use_gpdb_allocators
optimizer_partition_selection_log
optimizer_enable_query_parameter
ConfigureNamesBool_gp
ConfigureNamesEnum_gp
ConfigureNamesInt_gp
Expand Down
1 change: 1 addition & 0 deletions gpcontrib/orca/include/optimizer/orca_guc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ extern bool optimizer_force_comprehensive_join_implementation;
extern bool optimizer_enable_replicated_table;
extern bool optimizer_enable_foreign_table;
extern bool optimizer_enable_right_outer_join;
extern bool optimizer_enable_query_parameter;

/* Optimizer plan enumeration related GUCs */
extern bool optimizer_enumerate_plans;
Expand Down
20 changes: 20 additions & 0 deletions gpcontrib/orca/orca.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ void _PG_fini(void);

static planner_hook_type prev_planner = NULL;
static ExplainOneQuery_hook_type prev_explain = NULL;
static choose_custom_plan_hook_type prev_choose_custom_plan_hook = NULL;

/*
* Decide JIT settings for the given plan and record them in PlannedStmt.jitFlags.
Expand Down Expand Up @@ -246,6 +247,21 @@ orca_explain(Query *query, int cursorOptions, IntoClause *into,
params, queryEnv);
}

static PlanCacheMode
orca_choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams,
IntoClause *intoClause)
{
/* Generate custom plans if optimizer_enable_query_parameter is disabled and Orca is enabled */
if (optimizer && !optimizer_enable_query_parameter)
return PLAN_CACHE_MODE_FORCE_CUSTOM_PLAN;

if (prev_choose_custom_plan_hook)
return prev_choose_custom_plan_hook(plansource, boundParams,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can multiple extensions set their own hooks in choose_custom_plan_hook that return different results? And what to do about it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory it is possible, but I think that only planner extension would be interested in using choose_custom_plan_hook, and we do not support more than one external planner at the same time currently. So I guess currently it doesn't sound like a real problem.

intoClause);

return PLAN_CACHE_MODE_AUTO;
}

void
_PG_init(void)
{
Expand Down Expand Up @@ -286,6 +302,9 @@ _PG_init(void)
prev_explain = ExplainOneQuery_hook;
ExplainOneQuery_hook = orca_explain;

prev_choose_custom_plan_hook = choose_custom_plan_hook;
choose_custom_plan_hook = orca_choose_custom_plan;

SetConfigOption("optimizer", "on", PGC_POSTMASTER, PGC_S_DYNAMIC_DEFAULT);
}

Expand All @@ -294,4 +313,5 @@ _PG_fini(void)
{
planner_hook = prev_planner;
ExplainOneQuery_hook = prev_explain;
choose_custom_plan_hook = prev_choose_custom_plan_hook;
}
10 changes: 10 additions & 0 deletions gpcontrib/orca/orca_guc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ bool optimizer_force_comprehensive_join_implementation;
bool optimizer_enable_replicated_table;
bool optimizer_enable_foreign_table;
bool optimizer_enable_right_outer_join;
bool optimizer_enable_query_parameter;

/* Optimizer plan enumeration related GUCs */
bool optimizer_enumerate_plans;
Expand Down Expand Up @@ -891,6 +892,15 @@ struct config_bool configure_names_bool_orca[] = {
NULL,
NULL},

{{"optimizer_enable_query_parameter", PGC_USERSET, DEVELOPER_OPTIONS,
gettext_noop("Enable query parameters in Orca."), NULL,
GUC_NOT_IN_SAMPLE},
&optimizer_enable_query_parameter,
true,
NULL,
NULL,
NULL},

{{"optimizer_enable_coordinator_only_queries", PGC_USERSET,
QUERY_TUNING_METHOD,
gettext_noop("Process coordinator only queries via the optimizer."), NULL,
Expand Down
16 changes: 13 additions & 3 deletions src/backend/utils/cache/plancache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@
((plansource)->raw_parse_tree && \
IsA((plansource)->raw_parse_tree->stmt, TransactionStmt))

/* hooks */
choose_custom_plan_hook_type choose_custom_plan_hook = NULL;

/*
* This is the head of the backend's list of "saved" CachedPlanSources (i.e.,
* those that are in long-lived storage and are examined for sinval events).
Expand Down Expand Up @@ -1088,9 +1091,16 @@ choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams, Into
if (plansource->cursor_options & CURSOR_OPT_CUSTOM_PLAN)
return true;

/* Generate custom plans if optimizer_enable_query_parameter is disabled and Orca is enabled */
if (optimizer && !optimizer_enable_query_parameter)
return true;
/* See if extension wants to force the decision */
if (choose_custom_plan_hook != NULL)
{
PlanCacheMode mode = choose_custom_plan_hook(plansource, boundParams, intoClause);

if (mode == PLAN_CACHE_MODE_FORCE_GENERIC_PLAN)
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was such a return possible before the patch?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking about Orca - orca_choose_custom_plan() doesn't return PLAN_CACHE_MODE_FORCE_GENERIC_PLAN. Speaking about choose_custom_plan() in general - similar logic is already implenented for plan_cache_mode GUC, which can force the decision.

Overall, the delta should preserve previous logic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar logic is already implenented for plan_cache_mode GUC, which can force the decision.

Can we use it instead of the new hook?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it, but that means we'll need to change plan_cache_mode each time optimizer and optimizer_enable_query_parameter are changed. Such non-obvious coupling between GUCs doesn't seem a proper thing. And what if user also sets the optimizer_enable_query_parameter GUC? And we'll need somehow to handle cases when user sets the GUC after Orca has done it, and vice versa.

This approach seems too error prone, so I decided not go this route.

if (mode == PLAN_CACHE_MODE_FORCE_CUSTOM_PLAN)
return true;
}

/* Generate custom plans until we have done at least 5 (arbitrary) */
if (plansource->num_custom_plans < 5)
Expand Down
12 changes: 0 additions & 12 deletions src/backend/utils/misc/guc_gp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,6 @@ bool gp_force_random_redistribution = false;
bool optimizer;
bool optimizer_control = true;

bool optimizer_enable_query_parameter;

/* Analyze related GUCs for external planner */
bool optimizer_analyze_root_partition;
bool optimizer_analyze_midlevel_partition;
Expand Down Expand Up @@ -1943,16 +1941,6 @@ struct config_bool ConfigureNamesBool_gp[] =
false,
NULL, NULL, NULL
},
{
{"optimizer_enable_query_parameter", PGC_USERSET, DEVELOPER_OPTIONS,
gettext_noop("Enable query parameters in Orca."),
NULL,
GUC_NOT_IN_SAMPLE
},
&optimizer_enable_query_parameter,
true,
NULL, NULL, NULL
},
{
{"gp_log_suboverflow_statement", PGC_SUSET, LOGGING_WHAT,
gettext_noop("Enable logging of statements that cause subtransaction overflow."),
Expand Down
2 changes: 0 additions & 2 deletions src/include/utils/guc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,8 +427,6 @@ extern bool create_restartpoint_on_ckpt_record_replay;
extern bool optimizer;
extern bool optimizer_control; /* controls whether the user can change the setting of the "optimizer" guc */

extern bool optimizer_enable_query_parameter;

/* Analyze related GUCs for external planner */
extern bool optimizer_analyze_root_partition;
extern bool optimizer_analyze_midlevel_partition;
Expand Down
13 changes: 13 additions & 0 deletions src/include/utils/plancache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,19 @@ typedef struct CachedExpression
dlist_node node; /* link in global list of CachedExpressions */
} CachedExpression;

/*
* Choose custom plan hook.
*
* This hook can be set by an extension to control whether to use custom or
* generic plan.
*/
typedef
PlanCacheMode(*choose_custom_plan_hook_type) (
CachedPlanSource *plansource,
ParamListInfo boundParams,
IntoClause *intoClause);

extern PGDLLIMPORT choose_custom_plan_hook_type choose_custom_plan_hook;

extern void InitPlanCache(void);
extern void ResetPlanCache(void);
Expand Down
3 changes: 0 additions & 3 deletions src/include/utils/unsync_guc_name.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,9 +334,6 @@
"optimizer_analyze_midlevel_partition",
"optimizer_analyze_root_partition",
"optimizer_control",
"optimizer_enable_master_only_queries",
"optimizer_enable_motions_masteronly_queries",
"optimizer_enable_query_parameter",
"optimizer_replicated_table_insert",
"parallel_leader_participation",
"parallel_setup_cost",
Expand Down
Loading