From d888a73c8fea73f56820cebaca1150d7e23b6336 Mon Sep 17 00:00:00 2001 From: Sergey Cherkashin <4erkashin@list.ru> Date: Wed, 2 May 2018 21:36:01 +0300 Subject: [PATCH 1/3] Optimization of isVarChangedInTrans() --- pg_variables.c | 78 +++++++++++++++++++++++++++++++++----------------- pg_variables.h | 4 +++ 2 files changed, 56 insertions(+), 26 deletions(-) diff --git a/pg_variables.c b/pg_variables.c index 349ea1a..828a1a5 100755 --- a/pg_variables.c +++ b/pg_variables.c @@ -76,7 +76,7 @@ static HashVariableEntry *createVariableInternal(HashPackageEntry *package, text *name, Oid typid, bool is_transactional); static void createSavepoint(HashPackageEntry *package, HashVariableEntry *variable); -static bool isVarChangedInTrans(HashVariableEntry *variable); +static bool isVarChangedInCurrentTrans(HashVariableEntry *variable); static void addToChangedVars(HashPackageEntry *package, HashVariableEntry *variable); #define CHECK_ARGS_FOR_NULL() \ @@ -100,12 +100,11 @@ static HashPackageEntry *LastPackage = NULL; static HashVariableEntry *LastVariable = NULL; /* - * List of variables, changed in top level transaction. Used to limit + * Stack of lists of variables, changed in each transaction level. Used to limit * number of proceeded variables on start of transaction. */ -static dlist_head *changedVars = NULL; -static MemoryContext changedVarsContext = NULL; static dlist_head *changedVarsStack = NULL; +static MemoryContext changedVarsContext = NULL; #define get_actual_changed_vars_list() \ ((dlist_head_element(ChangedVarsStackNode, node, changedVarsStack))-> \ changedVarsList) @@ -621,7 +620,7 @@ variable_insert(PG_FUNCTION_ARGS) errmsg("variable \"%s\" already created as %sTRANSACTIONAL", key, LastVariable->is_transactional ? "" : "NOT "))); } - if (!isVarChangedInTrans(variable) && variable->is_transactional) + if (!isVarChangedInCurrentTrans(variable) && variable->is_transactional) { createSavepoint(package, variable); addToChangedVars(package, variable); @@ -707,7 +706,7 @@ variable_update(PG_FUNCTION_ARGS) else variable = LastVariable; - if (variable->is_transactional && !isVarChangedInTrans(variable)) + if (variable->is_transactional && !isVarChangedInCurrentTrans(variable)) { createSavepoint(package, variable); addToChangedVars(package, variable); @@ -785,7 +784,7 @@ variable_delete(PG_FUNCTION_ARGS) else variable = LastVariable; - if (variable->is_transactional && !isVarChangedInTrans(variable)) + if (variable->is_transactional && !isVarChangedInCurrentTrans(variable)) { createSavepoint(package, variable); addToChangedVars(package, variable); @@ -1220,7 +1219,7 @@ remove_packages(PG_FUNCTION_ARGS) packagesHash = NULL; ModuleContext = NULL; - changedVars = NULL; + changedVarsStack = NULL; PG_RETURN_VOID(); } @@ -1663,7 +1662,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 && !isVarChangedInTrans(variable)) + if (variable->is_transactional && !isVarChangedInCurrentTrans(variable)) { createSavepoint(package, variable); } @@ -1769,6 +1768,7 @@ releaseSavepoint(HashVariableEntry *variable) dlist_delete(nodeToDelete); pfree(historyEntryToDelete); } + (get_actual_value(variable)->level)--; } /* @@ -1792,24 +1792,35 @@ rollbackSavepoint(HashPackageEntry *package, HashVariableEntry *variable) * Check if variable was changed in current transaction level */ static bool -isVarChangedInTrans(HashVariableEntry *variable) +isVarChangedInCurrentTrans(HashVariableEntry *variable) { - dlist_iter iter; - dlist_head *changedVars; + ValueHistoryEntry *var_state; if (!changedVarsStack) return false; - changedVars = get_actual_changed_vars_list(); - dlist_foreach(iter, changedVars) - { - ChangedVarsNode *cvn; + var_state = get_actual_value(variable); + return (var_state->level == GetCurrentTransactionNestLevel()); +} - cvn = dlist_container(ChangedVarsNode, node, iter.cur); - if (cvn->variable == variable) - return true; +/* + * Check if variable was changed in parent transaction level + */ +static bool +isVarChangedInUpperTrans(HashVariableEntry *variable) +{ + 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; + else + return false; } /* @@ -1925,7 +1936,7 @@ addToChangedVars(HashPackageEntry *package, HashVariableEntry *variable) Assert(changedVarsStack && changedVarsContext); - if (!isVarChangedInTrans(variable)) + if (!isVarChangedInCurrentTrans(variable)) { ChangedVarsNode *cvn; @@ -1934,6 +1945,7 @@ addToChangedVars(HashPackageEntry *package, HashVariableEntry *variable) cvn->package = package; cvn->variable = variable; dlist_push_head(cvsn->changedVarsList, &cvn->node); + get_actual_value(cvn->variable)->level = GetCurrentTransactionNestLevel(); } } @@ -1957,13 +1969,27 @@ levelUpOrRelease() Assert(!dlist_is_empty(changedVarsStack)); dlist_foreach(iter, bottom_list->changedVarsList) { - ChangedVarsNode *cvn; + ChangedVarsNode *cvn_old; - cvn = dlist_container(ChangedVarsNode, node, iter.cur); - if (isVarChangedInTrans(cvn->variable)) - releaseSavepoint(cvn->variable); + cvn_old = dlist_container(ChangedVarsNode, node, iter.cur); + if (isVarChangedInUpperTrans(cvn_old->variable)) + releaseSavepoint(cvn_old->variable); else - addToChangedVars(cvn->package, cvn->variable); + { + 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 = MemoryContextAllocZero(cvsn->ctx, sizeof(ChangedVarsNode)); + 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)--; + } } MemoryContextDelete(bottom_list->ctx); } diff --git a/pg_variables.h b/pg_variables.h index 383e2c7..1357244 100755 --- a/pg_variables.h +++ b/pg_variables.h @@ -68,6 +68,8 @@ typedef struct ValueHistoryEntry{ ScalarVar scalar; RecordVar record; } value; + /* Transaction nest level of current entry */ + int level; } ValueHistoryEntry; typedef dlist_head ValueHistory; @@ -139,6 +141,8 @@ extern void insert_savepoint(HashVariableEntry *variable, (&((dlist_head_element(ValueHistoryEntry, node, &variable->data))->value.scalar)) #define get_actual_value_record(variable) \ (&((dlist_head_element(ValueHistoryEntry, node, &variable->data))->value.record)) +#define get_actual_value(variable) \ + (dlist_head_element(ValueHistoryEntry, node, &variable->data)) #define get_history_entry(node_ptr) \ dlist_container(ValueHistoryEntry, node, node_ptr) From 0055ca39b5bfa6034600f4c09e8be9008998f72a Mon Sep 17 00:00:00 2001 From: Sergey Cherkashin <4erkashin@list.ru> Date: Wed, 2 May 2018 21:39:57 +0300 Subject: [PATCH 2/3] Add clarification in README.md --- README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/README.md b/README.md index d017a32..862153a 100644 --- a/README.md +++ b/README.md @@ -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`: +```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 From bf7c4206726ae3cf746e1540bff1e9c67b2fc677 Mon Sep 17 00:00:00 2001 From: Sergey Cherkashin <4erkashin@list.ru> Date: Fri, 4 May 2018 13:37:18 +0300 Subject: [PATCH 3/3] Refactored --- pg_variables.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/pg_variables.c b/pg_variables.c index 828a1a5..1fc79a2 100755 --- a/pg_variables.c +++ b/pg_variables.c @@ -77,6 +77,7 @@ static HashVariableEntry *createVariableInternal(HashPackageEntry *package, 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() \ @@ -1768,6 +1769,10 @@ 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)--; } @@ -1819,8 +1824,7 @@ isVarChangedInUpperTrans(HashVariableEntry *variable) var_prev_state = get_history_entry(var_state->node.next); return (var_prev_state->level == (GetCurrentTransactionNestLevel() - 1)); } - else - return false; + return false; } /* @@ -1901,12 +1905,12 @@ popChangedVarsStack() { if (changedVarsStack) { - ChangedVarsStackNode *cvse; + ChangedVarsStackNode *cvsn; Assert(!dlist_is_empty(changedVarsStack)); - cvse = dlist_container(ChangedVarsStackNode, node, + cvsn = dlist_container(ChangedVarsStackNode, node, dlist_pop_head_node(changedVarsStack)); - MemoryContextDelete(cvse->ctx); + MemoryContextDelete(cvsn->ctx); if (dlist_is_empty(changedVarsStack)) { MemoryContextDelete(changedVarsContext); @@ -1916,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 */ @@ -1941,16 +1959,14 @@ addToChangedVars(HashPackageEntry *package, HashVariableEntry *variable) ChangedVarsNode *cvn; cvsn = dlist_head_element(ChangedVarsStackNode, node, changedVarsStack); - cvn = MemoryContextAllocZero(cvsn->ctx, sizeof(ChangedVarsNode)); - cvn->package = package; - cvn->variable = variable; + cvn = initChangedVarsNode(cvsn->ctx, package, variable); dlist_push_head(cvsn->changedVarsList, &cvn->node); get_actual_value(cvn->variable)->level = GetCurrentTransactionNestLevel(); } } /* - * If variable was chenged in some subtransaction, it is considered that it was + * If variable was changed 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. @@ -1984,9 +2000,7 @@ levelUpOrRelease() * it was created in another context */ cvsn = dlist_head_element(ChangedVarsStackNode, node, changedVarsStack); - cvn_new = MemoryContextAllocZero(cvsn->ctx, sizeof(ChangedVarsNode)); - cvn_new->package = cvn_old->package; - cvn_new->variable = cvn_old->variable; + 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)--; }