Skip to content

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 4, 2018
Merged

Add xact support optimization #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d888a73
Optimization of isVarChangedInTrans()
CherkashinSergey May 2, 2018
0055ca3
Add clarification in README.md
CherkashinSergey May 2, 2018
bf7c420
Refactored
CherkashinSergey May 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`:
Copy link
Collaborator

Choose a reason for hiding this comment

The 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
Expand Down
104 changes: 72 additions & 32 deletions pg_variables.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 isVarChangedInTrans(HashVariableEntry *variable);
static bool isVarChangedInCurrentTrans(HashVariableEntry *variable);
static bool isVarChangedInUpperTrans(HashVariableEntry *variable);
static void addToChangedVars(HashPackageEntry *package, HashVariableEntry *variable);

#define CHECK_ARGS_FOR_NULL() \
Expand All @@ -100,12 +101,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)
Expand Down Expand Up @@ -621,7 +621,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);
Expand Down Expand Up @@ -707,7 +707,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);
Expand Down Expand Up @@ -785,7 +785,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);
Expand Down Expand Up @@ -1220,7 +1220,7 @@ remove_packages(PG_FUNCTION_ARGS)

packagesHash = NULL;
ModuleContext = NULL;
changedVars = NULL;
changedVarsStack = NULL;

PG_RETURN_VOID();
}
Expand Down Expand Up @@ -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 && !isVarChangedInTrans(variable))
if (variable->is_transactional && !isVarChangedInCurrentTrans(variable))
{
createSavepoint(package, variable);
}
Expand Down Expand Up @@ -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)--;
}

/*
Expand All @@ -1792,22 +1797,32 @@ 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Функции не хватает декларации в начале файла (долго искал).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А теперь вопрос к оформлению: так ли нужны декларации в начале файла? Потому что таковых нет у большинства функций.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Как раз у большинства есть (если учитывать функции с PG_FUNCTION_ARGS). Я за декларации, потому что они позволяют выполнять рефакторинг и выделять новые методы без опасений, что сборка развалится из-за "неправильного" порядка определений функций.

Допустим, в изначальном коде уже есть такая особенность. Зачем усугублять?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В таком случае, там надо все функции добавить в декларации и вообще порядок в файле навести.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Порядок во всем файле -- это хорошо, но пока лучше не распыляться и привести в порядок свои правки.

Существующий код, который не был затронут в этом пулл реквесте, будем упорядочивать в будущих пулл реквестах.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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;
}
Expand Down Expand Up @@ -1890,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);
Expand All @@ -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
*/
Expand All @@ -1925,20 +1954,19 @@ addToChangedVars(HashPackageEntry *package, HashVariableEntry *variable)

Assert(changedVarsStack && changedVarsContext);

if (!isVarChangedInTrans(variable))
if (!isVarChangedInCurrentTrans(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.
Expand All @@ -1957,13 +1985,25 @@ 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 = 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);
}
Expand Down
4 changes: 4 additions & 0 deletions pg_variables.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)

Expand Down