external
/
citus
mirror of https://github.com/citusdata/citus.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

make sure to correctly decrement ExecutorLevel (#3311) 

DESCRIPTION: Fix counter that keeps track of internal depth in executor

While reviewing #3302 I ran into the `ExecutorLevel` variable which used a variable to keep the original value to restore on successful exit. I haven't explored the full space and if it is possible to get into an inconsistent state. However using `PG_TRY`/`PG_CATCH` seems generally more correct.

Given very bad things will happen if this level is not reset, I kept the failsafe of setting the variiable back to 0 on the `XactCallback` but I did add an assert to treat it as a developer bug.
pull/3288/head
Nils Dijk 2019-12-16 20:50:13 +01:00 committed by GitHub
parent f90bbc64f6
commit bfc3d2eb90
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 61 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
src/backend/distributed/executor/multi_executor.c
View File

@ -122,9 +122,11 @@ CitusExecutorRun(QueryDesc *queryDesc,
ScanDirection direction, uint64 count, bool execute_once) ScanDirection direction, uint64 count, bool execute_once)
{ {
DestReceiver *dest = queryDesc->dest; DestReceiver *dest = queryDesc->dest;
int originalLevel = ExecutorLevel;
PG_TRY();
{
ExecutorLevel++; ExecutorLevel++;
if (CitusHasBeenLoaded()) if (CitusHasBeenLoaded())
{ {
if (IsLocalReferenceTableJoinPlan(queryDesc->plannedstmt) && if (IsLocalReferenceTableJoinPlan(queryDesc->plannedstmt) &&
@ -175,11 +177,15 @@ CitusExecutorRun(QueryDesc *queryDesc,
standard_ExecutorRun(queryDesc, direction, count, execute_once); standard_ExecutorRun(queryDesc, direction, count, execute_once);
} }
/* ExecutorLevel--;
* Restore the original value. It is not sufficient to decrease the value }
* because exceptions might cause us to go back a few levels at once. PG_CATCH();
*/ {
ExecutorLevel = originalLevel; ExecutorLevel--;
PG_RE_THROW();
}
PG_END_TRY();
} }

7
src/backend/distributed/transaction/transaction_management.c
View File

@ -290,6 +290,13 @@ CoordinatedTransactionCallback(XactEvent event, void *arg)
dlist_init(&InProgressTransactions); dlist_init(&InProgressTransactions);
activeSetStmts = NULL; activeSetStmts = NULL;
CoordinatedTransactionUses2PC = false; CoordinatedTransactionUses2PC = false;
/*
* Getting here without ExecutorLevel 0 is a bug, however it is such a big
* problem that will persist between reuse of the backend we still assign 0 in
* production deploys, but during development and tests we want to crash.
*/
Assert(ExecutorLevel == 0);
ExecutorLevel = 0; ExecutorLevel = 0;
/* /*