- 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.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -335,6 +335,15 @@ SELECT pgv_get('pack','var_int', NULL::int); | ||
| ERROR: unrecognized variable "var_int" | ||
| COMMIT; | ||
| ``` | ||
| Also you cannot undo removing variable by `ROLLBACK`: | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Подобные вещи нужно уточнять у автора задачи. Предложил возможное решение проблемы в личной переписке. | ||
| ```sql | ||
| SELECT pgv_set('pack', 'var_int', 122, true); | ||
| BEGIN; | ||
| SELECT pgv_free(); | ||
| ROLLBACK; | ||
| SELECT pgv_get('pack', 'var_int', NULL::int); | ||
| ERROR: unrecognized package "pack" | ||
| ``` | ||
| If you created transactional variable once, you should use flag `is_transactional` | ||
| every time when you want to change variable value by functions `pgv_set()`, | ||
| `pgv_insert()` and deprecated setters (i.e. `pgv_set_int()`). If you try to | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -76,7 +76,8 @@ static HashVariableEntry *createVariableInternal(HashPackageEntry *package, | ||
| text *name, Oid typid, | ||
| bool is_transactional); | ||
| static void createSavepoint(HashPackageEntry *package, HashVariableEntry *variable); | ||
| static bool isVarChangedInCurrentTrans(HashVariableEntry *variable); | ||
| static bool isVarChangedInUpperTrans(HashVariableEntry *variable); | ||
| static void addToChangedVars(HashPackageEntry *package, HashVariableEntry *variable); | ||
| #define CHECK_ARGS_FOR_NULL() \ | ||
| @@ -100,12 +101,11 @@ static HashPackageEntry *LastPackage = NULL; | ||
| static HashVariableEntry *LastVariable = NULL; | ||
| /* | ||
| *Stack oflists ofvariables, changed ineachtransaction level. Used to limit | ||
| * number of proceeded variables on start of transaction. | ||
| */ | ||
| static dlist_head *changedVarsStack = NULL; | ||
| static MemoryContext changedVarsContext = NULL; | ||
| #define get_actual_changed_vars_list() \ | ||
| ((dlist_head_element(ChangedVarsStackNode, node, changedVarsStack))-> \ | ||
| changedVarsList) | ||
| @@ -621,7 +621,7 @@ variable_insert(PG_FUNCTION_ARGS) | ||
| errmsg("variable \"%s\" already created as %sTRANSACTIONAL", | ||
| key, LastVariable->is_transactional ? "" : "NOT "))); | ||
| } | ||
| if (!isVarChangedInCurrentTrans(variable) && variable->is_transactional) | ||
| { | ||
| createSavepoint(package, variable); | ||
| addToChangedVars(package, variable); | ||
| @@ -707,7 +707,7 @@ variable_update(PG_FUNCTION_ARGS) | ||
| else | ||
| variable = LastVariable; | ||
| if (variable->is_transactional && !isVarChangedInCurrentTrans(variable)) | ||
| { | ||
| createSavepoint(package, variable); | ||
| addToChangedVars(package, variable); | ||
| @@ -785,7 +785,7 @@ variable_delete(PG_FUNCTION_ARGS) | ||
| else | ||
| variable = LastVariable; | ||
| if (variable->is_transactional && !isVarChangedInCurrentTrans(variable)) | ||
| { | ||
| createSavepoint(package, variable); | ||
| addToChangedVars(package, variable); | ||
| @@ -1220,7 +1220,7 @@ remove_packages(PG_FUNCTION_ARGS) | ||
| packagesHash = NULL; | ||
| ModuleContext = NULL; | ||
| changedVarsStack = NULL; | ||
| PG_RETURN_VOID(); | ||
| } | ||
| @@ -1663,7 +1663,7 @@ createVariableInternal(HashPackageEntry *package, text *name, Oid typid, | ||
| * For each transaction level there should be own savepoint. | ||
| * New value should be stored in a last state. | ||
| */ | ||
| if (variable->is_transactional && !isVarChangedInCurrentTrans(variable)) | ||
| { | ||
| createSavepoint(package, variable); | ||
| } | ||
| @@ -1769,6 +1769,11 @@ releaseSavepoint(HashVariableEntry *variable) | ||
| dlist_delete(nodeToDelete); | ||
| pfree(historyEntryToDelete); | ||
| } | ||
| /* | ||
| * If variable was changed in subtransaction, so it is considered it | ||
| * was changed in parent transaction. | ||
| */ | ||
| (get_actual_value(variable)->level)--; | ||
| } | ||
| /* | ||
| @@ -1792,22 +1797,32 @@ rollbackSavepoint(HashPackageEntry *package, HashVariableEntry *variable) | ||
| * Check if variable was changed in current transaction level | ||
| */ | ||
| static bool | ||
| isVarChangedInCurrentTrans(HashVariableEntry *variable) | ||
| { | ||
| ValueHistoryEntry *var_state; | ||
| if (!changedVarsStack) | ||
| return false; | ||
| var_state = get_actual_value(variable); | ||
| return (var_state->level == GetCurrentTransactionNestLevel()); | ||
| } | ||
| /* | ||
| * Check if variable was changed in parent transaction level | ||
| */ | ||
| static bool | ||
| isVarChangedInUpperTrans(HashVariableEntry *variable) | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Функции не хватает декларации в начале файла (долго искал). CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. А теперь вопрос к оформлению: так ли нужны декларации в начале файла? Потому что таковых нет у большинства функций. Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Как раз у большинства есть (если учитывать функции с PG_FUNCTION_ARGS). Я за декларации, потому что они позволяют выполнять рефакторинг и выделять новые методы без опасений, что сборка развалится из-за "неправильного" порядка определений функций. Допустим, в изначальном коде уже есть такая особенность. Зачем усугублять? CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. В таком случае, там надо все функции добавить в декларации и вообще порядок в файле навести. Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Порядок во всем файле -- это хорошо, но пока лучше не распыляться и привести в порядок свои правки. Существующий код, который не был затронут в этом пулл реквесте, будем упорядочивать в будущих пулл реквестах. CollaboratorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Ну, не вопрос, конечно. Можно и так. Но мне почему-то кажется уместным сделать это сразу. | ||
| { | ||
| ValueHistoryEntry *var_state, | ||
| *var_prev_state; | ||
| var_state = get_actual_value(variable); | ||
| if(dlist_has_next(&variable->data, &var_state->node)) | ||
| { | ||
| var_prev_state = get_history_entry(var_state->node.next); | ||
| return (var_prev_state->level == (GetCurrentTransactionNestLevel() - 1)); | ||
| } | ||
| return false; | ||
| } | ||
| @@ -1890,12 +1905,12 @@ popChangedVarsStack() | ||
| { | ||
| if (changedVarsStack) | ||
| { | ||
| ChangedVarsStackNode *cvsn; | ||
| Assert(!dlist_is_empty(changedVarsStack)); | ||
| cvsn = dlist_container(ChangedVarsStackNode, node, | ||
| dlist_pop_head_node(changedVarsStack)); | ||
| MemoryContextDelete(cvsn->ctx); | ||
| if (dlist_is_empty(changedVarsStack)) | ||
| { | ||
| MemoryContextDelete(changedVarsContext); | ||
| @@ -1905,6 +1920,20 @@ popChangedVarsStack() | ||
| } | ||
| } | ||
| /* | ||
| * Initialize an instance of ChangedVarsNode datatype | ||
| */ | ||
| static inline ChangedVarsNode * | ||
| initChangedVarsNode(MemoryContext ctx, HashPackageEntry *package, HashVariableEntry *variable) | ||
| { | ||
| ChangedVarsNode *cvn; | ||
| cvn = MemoryContextAllocZero(ctx, sizeof(ChangedVarsNode)); | ||
| cvn->package = package; | ||
| cvn->variable = variable; | ||
| return cvn; | ||
| } | ||
| /* | ||
| * Add a variable to list of changed vars in current transaction level | ||
| */ | ||
| @@ -1925,20 +1954,19 @@ addToChangedVars(HashPackageEntry *package, HashVariableEntry *variable) | ||
| Assert(changedVarsStack && changedVarsContext); | ||
| if (!isVarChangedInCurrentTrans(variable)) | ||
| { | ||
| ChangedVarsNode *cvn; | ||
| cvsn = dlist_head_element(ChangedVarsStackNode, node, changedVarsStack); | ||
| cvn = initChangedVarsNode(cvsn->ctx, package, variable); | ||
| dlist_push_head(cvsn->changedVarsList, &cvn->node); | ||
| get_actual_value(cvn->variable)->level = GetCurrentTransactionNestLevel(); | ||
| } | ||
| } | ||
| /* | ||
| * If variable waschanged in some subtransaction, it is considered that it was | ||
| * changed in parent transaction. So it is important to add this variable to | ||
| * list of changes of parent transaction. But if var was already changed in | ||
| * upper level, it has savepoint there, so we need to release it. | ||
| @@ -1957,13 +1985,25 @@ levelUpOrRelease() | ||
| Assert(!dlist_is_empty(changedVarsStack)); | ||
| dlist_foreach(iter, bottom_list->changedVarsList) | ||
| { | ||
| ChangedVarsNode *cvn_old; | ||
| cvn_old = dlist_container(ChangedVarsNode, node, iter.cur); | ||
| if (isVarChangedInUpperTrans(cvn_old->variable)) | ||
| releaseSavepoint(cvn_old->variable); | ||
| else | ||
| { | ||
| ChangedVarsNode *cvn_new; | ||
| ChangedVarsStackNode *cvsn; | ||
| /* | ||
| * Impossible to push in upper list existing node because | ||
| * it was created in another context | ||
| */ | ||
| cvsn = dlist_head_element(ChangedVarsStackNode, node, changedVarsStack); | ||
| cvn_new = initChangedVarsNode(cvsn->ctx, cvn_old->package, cvn_old->variable); | ||
| dlist_push_head(cvsn->changedVarsList, &cvn_new->node); | ||
| (get_actual_value(cvn_new->variable)->level)--; | ||
| } | ||
| } | ||
| MemoryContextDelete(bottom_list->ctx); | ||
| } | ||