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.

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

Conversation

whitehawk
Copy link

@whitehawk whitehawk commented Dec 27, 2024

Move optimizer_enable_query_parameter GUC to Orca extension

Problem:
'optimizer_enable_query_parameter' GUC is a setting that affects Orca behavior,
so we would like it to reside in Orca. But this GUC was also referenced by the
core (in 'choose_custom_plan()'). It prevented moving of the GUC to the
extension.

Fix:
In 'choose_custom_plan()' the GUC was used to make a decision whether to use
custom or generic plan in plan cache manager. In general, this decision can be
affected by different parameters, and a generic external planner should be able
to have control over this decision. Thus, this patch:

  • Adds a hook, that is checked in 'choose_custom_plan()'.
  • Moves the custom logic, involving 'optimizer_enable_query_parameter' GUC,
    to the extension side.
  • Moves 'optimizer_enable_query_parameter' GUC into the extension.
  • Also, it removes some names from 'unsync_guc_name.h', that were moved to the
    extension previously.

@whitehawk whitehawk marked this pull request as ready for review December 27, 2024 03:30
@andr-sokolov
Copy link
Member

Can we revert 38f4cb8 instead of replacing GUC with hook? The purpose is to make it simpler bumping PostgreSQL version, to reduce difference between PostgreSQL and GPDB, but the diff in choose_custom_plan() is increased.

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.

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.

@whitehawk
Copy link
Author

Can we revert 38f4cb8 instead of replacing GUC with hook? The purpose is to make it simpler bumping PostgreSQL version, to reduce difference between PostgreSQL and GPDB, but the diff in choose_custom_plan() is increased.

If I revert the logic of the mentioned commit, I start to get failure in the test:

diff -I HINT: -I CONTEXT: -I GP_IGNORE: -U3 /usr/re/share/ws98-ADBDEV-6630/gpdb/src/test/regress/expected/window_optimizer.out /usr/re/share/ws98-ADBDEV-6630/gpdb/src/test/regress/results/window.out
--- /usr/re/share/ws98-ADBDEV-6630/gpdb/src/test/regress/expected/window_optimizer.out	2025-01-14 16:31:46.185514939 +1000
+++ /usr/re/share/ws98-ADBDEV-6630/gpdb/src/test/regress/results/window.out	2025-01-14 16:31:46.311578075 +1000
@@ -1334,8 +1338,6 @@
 	SELECT i, min(i) over (order by i range between '1 day' preceding and '10 days' following) as min_i
   FROM generate_series(now(), now()+'100 days'::interval, '1 hour') i;
 SELECT pg_get_viewdef('v_window');
-INFO:  GPORCA failed to produce a plan, falling back to Postgres-based planner
-DETAIL:  Falling back to Postgres-based planner because GPORCA does not support the following feature: Non-default collation
                                                       pg_get_viewdef                                                       
 ------------
      min(i.i) OVER (ORDER BY i.i RANGE BETWEEN '@ 1 day'::interval PRECEDING AND '@ 10 days'::interval FOLLOWING) AS min_i+

The query planning stops to fallback to Postgres planner. It looks that it's what the commit is talking about:

This was exposed in a test query that initially fell back, but stopped
falling back after it reached the parameterized query threshold of
5--causing the test to be flaky if the function call happened to be
costed slightly more or less than the custom vs cached plan threshold.

I don't think we should revert changes intended to fix test flakiness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants