From dfe23543785870f679f6b57284299172def60e23 Mon Sep 17 00:00:00 2001 From: Onder Kalaci Date: Mon, 6 Apr 2020 12:03:19 +0200 Subject: [PATCH] Some indentation/style fixes --- .../distributed/connection/connection_management.c | 4 ++-- .../connection/shared_connection_stats.c | 14 ++++++-------- .../distributed/executor/adaptive_executor.c | 14 ++++++++++---- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/backend/distributed/connection/connection_management.c b/src/backend/distributed/connection/connection_management.c index 98fa806d4..af2c6ba9c 100644 --- a/src/backend/distributed/connection/connection_management.c +++ b/src/backend/distributed/connection/connection_management.c @@ -324,7 +324,7 @@ StartNodeUserDatabaseConnection(uint32 flags, const char *hostname, int32 port, { /* * The caller doesn't want the connection manager to wait - * until a connection slot is avaliable on the remote node. + * until a connection slot is available on the remote node. * In the end, we might fail to establish connection to the * remote node as it might not have any space in * max_connections for this connection establishment. @@ -362,7 +362,7 @@ StartNodeUserDatabaseConnection(uint32 flags, const char *hostname, int32 port, PG_CATCH(); { /* - * Something went wrong, make sure to decement + * Something went wrong, make sure to decrement * here otherwise we'd leak the increment we have done for this * connection attempt. */ diff --git a/src/backend/distributed/connection/shared_connection_stats.c b/src/backend/distributed/connection/shared_connection_stats.c index e9050c657..8f0945140 100644 --- a/src/backend/distributed/connection/shared_connection_stats.c +++ b/src/backend/distributed/connection/shared_connection_stats.c @@ -37,7 +37,7 @@ /* - * The data structure used to store data in shared memory. This data structure only + * The data structure used to store data in shared memory. This data structure is only * used for storing the lock. The actual statistics about the connections are stored * in the hashmap, which is allocated separately, as Postgres provides different APIs * for allocating hashmaps in the shared memory. @@ -85,7 +85,7 @@ typedef struct SharedConnStatsHashEntry */ int MaxSharedPoolSize = 0; -/* the following two structs used for accessing shared memory */ +/* the following two structs are used for accessing shared memory */ static HTAB *SharedConnStatsHash = NULL; static ConnectionStatsSharedData *ConnectionStatsSharedState = NULL; @@ -185,7 +185,7 @@ RemoveAllSharedConnectionEntriesForNode(char *hostname, int port) connKey.port = port; strlcpy(connKey.hostname, hostname, MAX_NODE_LENGTH); - /* we're reading all shared connections, prevent any changes */ + /* we're modifying the hashmap, prevent any concurrent access */ LockConnectionSharedMemory(LW_EXCLUSIVE); bool entryFound = false; @@ -269,11 +269,9 @@ TryToIncrementSharedConnectionCounter(const char *hostname, int port) LockConnectionSharedMemory(LW_EXCLUSIVE); /* - * Note that while holding a spinlock, it would not allowed to use HASH_ENTER_NULL - * if the entries in SharedConnStatsHash were allocated via palloc (as palloc - * might throw OOM errors). However, in this case we're safe as the hash map is - * allocated in shared memory, which doesn't rely on palloc for memory allocation. - * This is already asserted in hash_search() by Postgres. + * As the hash map is allocated in shared memory, it doesn't rely on palloc for + * memory allocation, so we could get NULL via HASH_ENTER_NULL. That's why we prefer + * continuing the execution instead of throwing an error. */ bool entryFound = false; SharedConnStatsHashEntry *connectionEntry = diff --git a/src/backend/distributed/executor/adaptive_executor.c b/src/backend/distributed/executor/adaptive_executor.c index 1715efa09..e6c8c08da 100644 --- a/src/backend/distributed/executor/adaptive_executor.c +++ b/src/backend/distributed/executor/adaptive_executor.c @@ -2453,9 +2453,9 @@ ManageWorkerPool(WorkerPool *workerPool) /* - * ConnectionThrottlingRequired contains the logic to decide whether a new connection - * can be considered as optional or not. When the function return true, the connection - * can be established with optinal flag, else it should not be an optional connection. + * CanUseOptionalConnection contains the logic to decide whether a new connection + * can be considered as optional or not. When the function returns true, the connection + * can be established with optional flag, else it should not be an optional connection. */ static bool CanUseOptionalConnection(WorkerPool *workerPool) @@ -2483,7 +2483,13 @@ CanUseOptionalConnection(WorkerPool *workerPool) } else if (UseConnectionPerPlacement()) { - /* user wants one connection per placement, so no throttling is desired */ + /* + * User wants one connection per placement, so no throttling is desired. + * The primary reason for this is that allowing multiple backends to use + * connection per placement could lead to unresolved self deadlocks. In other + * words, each backend may stuck waiting for other backends to get a slot + * in the shared connection counters. + */ return false; }