diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c index f1fb482b3..9553600d7 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 8f8bede2a..796b79604 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 02df7c28b..c57d7eee7 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;