From ad42423c247dc175b4b50f1ff5642e920c3bd115 Mon Sep 17 00:00:00 2001 From: Amos Bird Date: Mon, 16 May 2016 13:45:25 +0800 Subject: [PATCH 1/3] Eliminates the possibilities of counter overflows. This patch uses scanint8 instead of pg_atoi to make sure the affected tuples counter never gets overflow. --- .../distributed/executor/multi_router_executor.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/backend/distributed/executor/multi_router_executor.c b/src/backend/distributed/executor/multi_router_executor.c index 3758d3134..7046165f3 100644 --- a/src/backend/distributed/executor/multi_router_executor.c +++ b/src/backend/distributed/executor/multi_router_executor.c @@ -33,6 +33,7 @@ #include "utils/errcodes.h" #include "utils/memutils.h" #include "utils/palloc.h" +#include "utils/int8.h" /* controls use of locks to enforce safe commutativity */ @@ -41,7 +42,7 @@ bool AllModificationsCommutative = false; static LOCKMODE CommutativityRuleToLockMode(CmdType commandType, bool upsertQuery); static void AcquireExecutorShardLock(Task *task, LOCKMODE lockMode); -static int32 ExecuteDistributedModify(Task *task); +static int64 ExecuteDistributedModify(Task *task); static void ExecuteSingleShardSelect(QueryDesc *queryDesc, uint64 tupleCount, Task *task, EState *executorState, TupleDesc tupleDescriptor, @@ -203,7 +204,7 @@ RouterExecutorRun(QueryDesc *queryDesc, ScanDirection direction, long count, Tas if (operation == CMD_INSERT || operation == CMD_UPDATE || operation == CMD_DELETE) { - int32 affectedRowCount = ExecuteDistributedModify(task); + int64 affectedRowCount = ExecuteDistributedModify(task); estate->es_processed = affectedRowCount; } else if (operation == CMD_SELECT) @@ -236,10 +237,10 @@ RouterExecutorRun(QueryDesc *queryDesc, ScanDirection direction, long count, Tas * of modified rows in that case and errors in all others. This function will * also generate warnings for individual placement failures. */ -static int32 +static int64 ExecuteDistributedModify(Task *task) { - int32 affectedTupleCount = -1; + int64 affectedTupleCount = -1; ListCell *taskPlacementCell = NULL; List *failedPlacementList = NIL; ListCell *failedPlacementCell = NULL; @@ -253,7 +254,7 @@ ExecuteDistributedModify(Task *task) PGconn *connection = NULL; PGresult *result = NULL; char *currentAffectedTupleString = NULL; - int32 currentAffectedTupleCount = -1; + int64 currentAffectedTupleCount = -1; Assert(taskPlacement->shardState == FILE_FINALIZED); @@ -294,7 +295,7 @@ ExecuteDistributedModify(Task *task) } currentAffectedTupleString = PQcmdTuples(result); - currentAffectedTupleCount = pg_atoi(currentAffectedTupleString, sizeof(int32), 0); + scanint8(currentAffectedTupleString, false, ¤tAffectedTupleCount); if ((affectedTupleCount == -1) || (affectedTupleCount == currentAffectedTupleCount)) @@ -303,7 +304,7 @@ ExecuteDistributedModify(Task *task) } else { - ereport(WARNING, (errmsg("modified %d tuples, but expected to modify %d", + ereport(WARNING, (errmsg("modified %ld tuples, but expected to modify %ld", currentAffectedTupleCount, affectedTupleCount), errdetail("modified placement on %s:%d", nodeName, nodePort))); From b58dfd93aee9d2ab0f8bfd4d06f9dd6dc70fd399 Mon Sep 17 00:00:00 2001 From: Amos Bird Date: Wed, 8 Jun 2016 10:28:59 +0800 Subject: [PATCH 2/3] Add overflow checks. --- .../executor/multi_router_executor.c | 25 ++++++++++++++----- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/backend/distributed/executor/multi_router_executor.c b/src/backend/distributed/executor/multi_router_executor.c index 7046165f3..cd9712c7a 100644 --- a/src/backend/distributed/executor/multi_router_executor.c +++ b/src/backend/distributed/executor/multi_router_executor.c @@ -42,7 +42,7 @@ bool AllModificationsCommutative = false; static LOCKMODE CommutativityRuleToLockMode(CmdType commandType, bool upsertQuery); static void AcquireExecutorShardLock(Task *task, LOCKMODE lockMode); -static int64 ExecuteDistributedModify(Task *task); +static uint64 ExecuteDistributedModify(Task *task); static void ExecuteSingleShardSelect(QueryDesc *queryDesc, uint64 tupleCount, Task *task, EState *executorState, TupleDesc tupleDescriptor, @@ -204,7 +204,7 @@ RouterExecutorRun(QueryDesc *queryDesc, ScanDirection direction, long count, Tas if (operation == CMD_INSERT || operation == CMD_UPDATE || operation == CMD_DELETE) { - int64 affectedRowCount = ExecuteDistributedModify(task); + uint64 affectedRowCount = ExecuteDistributedModify(task); estate->es_processed = affectedRowCount; } else if (operation == CMD_SELECT) @@ -237,7 +237,7 @@ RouterExecutorRun(QueryDesc *queryDesc, ScanDirection direction, long count, Tas * of modified rows in that case and errors in all others. This function will * also generate warnings for individual placement failures. */ -static int64 +static uint64 ExecuteDistributedModify(Task *task) { int64 affectedTupleCount = -1; @@ -295,7 +295,18 @@ ExecuteDistributedModify(Task *task) } currentAffectedTupleString = PQcmdTuples(result); + + /* May throw out of range errors if the tuple + * count is greater than MAX_INT64. + */ scanint8(currentAffectedTupleString, false, ¤tAffectedTupleCount); + Assert(currentAffectedTupleCount >= 0); + +#if (PG_VERSION_NUM < 90600) + + /* make sure that prior version workers don't overflow */ + Assert(currentAffectedTupleCount <= PG_UINT32_MAX); +#endif if ((affectedTupleCount == -1) || (affectedTupleCount == currentAffectedTupleCount)) @@ -304,8 +315,10 @@ ExecuteDistributedModify(Task *task) } else { - ereport(WARNING, (errmsg("modified %ld tuples, but expected to modify %ld", - currentAffectedTupleCount, affectedTupleCount), + ereport(WARNING, (errmsg( + "modified " INT64_FORMAT + " tuples, but expected to modify " INT64_FORMAT, + currentAffectedTupleCount, affectedTupleCount), errdetail("modified placement on %s:%d", nodeName, nodePort))); } @@ -331,7 +344,7 @@ ExecuteDistributedModify(Task *task) failedPlacement->nodeName, failedPlacement->nodePort); } - return affectedTupleCount; + return (uint64) affectedTupleCount; } From 369ab7664ce926813a193a5d4ec4f6a1ae6ce4ca Mon Sep 17 00:00:00 2001 From: Jason Petersen Date: Wed, 8 Jun 2016 10:34:07 -0600 Subject: [PATCH 3/3] Minor formatting/comment fixes --- .../distributed/executor/multi_router_executor.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/backend/distributed/executor/multi_router_executor.c b/src/backend/distributed/executor/multi_router_executor.c index cd9712c7a..679a77067 100644 --- a/src/backend/distributed/executor/multi_router_executor.c +++ b/src/backend/distributed/executor/multi_router_executor.c @@ -296,15 +296,13 @@ ExecuteDistributedModify(Task *task) currentAffectedTupleString = PQcmdTuples(result); - /* May throw out of range errors if the tuple - * count is greater than MAX_INT64. - */ + /* could throw error if input > MAX_INT64 */ scanint8(currentAffectedTupleString, false, ¤tAffectedTupleCount); Assert(currentAffectedTupleCount >= 0); #if (PG_VERSION_NUM < 90600) - /* make sure that prior version workers don't overflow */ + /* before 9.6, PostgreSQL used a uint32 for this field, so check */ Assert(currentAffectedTupleCount <= PG_UINT32_MAX); #endif @@ -315,12 +313,10 @@ ExecuteDistributedModify(Task *task) } else { - ereport(WARNING, (errmsg( - "modified " INT64_FORMAT - " tuples, but expected to modify " INT64_FORMAT, - currentAffectedTupleCount, affectedTupleCount), - errdetail("modified placement on %s:%d", - nodeName, nodePort))); + ereport(WARNING, + (errmsg("modified " INT64_FORMAT " tuples, but expected to modify " + INT64_FORMAT, currentAffectedTupleCount, affectedTupleCount), + errdetail("modified placement on %s:%d", nodeName, nodePort))); } PQclear(result);