Add support of package removal rollback #11
Conversation
|
@CherkashinSergey , похоже pg_variables на тестах падает. |
Codecov Report
@@ Coverage Diff @@
## master #11 +/- ##
==========================================
+ Coverage 93.91% 94.29% +0.38%
==========================================
Files 4 4
Lines 772 876 +104
==========================================
+ Hits 725 826 +101
- Misses 47 50 +3
Continue to review full report at Codecov.
|
pg_variables.c
Outdated
| @@ -144,7 +157,9 @@ variable_set(text *package_name, text *var_name, | |||
| scalar->is_null = is_null; | |||
| if (!scalar->is_null) | |||
| { | |||
| oldcxt = MemoryContextSwitchTo(package->hctx); | |||
| oldcxt = MemoryContextSwitchTo(is_transactional ? | |||
There was a problem hiding this comment.
Предлагаю выделить выбор контекста is_transactional ? ... : ... в отдельный макрос, принимающий в качестве аргумента package.
pg_variables.c
Outdated
| hash_seq_init(&vstat, package->variablesHash); | ||
| while ((variable = | ||
| (HashVariableEntry *) hash_seq_search(&vstat)) != NULL) | ||
| for(int i=0; i < 2; i++) |
There was a problem hiding this comment.
Объявление переменной в цикле не соответствует ansi c89.
There was a problem hiding this comment.
Я мог проглядеть похожие ошибки, советую компилировать с CFLAGS=... -std=c89. Если найдем что-то еще, нужно будет поправить. Мне самому больше нравится не выносить переменные из шапки цикла, но это не соответствует стилю PostgreSQL, а значит, нежелательно.
pg_variables.c
Outdated
| package->hctxRegular = AllocSetContextCreate(ModuleContext, | ||
| PGV_MCXT_VARS, | ||
| ALLOCSET_DEFAULT_SIZES); | ||
| sprintf(hash_name, "%s variables hash for package \"%s\"", |
There was a problem hiding this comment.
Для буферов всегда надо использовать snprintf.
pg_variables.c
Outdated
| { | ||
| switch (event) | ||
| { | ||
| case XACT_EVENT_PRE_COMMIT: | ||
| applyActionOnChangedVars(RELEASE_SAVEPOINT); | ||
| popChangedVarsStack(); | ||
| proceedChanges(RELEASE_SAVEPOINT); |
There was a problem hiding this comment.
Я думаю, имелся в виду все-таки process, а не proceed.
pg_variables.c
Outdated
| hash_seq_init(&vstat, package->variablesHash); | ||
| while ((variable = | ||
| (HashVariableEntry *) hash_seq_search(&vstat)) != NULL) | ||
| for(int i=0; i < 2; i++) |
There was a problem hiding this comment.
Я посмотрел код в целом, и мне кажется, что как-то много дублирующихся фрагментов.
Например, addToChanged{Packs,Vars}(), is{Var,Pack}ChangedInUpperTrans() итд абсолютно эквивалентны с точностью до пары строчек, в removeFromChangedVars() два почти одинаковых цикла. Мне не нравится, что практически не производится никаких попыток сжать код.
Я думаю, значительную часть кода по управлению стеками можно было бы выделить в отдельные функции, с возможностью задать дополнительные обработчики для действий, зависящих от типа стека.
| MemoryContext ctx; | ||
| } ChangedVarsStackNode; | ||
| } ChangesStackNode; |
There was a problem hiding this comment.
Мне кажется тут название непоследовательное. Может ChangedStackNode?
There was a problem hiding this comment.
Подразумевается "стэк изменеий". Потому что в самом стеке лежит история изменений как переменных, так и пакетов. "ChangedStack" больше выглядит как "изменённый стэк".
pg_variables.h
Outdated
| @@ -137,13 +159,17 @@ extern void insert_savepoint(HashVariableEntry *variable, | |||
| MemoryContext packageContext); | |||
|
|
|||
| /* Internal macros to manage with dlist structure */ | |||
| #define get_actual_value_scalar(variable) \ | |||
There was a problem hiding this comment.
Для того, чтобы можно было нормально использовать шаблоны, пришлось привести эти макросы к одному виду.
pg_variables.h
Outdated
| dlist_node node; | ||
| bool is_valid; | ||
| int level; | ||
| } PackHistoryEntry; |
There was a problem hiding this comment.
Пока еще не все смотрел, но кажется тут можно реализовать наследование. VarHistoryEntry наследуется от PackHistoryEntry.
There was a problem hiding this comment.
typedef struct HistoryEntry
{
dlist_node node;
bool is_valid;
int level;
} HistoryEntry;
typedef struct VarHistoryEntry
{
HistoryEntry entry;
union
{
ScalarVar scalar;
RecordVar record;
} value;
} VarHistoryEntry;
/* ... */
static bool
func(HistoryEntry *entry)
{
/* Do something */
return entry->is_valid;
}
/* ... */
{
HistoryEntry *entry = /* ... */;
if (is_var)
{
VarHistoryEntry *var_entry = (VarHistoryEntry *) entry;
/* ... */
}
return func(entry);
}
|
Смержил! |
Можно откатывать как удаление одного пакета переменных, так и pgv_free(). Теперь обычные и транзакционные переменные хранятся в разных хэш-таблицах, сами пакеты имеют такую же историю состояний, как и переменные.