From 454179e0bd722724a6d9a9f39313ef43f269a627 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Wed, 18 Dec 2024 14:54:12 +0300 Subject: [PATCH 01/11] Add a test that reproduces the bug --- .../regress/expected/remove_coordinator.out | 18 ++++++---- src/test/regress/sql/remove_coordinator.sql | 35 +++++++++++++++++++ 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/src/test/regress/expected/remove_coordinator.out b/src/test/regress/expected/remove_coordinator.out index 0226a7cd073..907eef8a204 100644 --- a/src/test/regress/expected/remove_coordinator.out +++ b/src/test/regress/expected/remove_coordinator.out @@ -5,10 +5,14 @@ SELECT master_remove_node('localhost', :master_port); (1 row) --- restore coordinator for the rest of the tests -SELECT citus_set_coordinator_host('localhost', :master_port); - citus_set_coordinator_host ---------------------------------------------------------------------- - -(1 row) - +-- to silence -potentially flaky- "could not establish connection after" warnings in below test +SET client_min_messages TO ERROR; +-- to fail fast if the hostname is not resolvable +SET citus.node_connection_timeout to '1s'; +BEGIN; + SET application_name TO 'new_app_name'; + -- that should fail because of bad hostname & port + SELECT citus_add_node('200.200.200.200', 1, 200); +ERROR: connection to the remote node postgres@200.200.200.200:1 failed +SSL SYSCALL error: EOF detected +connection to server was lost diff --git a/src/test/regress/sql/remove_coordinator.sql b/src/test/regress/sql/remove_coordinator.sql index b0df327d15f..02df7c28be5 100644 --- a/src/test/regress/sql/remove_coordinator.sql +++ b/src/test/regress/sql/remove_coordinator.sql @@ -1,5 +1,40 @@ -- removing coordinator from pg_dist_node should update pg_dist_colocation SELECT master_remove_node('localhost', :master_port); +-- to silence -potentially flaky- "could not establish connection after" warnings in below test +SET client_min_messages TO ERROR; + +-- to fail fast if the hostname is not resolvable +SET citus.node_connection_timeout to '1s'; + +BEGIN; + SET application_name TO 'new_app_name'; + + -- that should fail because of bad hostname & port + SELECT citus_add_node('200.200.200.200', 1, 200); + + -- Since above command failed, now Postgres will need to revert the + -- application_name change made in this transaction and this will + -- happen within abort-transaction callback, so we won't be in a + -- transaction block while Postgres does that. + -- + -- And when the application_name changes, Citus tries to re-assign + -- the global pid but it does so only for Citus internal backends, + -- and doing so for Citus internal backends doesn't require being + -- in a transaction block and is safe. + -- + -- However, for the client external backends (like us here), Citus + -- doesn't re-assign the global pid because it's not safe to do so + -- outside of a transaction block. This is because, it would require + -- performing a catalog access to retrive the local node id when the + -- cached local node is invalidated like what just happened here + -- because of the failed citus_add_node() call made above. + -- + -- So by failing here (rather than crashing), we ensure this behavior. +ROLLBACK; + +RESET client_min_messages; +RESET citus.node_connection_timeout; + -- restore coordinator for the rest of the tests SELECT citus_set_coordinator_host('localhost', :master_port); From d06c0cd95812f0c3af679d521dcc23bc645a2ac0 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Tue, 17 Dec 2024 19:09:56 +0300 Subject: [PATCH 02/11] Avoid re-assigning the global pid when we cannot retrieve local node id without unsafe catalog access --- src/backend/distributed/metadata/metadata_cache.c | 10 ++++++++++ src/backend/distributed/shared_library_init.c | 10 +++++++++- src/include/distributed/metadata_cache.h | 1 + 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/backend/distributed/metadata/metadata_cache.c b/src/backend/distributed/metadata/metadata_cache.c index 4f1b942a085..3bf525dd8a6 100644 --- a/src/backend/distributed/metadata/metadata_cache.c +++ b/src/backend/distributed/metadata/metadata_cache.c @@ -4545,6 +4545,16 @@ GetLocalNodeId(void) } +/* + * CachedLocalNodeIdIsValid return true if the cached local node id is valid. + */ +bool +CachedLocalNodeIdIsValid(void) +{ + return LocalNodeId != -1; +} + + /* * RegisterLocalGroupIdCacheCallbacks registers the callbacks required to * maintain LocalGroupId at a consistent value. It's separate from diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c index bd65fa60c01..28a2da7b517 100644 --- a/src/backend/distributed/shared_library_init.c +++ b/src/backend/distributed/shared_library_init.c @@ -2899,6 +2899,13 @@ ApplicationNameAssignHook(const char *newval, void *extra) * So we set the FinishedStartupCitusBackend flag in StartupCitusBackend to * indicate when this responsibility handoff has happened. * + * On the other hand, even if now it's this hook's responsibility to update + * the global pid, we cannot do so if the cached local node id is invalidated + * and we're not allowed to access the catalog tables. Within a transaction + * block, we can access the catalog tables. For this reason, in addition to + * checking FinishedStartupCitusBackend, we also require either being in a + * transaction block or the cached local node id to be valid. + * * Another solution to the catalog table acccess problem would be to update * global pid lazily, like we do for HideShards. But that's not possible * for the global pid, since it is stored in shared memory instead of in a @@ -2907,7 +2914,8 @@ ApplicationNameAssignHook(const char *newval, void *extra) * as reasonably possible, which is also why we extract global pids in the * AuthHook already (extracting doesn't require catalog access). */ - if (FinishedStartupCitusBackend) + if (FinishedStartupCitusBackend && + (IsTransactionState() || CachedLocalNodeIdIsValid())) { AssignGlobalPID(newval); } diff --git a/src/include/distributed/metadata_cache.h b/src/include/distributed/metadata_cache.h index f1120497b72..d4779a98aa6 100644 --- a/src/include/distributed/metadata_cache.h +++ b/src/include/distributed/metadata_cache.h @@ -181,6 +181,7 @@ extern CitusTableCacheEntry * LookupCitusTableCacheEntry(Oid relationId); extern DistObjectCacheEntry * LookupDistObjectCacheEntry(Oid classid, Oid objid, int32 objsubid); extern int32 GetLocalGroupId(void); +extern bool CachedLocalNodeIdIsValid(void); extern int32 GetLocalNodeId(void); extern void CitusTableCacheFlushInvalidatedEntries(void); extern Oid LookupShardRelationFromCatalog(int64 shardId, bool missing_ok); From 1754f9f3ec4cd3c52a02af93d9a2a61ed11c1db1 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Wed, 18 Dec 2024 14:59:20 +0300 Subject: [PATCH 03/11] Re run the crashing test & update the output --- .../regress/expected/remove_coordinator.out | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/src/test/regress/expected/remove_coordinator.out b/src/test/regress/expected/remove_coordinator.out index 907eef8a204..8f8bede2a16 100644 --- a/src/test/regress/expected/remove_coordinator.out +++ b/src/test/regress/expected/remove_coordinator.out @@ -14,5 +14,31 @@ BEGIN; -- that should fail because of bad hostname & port SELECT citus_add_node('200.200.200.200', 1, 200); ERROR: connection to the remote node postgres@200.200.200.200:1 failed -SSL SYSCALL error: EOF detected -connection to server was lost + -- Since above command failed, now Postgres will need to revert the + -- application_name change made in this transaction and this will + -- happen within abort-transaction callback, so we won't be in a + -- transaction block while Postgres does that. + -- + -- And when the application_name changes, Citus tries to re-assign + -- the global pid but it does so only for Citus internal backends, + -- and doing so for Citus internal backends doesn't require being + -- in a transaction block and is safe. + -- + -- However, for the client external backends (like us here), Citus + -- doesn't re-assign the global pid because it's not safe to do so + -- outside of a transaction block. This is because, it would require + -- performing a catalog access to retrive the local node id when the + -- cached local node is invalidated like what just happened here + -- because of the failed citus_add_node() call made above. + -- + -- So by failing here (rather than crashing), we ensure this behavior. +ROLLBACK; +RESET client_min_messages; +RESET citus.node_connection_timeout; +-- restore coordinator for the rest of the tests +SELECT citus_set_coordinator_host('localhost', :master_port); + citus_set_coordinator_host +--------------------------------------------------------------------- + +(1 row) + From 4745a889da5b211b853f4117eee2925e65d6ae50 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Wed, 18 Dec 2024 16:38:33 +0300 Subject: [PATCH 04/11] Revert "Avoid re-assigning the global pid when we cannot retrieve local node id without unsafe catalog access" This reverts commit d06c0cd95812f0c3af679d521dcc23bc645a2ac0. --- src/backend/distributed/metadata/metadata_cache.c | 10 ---------- src/backend/distributed/shared_library_init.c | 10 +--------- src/include/distributed/metadata_cache.h | 1 - 3 files changed, 1 insertion(+), 20 deletions(-) diff --git a/src/backend/distributed/metadata/metadata_cache.c b/src/backend/distributed/metadata/metadata_cache.c index 3bf525dd8a6..4f1b942a085 100644 --- a/src/backend/distributed/metadata/metadata_cache.c +++ b/src/backend/distributed/metadata/metadata_cache.c @@ -4545,16 +4545,6 @@ GetLocalNodeId(void) } -/* - * CachedLocalNodeIdIsValid return true if the cached local node id is valid. - */ -bool -CachedLocalNodeIdIsValid(void) -{ - return LocalNodeId != -1; -} - - /* * RegisterLocalGroupIdCacheCallbacks registers the callbacks required to * maintain LocalGroupId at a consistent value. It's separate from diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c index 28a2da7b517..bd65fa60c01 100644 --- a/src/backend/distributed/shared_library_init.c +++ b/src/backend/distributed/shared_library_init.c @@ -2899,13 +2899,6 @@ ApplicationNameAssignHook(const char *newval, void *extra) * So we set the FinishedStartupCitusBackend flag in StartupCitusBackend to * indicate when this responsibility handoff has happened. * - * On the other hand, even if now it's this hook's responsibility to update - * the global pid, we cannot do so if the cached local node id is invalidated - * and we're not allowed to access the catalog tables. Within a transaction - * block, we can access the catalog tables. For this reason, in addition to - * checking FinishedStartupCitusBackend, we also require either being in a - * transaction block or the cached local node id to be valid. - * * Another solution to the catalog table acccess problem would be to update * global pid lazily, like we do for HideShards. But that's not possible * for the global pid, since it is stored in shared memory instead of in a @@ -2914,8 +2907,7 @@ ApplicationNameAssignHook(const char *newval, void *extra) * as reasonably possible, which is also why we extract global pids in the * AuthHook already (extracting doesn't require catalog access). */ - if (FinishedStartupCitusBackend && - (IsTransactionState() || CachedLocalNodeIdIsValid())) + if (FinishedStartupCitusBackend) { AssignGlobalPID(newval); } diff --git a/src/include/distributed/metadata_cache.h b/src/include/distributed/metadata_cache.h index d4779a98aa6..f1120497b72 100644 --- a/src/include/distributed/metadata_cache.h +++ b/src/include/distributed/metadata_cache.h @@ -181,7 +181,6 @@ extern CitusTableCacheEntry * LookupCitusTableCacheEntry(Oid relationId); extern DistObjectCacheEntry * LookupDistObjectCacheEntry(Oid classid, Oid objid, int32 objsubid); extern int32 GetLocalGroupId(void); -extern bool CachedLocalNodeIdIsValid(void); extern int32 GetLocalNodeId(void); extern void CitusTableCacheFlushInvalidatedEntries(void); extern Oid LookupShardRelationFromCatalog(int64 shardId, bool missing_ok); From 1bd0ed8d82bb1e2474cf200caa9f44b685d33247 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Wed, 18 Dec 2024 14:35:20 +0300 Subject: [PATCH 05/11] Avoid re-assigning the global pid for client backends when the application_name changes --- src/backend/distributed/shared_library_init.c | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c index bd65fa60c01..c4b8d36f06d 100644 --- a/src/backend/distributed/shared_library_init.c +++ b/src/backend/distributed/shared_library_init.c @@ -2890,14 +2890,25 @@ ApplicationNameAssignHook(const char *newval, void *extra) DetermineCitusBackendType(newval); /* - * AssignGlobalPID might read from catalog tables to get the the local - * nodeid. But ApplicationNameAssignHook might be called before catalog - * access is available to the backend (such as in early stages of - * authentication). We use StartupCitusBackend to initialize the global pid - * after catalogs are available. After that happens this hook becomes - * responsible to update the global pid on later application_name changes. - * So we set the FinishedStartupCitusBackend flag in StartupCitusBackend to - * indicate when this responsibility handoff has happened. + * We use StartupCitusBackend to initialize the global pid after catalogs + * are available. After that happens this hook becomes responsible to update + * the global pid on later application_name changes. So we set the + * FinishedStartupCitusBackend flag in StartupCitusBackend to indicate when + * this responsibility handoff has happened. + * + * Also note that when application_name changes, we don't actually need to + * try re-assigning the global pid for external client backends because + * application_name doesn't affect the global pid for such backends. Plus, + * trying to re-assign the global pid for external client backends would + * unnecessarily cause performing a catalog access when the cached local + * node id is invalidated. However, accessing to the catalog tables is + * dangerous in certain situations like when we're not in a transaction + * block. And for the other types of backends, i.e., the Citus internal + * backends, we need to re-assign the global pid when the application_name + * changes because for such backends we simply extract the global pid + * inherited from the originating backend from the application_name -that's + * specified by originating backend when openning that connection- and this + * doesn't require catalog access. * * Another solution to the catalog table acccess problem would be to update * global pid lazily, like we do for HideShards. But that's not possible @@ -2907,7 +2918,7 @@ ApplicationNameAssignHook(const char *newval, void *extra) * as reasonably possible, which is also why we extract global pids in the * AuthHook already (extracting doesn't require catalog access). */ - if (FinishedStartupCitusBackend) + if (FinishedStartupCitusBackend && !IsExternalClientBackend()) { AssignGlobalPID(newval); } From c6c6d9ffcb212ced53408d6c7dfa3fc3e99801a2 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Wed, 18 Dec 2024 19:56:43 +0300 Subject: [PATCH 06/11] Revert "Avoid re-assigning the global pid for client backends when the application_name changes" This reverts commit 1bd0ed8d82bb1e2474cf200caa9f44b685d33247. --- src/backend/distributed/shared_library_init.c | 29 ++++++------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c index c4b8d36f06d..bd65fa60c01 100644 --- a/src/backend/distributed/shared_library_init.c +++ b/src/backend/distributed/shared_library_init.c @@ -2890,25 +2890,14 @@ ApplicationNameAssignHook(const char *newval, void *extra) DetermineCitusBackendType(newval); /* - * We use StartupCitusBackend to initialize the global pid after catalogs - * are available. After that happens this hook becomes responsible to update - * the global pid on later application_name changes. So we set the - * FinishedStartupCitusBackend flag in StartupCitusBackend to indicate when - * this responsibility handoff has happened. - * - * Also note that when application_name changes, we don't actually need to - * try re-assigning the global pid for external client backends because - * application_name doesn't affect the global pid for such backends. Plus, - * trying to re-assign the global pid for external client backends would - * unnecessarily cause performing a catalog access when the cached local - * node id is invalidated. However, accessing to the catalog tables is - * dangerous in certain situations like when we're not in a transaction - * block. And for the other types of backends, i.e., the Citus internal - * backends, we need to re-assign the global pid when the application_name - * changes because for such backends we simply extract the global pid - * inherited from the originating backend from the application_name -that's - * specified by originating backend when openning that connection- and this - * doesn't require catalog access. + * AssignGlobalPID might read from catalog tables to get the the local + * nodeid. But ApplicationNameAssignHook might be called before catalog + * access is available to the backend (such as in early stages of + * authentication). We use StartupCitusBackend to initialize the global pid + * after catalogs are available. After that happens this hook becomes + * responsible to update the global pid on later application_name changes. + * So we set the FinishedStartupCitusBackend flag in StartupCitusBackend to + * indicate when this responsibility handoff has happened. * * Another solution to the catalog table acccess problem would be to update * global pid lazily, like we do for HideShards. But that's not possible @@ -2918,7 +2907,7 @@ ApplicationNameAssignHook(const char *newval, void *extra) * as reasonably possible, which is also why we extract global pids in the * AuthHook already (extracting doesn't require catalog access). */ - if (FinishedStartupCitusBackend && !IsExternalClientBackend()) + if (FinishedStartupCitusBackend) { AssignGlobalPID(newval); } From 0f84ed807be883f799610c5a3655c53ca87a2e62 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Tue, 17 Dec 2024 19:09:56 +0300 Subject: [PATCH 07/11] Avoid re-assigning the global pid when we cannot retrieve local node id without unsafe catalog access (cherry picked from commit d06c0cd95812f0c3af679d521dcc23bc645a2ac0) --- src/backend/distributed/metadata/metadata_cache.c | 10 ++++++++++ src/backend/distributed/shared_library_init.c | 10 +++++++++- src/include/distributed/metadata_cache.h | 1 + 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/backend/distributed/metadata/metadata_cache.c b/src/backend/distributed/metadata/metadata_cache.c index 4f1b942a085..3bf525dd8a6 100644 --- a/src/backend/distributed/metadata/metadata_cache.c +++ b/src/backend/distributed/metadata/metadata_cache.c @@ -4545,6 +4545,16 @@ GetLocalNodeId(void) } +/* + * CachedLocalNodeIdIsValid return true if the cached local node id is valid. + */ +bool +CachedLocalNodeIdIsValid(void) +{ + return LocalNodeId != -1; +} + + /* * RegisterLocalGroupIdCacheCallbacks registers the callbacks required to * maintain LocalGroupId at a consistent value. It's separate from diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c index bd65fa60c01..28a2da7b517 100644 --- a/src/backend/distributed/shared_library_init.c +++ b/src/backend/distributed/shared_library_init.c @@ -2899,6 +2899,13 @@ ApplicationNameAssignHook(const char *newval, void *extra) * So we set the FinishedStartupCitusBackend flag in StartupCitusBackend to * indicate when this responsibility handoff has happened. * + * On the other hand, even if now it's this hook's responsibility to update + * the global pid, we cannot do so if the cached local node id is invalidated + * and we're not allowed to access the catalog tables. Within a transaction + * block, we can access the catalog tables. For this reason, in addition to + * checking FinishedStartupCitusBackend, we also require either being in a + * transaction block or the cached local node id to be valid. + * * Another solution to the catalog table acccess problem would be to update * global pid lazily, like we do for HideShards. But that's not possible * for the global pid, since it is stored in shared memory instead of in a @@ -2907,7 +2914,8 @@ ApplicationNameAssignHook(const char *newval, void *extra) * as reasonably possible, which is also why we extract global pids in the * AuthHook already (extracting doesn't require catalog access). */ - if (FinishedStartupCitusBackend) + if (FinishedStartupCitusBackend && + (IsTransactionState() || CachedLocalNodeIdIsValid())) { AssignGlobalPID(newval); } diff --git a/src/include/distributed/metadata_cache.h b/src/include/distributed/metadata_cache.h index f1120497b72..d4779a98aa6 100644 --- a/src/include/distributed/metadata_cache.h +++ b/src/include/distributed/metadata_cache.h @@ -181,6 +181,7 @@ extern CitusTableCacheEntry * LookupCitusTableCacheEntry(Oid relationId); extern DistObjectCacheEntry * LookupDistObjectCacheEntry(Oid classid, Oid objid, int32 objsubid); extern int32 GetLocalGroupId(void); +extern bool CachedLocalNodeIdIsValid(void); extern int32 GetLocalNodeId(void); extern void CitusTableCacheFlushInvalidatedEntries(void); extern Oid LookupShardRelationFromCatalog(int64 shardId, bool missing_ok); From c0557dc4669de290ce354021d3dba5abee07e6e6 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Wed, 18 Dec 2024 20:11:01 +0300 Subject: [PATCH 08/11] improve comments --- src/backend/distributed/shared_library_init.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c index 28a2da7b517..f1fb482b3ce 100644 --- a/src/backend/distributed/shared_library_init.c +++ b/src/backend/distributed/shared_library_init.c @@ -2900,11 +2900,16 @@ ApplicationNameAssignHook(const char *newval, void *extra) * indicate when this responsibility handoff has happened. * * On the other hand, even if now it's this hook's responsibility to update - * the global pid, we cannot do so if the cached local node id is invalidated - * and we're not allowed to access the catalog tables. Within a transaction - * block, we can access the catalog tables. For this reason, in addition to - * checking FinishedStartupCitusBackend, we also require either being in a - * transaction block or the cached local node id to be valid. + * the global pid, we cannot do so if we might need to read from catalog + * but it's unsafe to do so. For Citus internal backends, this cannot be the + * case because in that case AssignGlobalPID() just extracts its global pid + * from the application_name. But for external backends, we either need to + * guarantee that we won't read from catalog tables or that it's safe to do + * so. The only case where AssignGlobalPID() could read from catalog tables + * is when the cached local node id is invalidated. So for this reason, for + * external client backends, we require that either the cached local node id + * is valid or that we are in a transaction block -because in that case it's + * safe to read from catalog. * * Another solution to the catalog table acccess problem would be to update * global pid lazily, like we do for HideShards. But that's not possible @@ -2915,7 +2920,8 @@ ApplicationNameAssignHook(const char *newval, void *extra) * AuthHook already (extracting doesn't require catalog access). */ if (FinishedStartupCitusBackend && - (IsTransactionState() || CachedLocalNodeIdIsValid())) + (!IsExternalClientBackend() || CachedLocalNodeIdIsValid() || + IsTransactionState())) { AssignGlobalPID(newval); } From ca091116e6e1242cbc593c9735d55c34d0c1009b Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Thu, 19 Dec 2024 10:58:31 +0300 Subject: [PATCH 09/11] improve a bit more --- src/backend/distributed/shared_library_init.c | 14 +++++++------- src/test/regress/expected/remove_coordinator.out | 15 +++++++-------- src/test/regress/sql/remove_coordinator.sql | 15 +++++++-------- 3 files changed, 21 insertions(+), 23 deletions(-) diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c index f1fb482b3ce..9553600d701 100644 --- a/src/backend/distributed/shared_library_init.c +++ b/src/backend/distributed/shared_library_init.c @@ -2903,13 +2903,13 @@ ApplicationNameAssignHook(const char *newval, void *extra) * the global pid, we cannot do so if we might need to read from catalog * but it's unsafe to do so. For Citus internal backends, this cannot be the * case because in that case AssignGlobalPID() just extracts its global pid - * from the application_name. But for external backends, we either need to - * guarantee that we won't read from catalog tables or that it's safe to do - * so. The only case where AssignGlobalPID() could read from catalog tables - * is when the cached local node id is invalidated. So for this reason, for - * external client backends, we require that either the cached local node id - * is valid or that we are in a transaction block -because in that case it's - * safe to read from catalog. + * from the application_name and extracting doesn't require catalog access. + * But for external client backends, we either need to guarantee that we won't + * read from catalog tables or that it's safe to do so. The only case where + * AssignGlobalPID() could read from catalog tables is when the cached local + * node id is invalidated. So for this reason, for external client backends, + * we require that either the cached local node id is valid or that we are in + * a transaction block -because in that case it's safe to read from catalog. * * Another solution to the catalog table acccess problem would be to update * global pid lazily, like we do for HideShards. But that's not possible diff --git a/src/test/regress/expected/remove_coordinator.out b/src/test/regress/expected/remove_coordinator.out index 8f8bede2a16..796b79604dd 100644 --- a/src/test/regress/expected/remove_coordinator.out +++ b/src/test/regress/expected/remove_coordinator.out @@ -20,16 +20,15 @@ ERROR: connection to the remote node postgres@200.200.200.200:1 failed -- transaction block while Postgres does that. -- -- And when the application_name changes, Citus tries to re-assign - -- the global pid but it does so only for Citus internal backends, - -- and doing so for Citus internal backends doesn't require being - -- in a transaction block and is safe. + -- the global pid and doing so for Citus internal backends doesn't + -- require being in a transaction block and is safe. -- -- However, for the client external backends (like us here), Citus - -- doesn't re-assign the global pid because it's not safe to do so - -- outside of a transaction block. This is because, it would require - -- performing a catalog access to retrive the local node id when the - -- cached local node is invalidated like what just happened here - -- because of the failed citus_add_node() call made above. + -- doesn't try to re-assign the global pid if doing so requires catalog + -- access and we're outside of a transaction block. Note that in that + -- case the catalog access may only be needed to retrive the local node + -- id when the cached local node is invalidated like what just happened + -- here because of the failed citus_add_node() call made above. -- -- So by failing here (rather than crashing), we ensure this behavior. ROLLBACK; diff --git a/src/test/regress/sql/remove_coordinator.sql b/src/test/regress/sql/remove_coordinator.sql index 02df7c28be5..c57d7eee740 100644 --- a/src/test/regress/sql/remove_coordinator.sql +++ b/src/test/regress/sql/remove_coordinator.sql @@ -19,16 +19,15 @@ BEGIN; -- transaction block while Postgres does that. -- -- And when the application_name changes, Citus tries to re-assign - -- the global pid but it does so only for Citus internal backends, - -- and doing so for Citus internal backends doesn't require being - -- in a transaction block and is safe. + -- the global pid and doing so for Citus internal backends doesn't + -- require being in a transaction block and is safe. -- -- However, for the client external backends (like us here), Citus - -- doesn't re-assign the global pid because it's not safe to do so - -- outside of a transaction block. This is because, it would require - -- performing a catalog access to retrive the local node id when the - -- cached local node is invalidated like what just happened here - -- because of the failed citus_add_node() call made above. + -- doesn't try to re-assign the global pid if doing so requires catalog + -- access and we're outside of a transaction block. Note that in that + -- case the catalog access may only be needed to retrive the local node + -- id when the cached local node is invalidated like what just happened + -- here because of the failed citus_add_node() call made above. -- -- So by failing here (rather than crashing), we ensure this behavior. ROLLBACK; From 3e946781da394d6ef8ee4df8721ba1da781e5206 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Thu, 19 Dec 2024 11:02:07 +0300 Subject: [PATCH 10/11] more --- src/test/regress/expected/remove_coordinator.out | 2 +- src/test/regress/sql/remove_coordinator.sql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/regress/expected/remove_coordinator.out b/src/test/regress/expected/remove_coordinator.out index 796b79604dd..cf3711a1926 100644 --- a/src/test/regress/expected/remove_coordinator.out +++ b/src/test/regress/expected/remove_coordinator.out @@ -7,7 +7,7 @@ SELECT master_remove_node('localhost', :master_port); -- to silence -potentially flaky- "could not establish connection after" warnings in below test SET client_min_messages TO ERROR; --- to fail fast if the hostname is not resolvable +-- to fail fast when the hostname is not resolvable, as it will be the case below SET citus.node_connection_timeout to '1s'; BEGIN; SET application_name TO 'new_app_name'; diff --git a/src/test/regress/sql/remove_coordinator.sql b/src/test/regress/sql/remove_coordinator.sql index c57d7eee740..10d703aba33 100644 --- a/src/test/regress/sql/remove_coordinator.sql +++ b/src/test/regress/sql/remove_coordinator.sql @@ -4,7 +4,7 @@ SELECT master_remove_node('localhost', :master_port); -- to silence -potentially flaky- "could not establish connection after" warnings in below test SET client_min_messages TO ERROR; --- to fail fast if the hostname is not resolvable +-- to fail fast when the hostname is not resolvable, as it will be the case below SET citus.node_connection_timeout to '1s'; BEGIN; From 0aeafce8a0c258f5731c6a5aeab5a5c94755e6fc Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Fri, 20 Dec 2024 10:09:27 +0300 Subject: [PATCH 11/11] reland the alternative approach - works this time --- .../distributed/metadata/metadata_cache.c | 10 ----- src/backend/distributed/shared_library_init.c | 43 +++++++++---------- .../test/run_from_same_connection.c | 4 ++ .../distributed/transaction/backend_data.c | 17 ++++++++ src/include/distributed/backend_data.h | 1 + src/include/distributed/metadata_cache.h | 1 - .../regress/expected/remove_coordinator.out | 16 ++++--- src/test/regress/sql/remove_coordinator.sql | 16 ++++--- 8 files changed, 61 insertions(+), 47 deletions(-) diff --git a/src/backend/distributed/metadata/metadata_cache.c b/src/backend/distributed/metadata/metadata_cache.c index 3bf525dd8a6..4f1b942a085 100644 --- a/src/backend/distributed/metadata/metadata_cache.c +++ b/src/backend/distributed/metadata/metadata_cache.c @@ -4545,16 +4545,6 @@ GetLocalNodeId(void) } -/* - * CachedLocalNodeIdIsValid return true if the cached local node id is valid. - */ -bool -CachedLocalNodeIdIsValid(void) -{ - return LocalNodeId != -1; -} - - /* * RegisterLocalGroupIdCacheCallbacks registers the callbacks required to * maintain LocalGroupId at a consistent value. It's separate from diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c index 9553600d701..6d26b802f64 100644 --- a/src/backend/distributed/shared_library_init.c +++ b/src/backend/distributed/shared_library_init.c @@ -2890,26 +2890,27 @@ ApplicationNameAssignHook(const char *newval, void *extra) DetermineCitusBackendType(newval); /* - * AssignGlobalPID might read from catalog tables to get the the local - * nodeid. But ApplicationNameAssignHook might be called before catalog - * access is available to the backend (such as in early stages of - * authentication). We use StartupCitusBackend to initialize the global pid - * after catalogs are available. After that happens this hook becomes - * responsible to update the global pid on later application_name changes. - * So we set the FinishedStartupCitusBackend flag in StartupCitusBackend to - * indicate when this responsibility handoff has happened. + * We use StartupCitusBackend to initialize the global pid after catalogs + * are available. After that happens this hook becomes responsible to update + * the global pid on later application_name changes. So we set the + * FinishedStartupCitusBackend flag in StartupCitusBackend to indicate when + * this responsibility handoff has happened. * - * On the other hand, even if now it's this hook's responsibility to update - * the global pid, we cannot do so if we might need to read from catalog - * but it's unsafe to do so. For Citus internal backends, this cannot be the - * case because in that case AssignGlobalPID() just extracts its global pid - * from the application_name and extracting doesn't require catalog access. - * But for external client backends, we either need to guarantee that we won't - * read from catalog tables or that it's safe to do so. The only case where - * AssignGlobalPID() could read from catalog tables is when the cached local - * node id is invalidated. So for this reason, for external client backends, - * we require that either the cached local node id is valid or that we are in - * a transaction block -because in that case it's safe to read from catalog. + * Also note that when application_name changes, we don't actually need to + * try re-assigning the global pid for external client backends and + * background workers because application_name doesn't affect the global + * pid for such backends - note that !IsExternalClientBackend() check covers + * both types of backends. Plus, + * trying to re-assign the global pid for such backends would unnecessarily + * cause performing a catalog access when the cached local node id is + * invalidated. However, accessing to the catalog tables is dangerous in + * certain situations like when we're not in a transaction block. And for + * the other types of backends, i.e., the Citus internal backends, we need + * to re-assign the global pid when the application_name changes because for + * such backends we simply extract the global pid inherited from the + * originating backend from the application_name -that's specified by + * originating backend when openning that connection- and this doesn't require + * catalog access. * * Another solution to the catalog table acccess problem would be to update * global pid lazily, like we do for HideShards. But that's not possible @@ -2919,9 +2920,7 @@ ApplicationNameAssignHook(const char *newval, void *extra) * as reasonably possible, which is also why we extract global pids in the * AuthHook already (extracting doesn't require catalog access). */ - if (FinishedStartupCitusBackend && - (!IsExternalClientBackend() || CachedLocalNodeIdIsValid() || - IsTransactionState())) + if (FinishedStartupCitusBackend && !IsExternalClientBackend()) { AssignGlobalPID(newval); } diff --git a/src/backend/distributed/test/run_from_same_connection.c b/src/backend/distributed/test/run_from_same_connection.c index 52b2e0b181b..d22ee442834 100644 --- a/src/backend/distributed/test/run_from_same_connection.c +++ b/src/backend/distributed/test/run_from_same_connection.c @@ -190,6 +190,9 @@ run_commands_on_session_level_connection_to_node(PG_FUNCTION_ARGS) /* * override_backend_data_gpid is a wrapper around SetBackendDataGpid(). + * Also sets distributedCommandOriginator to true since the only caller of + * this method calls this function actually wants this backend to + * be treated as a distributed command originator with the given global pid. */ Datum override_backend_data_gpid(PG_FUNCTION_ARGS) @@ -199,6 +202,7 @@ override_backend_data_gpid(PG_FUNCTION_ARGS) uint64 gpid = PG_GETARG_INT64(0); SetBackendDataGlobalPID(gpid); + SetBackendDataDistributedCommandOriginator(true); PG_RETURN_VOID(); } diff --git a/src/backend/distributed/transaction/backend_data.c b/src/backend/distributed/transaction/backend_data.c index 67acadd2940..85fb0f6cfb9 100644 --- a/src/backend/distributed/transaction/backend_data.c +++ b/src/backend/distributed/transaction/backend_data.c @@ -964,6 +964,23 @@ SetBackendDataGlobalPID(uint64 gpid) } +/* + * SetBackendDataDistributedCommandOriginator sets the distributedCommandOriginator + * field on MyBackendData. + */ +void +SetBackendDataDistributedCommandOriginator(bool distributedCommandOriginator) +{ + if (!MyBackendData) + { + return; + } + SpinLockAcquire(&MyBackendData->mutex); + MyBackendData->distributedCommandOriginator = distributedCommandOriginator; + SpinLockRelease(&MyBackendData->mutex); +} + + /* * GetGlobalPID returns the global process id of the current backend. */ diff --git a/src/include/distributed/backend_data.h b/src/include/distributed/backend_data.h index 8014fe5a6a9..5b3fcf2ac22 100644 --- a/src/include/distributed/backend_data.h +++ b/src/include/distributed/backend_data.h @@ -61,6 +61,7 @@ extern void AssignGlobalPID(const char *applicationName); extern uint64 GetGlobalPID(void); extern void SetBackendDataDatabaseId(void); extern void SetBackendDataGlobalPID(uint64 gpid); +extern void SetBackendDataDistributedCommandOriginator(bool distributedCommandOriginator); extern uint64 ExtractGlobalPID(const char *applicationName); extern int ExtractNodeIdFromGlobalPID(uint64 globalPID, bool missingOk); extern int ExtractProcessIdFromGlobalPID(uint64 globalPID); diff --git a/src/include/distributed/metadata_cache.h b/src/include/distributed/metadata_cache.h index d4779a98aa6..f1120497b72 100644 --- a/src/include/distributed/metadata_cache.h +++ b/src/include/distributed/metadata_cache.h @@ -181,7 +181,6 @@ extern CitusTableCacheEntry * LookupCitusTableCacheEntry(Oid relationId); extern DistObjectCacheEntry * LookupDistObjectCacheEntry(Oid classid, Oid objid, int32 objsubid); extern int32 GetLocalGroupId(void); -extern bool CachedLocalNodeIdIsValid(void); extern int32 GetLocalNodeId(void); extern void CitusTableCacheFlushInvalidatedEntries(void); extern Oid LookupShardRelationFromCatalog(int64 shardId, bool missing_ok); diff --git a/src/test/regress/expected/remove_coordinator.out b/src/test/regress/expected/remove_coordinator.out index cf3711a1926..e2fd5df027b 100644 --- a/src/test/regress/expected/remove_coordinator.out +++ b/src/test/regress/expected/remove_coordinator.out @@ -20,15 +20,17 @@ ERROR: connection to the remote node postgres@200.200.200.200:1 failed -- transaction block while Postgres does that. -- -- And when the application_name changes, Citus tries to re-assign - -- the global pid and doing so for Citus internal backends doesn't - -- require being in a transaction block and is safe. + -- the global pid but it does so only for Citus internal backends, + -- and doing so for Citus internal backends doesn't require being + -- in a transaction block and is safe. -- -- However, for the client external backends (like us here), Citus - -- doesn't try to re-assign the global pid if doing so requires catalog - -- access and we're outside of a transaction block. Note that in that - -- case the catalog access may only be needed to retrive the local node - -- id when the cached local node is invalidated like what just happened - -- here because of the failed citus_add_node() call made above. + -- doesn't re-assign the global pid because it's not needed and it's + -- not safe to do so outside of a transaction block. This is because, + -- it would require performing a catalog access to retrive the local + -- node id when the cached local node is invalidated like what just + -- happened here because of the failed citus_add_node() call made + -- above. -- -- So by failing here (rather than crashing), we ensure this behavior. ROLLBACK; diff --git a/src/test/regress/sql/remove_coordinator.sql b/src/test/regress/sql/remove_coordinator.sql index 10d703aba33..35a8a57189b 100644 --- a/src/test/regress/sql/remove_coordinator.sql +++ b/src/test/regress/sql/remove_coordinator.sql @@ -19,15 +19,17 @@ BEGIN; -- transaction block while Postgres does that. -- -- And when the application_name changes, Citus tries to re-assign - -- the global pid and doing so for Citus internal backends doesn't - -- require being in a transaction block and is safe. + -- the global pid but it does so only for Citus internal backends, + -- and doing so for Citus internal backends doesn't require being + -- in a transaction block and is safe. -- -- However, for the client external backends (like us here), Citus - -- doesn't try to re-assign the global pid if doing so requires catalog - -- access and we're outside of a transaction block. Note that in that - -- case the catalog access may only be needed to retrive the local node - -- id when the cached local node is invalidated like what just happened - -- here because of the failed citus_add_node() call made above. + -- doesn't re-assign the global pid because it's not needed and it's + -- not safe to do so outside of a transaction block. This is because, + -- it would require performing a catalog access to retrive the local + -- node id when the cached local node is invalidated like what just + -- happened here because of the failed citus_add_node() call made + -- above. -- -- So by failing here (rather than crashing), we ensure this behavior. ROLLBACK;