- Notifications
You must be signed in to change notification settings - Fork3
Add support of package removal rollback#11
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
za-arthur commentedMay 23, 2018
@CherkashinSergey , похоже pg_variables на тестах падает. |
codecov-io commentedMay 25, 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 #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
| if (!scalar->is_null) | ||
| { | ||
| oldcxt=MemoryContextSwitchTo(package->hctx); | ||
| oldcxt=MemoryContextSwitchTo(is_transactional ? |
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.
Предлагаю выделить выбор контекстаis_transactional ? ... : ... в отдельный макрос, принимающий в качестве аргументаpackage.
pg_variables.c Outdated
| hash_seq_init(&vstat,package->variablesHash); | ||
| while ((variable= | ||
| (HashVariableEntry*)hash_seq_search(&vstat))!=NULL) | ||
| for(inti=0;i<2;i++) |
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.
Объявление переменной в цикле не соответствует ansi c89.
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.
Я мог проглядеть похожие ошибки, советую компилировать с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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Для буферов всегда надо использоватьsnprintf.
pg_variables.c Outdated
| caseXACT_EVENT_PRE_COMMIT: | ||
| applyActionOnChangedVars(RELEASE_SAVEPOINT); | ||
| popChangedVarsStack(); | ||
| proceedChanges(RELEASE_SAVEPOINT); |
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.
Я думаю, имелся в виду все-таки process, а не proceed.
pg_variables.c Outdated
| hash_seq_init(&vstat,package->variablesHash); | ||
| while ((variable= | ||
| (HashVariableEntry*)hash_seq_search(&vstat))!=NULL) | ||
| for(inti=0;i<2;i++) |
funbringerMay 28, 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.
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.
Я посмотрел код в целом, и мне кажется, что как-то много дублирующихся фрагментов.
Например,addToChanged{Packs,Vars}(),is{Var,Pack}ChangedInUpperTrans() итд абсолютно эквивалентны с точностью до пары строчек, вremoveFromChangedVars() два почти одинаковых цикла. Мне не нравится, что практически не производится никаких попыток сжать код.
Я думаю, значительную часть кода по управлению стеками можно было бы выделить в отдельные функции, с возможностью задать дополнительные обработчики для действий, зависящих от типа стека.
| dlist_head*changedPacksList; | ||
| MemoryContextctx; | ||
| }ChangedVarsStackNode; | ||
| }ChangesStackNode; |
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.
Мне кажется тут название непоследовательное. Может ChangedStackNode?
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.
Подразумевается "стэк изменеий". Потому что в самом стеке лежит история изменений как переменных, так и пакетов. "ChangedStack" больше выглядит как "изменённый стэк".
| MemoryContextpackageContext); | ||
| /* Internal macros to manage with dlist structure */ | ||
| #defineget_actual_value_scalar(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.
Для того, чтобы можно было нормально использовать шаблоны, пришлось привести эти макросы к одному виду.
pg_variables.h Outdated
| dlist_nodenode; | ||
| boolis_valid; | ||
| intlevel; | ||
| }PackHistoryEntry; |
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.
Пока еще не все смотрел, но кажется тут можно реализовать наследование. VarHistoryEntry наследуется от PackHistoryEntry.
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.
typedefstructHistoryEntry{dlist_nodenode;boolis_valid;intlevel;}HistoryEntry;typedefstructVarHistoryEntry{HistoryEntryentry;union {ScalarVarscalar;RecordVarrecord; }value;}VarHistoryEntry;/* ... */staticboolfunc(HistoryEntry*entry){/* Do something */returnentry->is_valid;}/* ... */{HistoryEntry*entry=/* ... */;if (is_var) {VarHistoryEntry*var_entry= (VarHistoryEntry*)entry;/* ... */ }returnfunc(entry);}
za-arthur commentedJun 21, 2018
Смержил! |
Можно откатывать как удаление одного пакета переменных, так и pgv_free(). Теперь обычные и транзакционные переменные хранятся в разных хэш-таблицах, сами пакеты имеют такую же историю состояний, как и переменные.