Bugfix. Switch off quickly all AQO features if queryId is disabled. #142
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Anisimov-ds
reviewed
Mar 24, 2023
aqo.c
Outdated
| /* Saved hooks */ | ||
| static planner_hook_type prev_planner_hook = NULL; | ||
| static ExecutorStart_hook_type prev_ExecutorStart_hook = NULL; | ||
| static ExecutorRun_hook_type prev_ExecutorRun = NULL; |
There was a problem hiding this comment.
Остальные хуки именуются *_hook, а prev_ExecutorRun отличается от остальных.
cardinality_hooks.c
Outdated
| set_baserel_rows_estimate_hook_type prev_set_baserel_rows_estimate_hook = NULL; | ||
| get_parameterized_baserel_size_hook_type prev_get_parameterized_baserel_size_hook = NULL; | ||
| set_joinrel_size_estimates_hook_type prev_set_joinrel_size_estimates_hook = NULL; | ||
| get_parameterized_joinrel_size_hook_type prev_get_parameterized_joinrel_size_hook = NULL; |
There was a problem hiding this comment.
Эти переменные используются только в aqo.c, но там они переобъявлены.
cardinality_hooks.c
Outdated
| @@ -33,103 +33,16 @@ | |||
| #include "machine_learning.h" | |||
| #include "path_utils.h" | |||
|
|
|||
| estimate_num_groups_hook_type prev_estimate_num_groups_hook = NULL; | |||
| estimate_num_groups_hook_type prev_estimate_num_groups_hook = NULL; | |||
There was a problem hiding this comment.
prev_estimate_num_groups_hook используется в функции aqo_estimate_num_groups_hook() для проверки, что нет посторонних хуков.
Наверное, проверку можно вынести в PG_init()
There was a problem hiding this comment.
Не согласен. Кто-то может позже переопределить хук. Добавил более строгую проверку
danolivo
force-pushed
the
off-without-queryid
branch
2 times, most recently
from
2dc14d9 to
cacc8c2
Compare
different libraries. To avoid such a problem in future, refactor AQO interfaces: declare all hooks as static, reduce number of exporting functions and introduce concept of *_init() function for a module that needs some actions in the PG_init() routine. Reviewed by: @Anisimov-ds
One installcheck test was added into the github actions workflow. Reviewed by: @Anisimov-ds
danolivo
force-pushed
the
off-without-queryid
branch
from
cacc8c2 to
700ed29
Compare
…n of AQO prediction hooks. It isn't a strict rule, but we should know about that.
Anisimov-ds
approved these changes
Mar 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One installcheck test was added into the github actions workflow.
Push it into stable14 and master as quick as possible.
stable13 based on custom query hash and don't needed this.
Remember! The master branch can contain one more hook! Check it.