- Notifications
You must be signed in to change notification settings - Fork26
Fix bug related with queries to dead backends#10
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
codecov-io commentedOct 31, 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 #10 +/- ##=========================================- Coverage 76.78% 75.69% -1.1%========================================= Files 5 5 Lines 461 469 +8 =========================================+ Hits 354 355 +1- Misses 107 114 +7
Continue to review full report at Codecov.
|
pg_query_state.c Outdated
| @@ -971,7 +982,7 @@ GetRemoteBackendQueryStates(PGPROC *leader, | |||
| foreach(iter, pworkers) | |||
| { | |||
| PGPROC *proc = (PGPROC *) lfirst(iter); | |||
| Assert (proc && proc->pid); | |||
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.
вот здесь тоже надо через continue, бекенд может помереть и в этом месте
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.
Нет, Assert тут правильнее - мы предполагаем, что proc никогда не будет нулевым. А вот условие!proc->pid лишнее.
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.
У вас тут условие есть в GetRemoteBackendWorkers()
if (!proc || !proc->pid) continue;Поэтому видимо не может быть нулевым.
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.
Тут в смысле proc нулевым не будет в любом случае, но процесс мог умереть (или завершиться) и проверка на pid все равно нужна. По хорошему вообще SHARED лок надо ставить на procArray
@ildus зачем смёржил свой вариант!? |
@maksm90 ответь на вопрос, всегда успеем отменить если нужно. |
На какой? Выше вроде на всё ответил. Список pworkers как входной аргументGetRemoteBackendQueryStates должен состоять из ненулевых proc - это предусловие. Обеспечиваем это условие на уровне выше вGetRemoteBackendWorkers - это будет постусловием для этой функции. Это обеспечивается вставкойcontinue в цикл: |
Это указатели на структуры в shared memory, откуда предположение что pid у них будет валидным всегда? |
Тут соглашусь, гонки с аллокатором ProcArray могут быть. Тогда имеет смысл работать с pid'ами, а не со структурами PGPROC. Валидность pid'а мы сможем определить, отправляя сигнал (вызовSendProcSignal) или выполняя резолв на PGPROC. Пока мы остановимся на предположении, что гонки допустимы, но маловероятны. Для их полного разрешения надо переписывать всю логику взаимодействия, уменьшая кол-во RPC между процессами. Но это уже будет задача для следующей стадии. |
Исправлен баг: во время слоных запросов pg_query_state иногда пытался опрашивать процессы, которые уже завершили свою работу.