- Notifications
You must be signed in to change notification settings - Fork3
Add xact support optimization#9
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-io commentedMay 2, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #9 +/- ##==========================================+ Coverage 95.02% 95.07% +0.05%========================================== Files 4 4 Lines 945 955 +10 ==========================================+ Hits 898 908 +10 Misses 47 47
Continue to review full report at Codecov.
|
| * Check if variable was changed in parent transaction level | ||
| */ | ||
| staticbool | ||
| isVarChangedInUpperTrans(HashVariableEntry*variable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Функции не хватает декларации в начале файла (долго искал).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
А теперь вопрос к оформлению: так ли нужны декларации в начале файла? Потому что таковых нет у большинства функций.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Как раз у большинства есть (если учитывать функции с PG_FUNCTION_ARGS). Я за декларации, потому что они позволяют выполнять рефакторинг и выделять новые методы без опасений, что сборка развалится из-за "неправильного" порядка определений функций.
Допустим, в изначальном коде уже есть такая особенность. Зачем усугублять?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
В таком случае, там надо все функции добавить в декларации и вообще порядок в файле навести.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Порядок во всем файле -- это хорошо, но пока лучше не распыляться и привести в порядок свои правки.
Существующий код, который не был затронут в этом пулл реквесте, будем упорядочивать в будущих пулл реквестах.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ну, не вопрос, конечно. Можно и так. Но мне почему-то кажется уместным сделать это сразу.
pg_variables.c Outdated
| * it was created in another context | ||
| */ | ||
| cvsn=dlist_head_element(ChangedVarsStackNode,node,changedVarsStack); | ||
| cvn_new=MemoryContextAllocZero(cvsn->ctx,sizeof(ChangedVarsNode)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Было бы недурно выделить
cvn_new = MemoryContextAllocZero ...cnv_new-> ... = ...в отдельную inline-функцию, принимающую на вход значения полей и контекст.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Выделил. Но не скажу, что код стал выглядеть менее монстрозно.
| cvn_new->package=cvn_old->package; | ||
| cvn_new->variable=cvn_old->variable; | ||
| dlist_push_head(cvsn->changedVarsList,&cvn_new->node); | ||
| (get_actual_value(cvn_new->variable)->level)--; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Надо бы каждое такое место с инкрементом и декрементом уровня выделить в отдельный абзац и сопроводить кратким комментарием-напоминалкой, зачем это нужно.
pg_variables.c Outdated
| return (var_prev_state->level== (GetCurrentTransactionNestLevel()-1)); | ||
| } | ||
| return false; | ||
| else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Лишняя правка (которая не особо красиво выглядит).
| ERROR: unrecognized variable"var_int" | ||
| COMMIT; | ||
| ``` | ||
| Also you cannot undo removing variable by`ROLLBACK`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Подобные вещи нужно уточнять у автора задачи. Предложил возможное решение проблемы в личной переписке.
Optimization of isVarChangedInTrans()