- Notifications
You must be signed in to change notification settings - Fork16
[PGPRO-5691] move mmapped ptrack map into shared postgres memory#19
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
Вопрос ревьюирам: как думаете, нужно ли удалять строку |
| /* | ||
| * XXX: for some reason assign_ptrack_map_size is called twice during the | ||
| * postmaster boot! First, it is always called with bootValue, so we use | ||
| * -1 as default value and no-op here. Next, it is called with the actual | ||
| * value from config. That way, we use 0 as an option for user to turn | ||
| * off ptrack and clean up all files. | ||
| */ | ||
| if (newval == -1) | ||
| return; |
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 it not a problem anymore? I see you use 0 as a boot value and clean up files when ptrack_map_size == 0
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.
We now do nothing (with files and memory) inside assign_ptrack_map_size() and use the value of ptrack_map_size after the re-assignment has passed.
Well, at least that's how I understand it.
Yeah, you don't write this file, so I don't think that we need this check in tests anymore |
* fix tap test (remove checking of ptrack.map.mmap file)* cosmetic change (minimize diff with master)
engine.c Outdated
| elog(ERROR, "ptrack read map: unexpected end of file while reading map file \"%s\", expected to read %zu, but read only %zu bytes", | ||
| ptrack_path, PtrackActualSize, readed); | ||
| } | ||
| else if (last_readed < 0) |
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.
Надо на EINTR проверить. И...
Посмотрел на искось код постгресса, практически везде просто делают continue. Т.е. ни какой специальной реакции типа CHECK_FOR_INTERRUPTS обычно нет.
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.
Добавил проверку EINTR. CHECK_FOR_INTERRUPTS не добавлял.
Заодно убрал дублирующее readed += last_readed, которое каким-то чудом проскочило через предыдущие запуски тестов.
engine.c Outdated
| #ifdef WIN32 | ||
| /* Check ptrack version inside old ptrack map */ | ||
| if (ptrack_map->version_num < PTRACK_COMPATIBLE_VERSION_NUM) |
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.
исправил
engine.c Outdated
| * Unconditionally update version | ||
| * This is usefull if we have read early compatible version of map | ||
| */ | ||
| ptrack_map->version_num = PTRACK_VERSION_NUM; |
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.
А почему не PTRACK_COMPATIBLE_VERSION_NUM ?
Соответственно, это нужно только подif (is_new_map)
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.
исправил
| if (ptrack_map_size != 0) | ||
| RequestAddinShmemSpace(PtrackActualSize); | ||
| else | ||
| ptrackCleanFiles(); |
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.
А к этому моменту уже был позван assign_ptrack_map_size ?
#471)* [PGPRO-5691] ptrack-2.3: move mmapped ptrack map into shared postgres memoryIn ptrack-2.3 ptrack.map.mmap will be removed and 'incorrect checksum' errorwill not be fatal (postgrespro/ptrack#19)* added test_corrupt_ptrack_map test compatibility with both version 2.2 and version 2.3 of ptrack
codecovbot commentedFeb 12, 2022 • 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 #19 +/- ##==========================================+ Coverage 88.55% 89.07% +0.52%========================================== Files 2 2 Lines 428 421 -7 ==========================================- Hits 379 375 -4+ Misses 49 46 -3
Continue to review full report at Codecov.
|
No description provided.