From 8ab696f7e2ce9d479370cbaa7c547b5e9424d81a Mon Sep 17 00:00:00 2001 From: Onder Kalaci Date: Wed, 13 Jul 2022 14:08:49 +0200 Subject: [PATCH 1/4] LOCK COMMAND does not require primaries at the start --- src/backend/distributed/metadata/metadata_sync.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/distributed/metadata/metadata_sync.c b/src/backend/distributed/metadata/metadata_sync.c index 2034c2e6f..aa6294ca2 100644 --- a/src/backend/distributed/metadata/metadata_sync.c +++ b/src/backend/distributed/metadata/metadata_sync.c @@ -2505,7 +2505,7 @@ SchemaOwnerName(Oid objectId) static bool HasMetadataWorkers(void) { - List *workerNodeList = ActivePrimaryNonCoordinatorNodeList(NoLock); + List *workerNodeList = ActiveReadableNonCoordinatorNodeList(); WorkerNode *workerNode = NULL; foreach_ptr(workerNode, workerNodeList) From b2e9a5baf15e6fbb461bd8f5476a947c6512be03 Mon Sep 17 00:00:00 2001 From: Onder Kalaci Date: Wed, 13 Jul 2022 14:11:18 +0200 Subject: [PATCH 2/4] Make sure citus_is_coordinator works on read replicas --- src/backend/distributed/metadata/node_metadata.c | 2 +- src/backend/distributed/operations/worker_node_manager.c | 6 +++--- src/include/distributed/worker_manager.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/backend/distributed/metadata/node_metadata.c b/src/backend/distributed/metadata/node_metadata.c index 23d4d3f4d..5603189c5 100644 --- a/src/backend/distributed/metadata/node_metadata.c +++ b/src/backend/distributed/metadata/node_metadata.c @@ -1687,7 +1687,7 @@ citus_is_coordinator(PG_FUNCTION_ARGS) bool isCoordinator = false; if (GetLocalGroupId() == COORDINATOR_GROUP_ID && - ActivePrimaryNodeCount() > 0) + ActiveReadableNodeCount() > 0) { isCoordinator = true; } diff --git a/src/backend/distributed/operations/worker_node_manager.c b/src/backend/distributed/operations/worker_node_manager.c index 16c0afb54..c516e27ef 100644 --- a/src/backend/distributed/operations/worker_node_manager.c +++ b/src/backend/distributed/operations/worker_node_manager.c @@ -94,12 +94,12 @@ ActivePrimaryNonCoordinatorNodeCount(void) /* - * ActivePrimaryNodeCount returns the number of groups with a primary in the cluster. + * ActiveReadableNodeCount returns the number of nodes in the cluster. */ uint32 -ActivePrimaryNodeCount(void) +ActiveReadableNodeCount(void) { - List *nodeList = ActivePrimaryNodeList(NoLock); + List *nodeList = ActiveReadableNodeList(); return list_length(nodeList); } diff --git a/src/include/distributed/worker_manager.h b/src/include/distributed/worker_manager.h index e861b8a65..cf1458047 100644 --- a/src/include/distributed/worker_manager.h +++ b/src/include/distributed/worker_manager.h @@ -72,7 +72,7 @@ extern WorkerNode * WorkerGetRoundRobinCandidateNode(List *workerNodeList, uint64 shardId, uint32 placementIndex); extern uint32 ActivePrimaryNonCoordinatorNodeCount(void); -extern uint32 ActivePrimaryNodeCount(void); +extern uint32 ActiveReadableNodeCount(void); extern List * ActivePrimaryNonCoordinatorNodeList(LOCKMODE lockMode); extern List * ActivePrimaryNodeList(LOCKMODE lockMode); extern List * ActivePrimaryRemoteNodeList(LOCKMODE lockMode); From 3c343d45633c95e249dfcb419dabc659e2b78066 Mon Sep 17 00:00:00 2001 From: Onder Kalaci Date: Wed, 13 Jul 2022 14:27:11 +0200 Subject: [PATCH 3/4] Add regression tests for LOCK command citus.use_secondary_nodes=always mode --- .../regress/expected/multi_follower_dml.out | 27 +++++++++++++++++++ src/test/regress/sql/multi_follower_dml.sql | 14 ++++++++++ 2 files changed, 41 insertions(+) diff --git a/src/test/regress/expected/multi_follower_dml.out b/src/test/regress/expected/multi_follower_dml.out index d6a5acd65..e343f0a6d 100644 --- a/src/test/regress/expected/multi_follower_dml.out +++ b/src/test/regress/expected/multi_follower_dml.out @@ -354,6 +354,33 @@ ERROR: writing to worker nodes is not currently allowed DETAIL: citus.use_secondary_nodes is set to 'always' SELECT * FROM citus_local_table ORDER BY a; ERROR: there is a shard placement in node group 0 but there are no nodes in that group +\c "port=57636 dbname=regression options='-c\ citus.use_secondary_nodes=always\ -c\ citus.cluster_name=second-cluster'" +-- when an existing read-replica is forked to become +-- another primary node, we sometimes have to use citus.use_secondary_nodes=always +-- even if the node is not in recovery mode. In those cases, allow LOCK +-- command on local / metadata tables, and also certain UDFs +SHOW citus.use_secondary_nodes; + citus.use_secondary_nodes +--------------------------------------------------------------------- + always +(1 row) + +SELECT pg_is_in_recovery(); + pg_is_in_recovery +--------------------------------------------------------------------- + f +(1 row) + +SELECT citus_is_coordinator(); + citus_is_coordinator +--------------------------------------------------------------------- + t +(1 row) + +BEGIN; + LOCK TABLE pg_dist_node IN SHARE ROW EXCLUSIVE MODE; + LOCK TABLE local IN SHARE ROW EXCLUSIVE MODE; +COMMIT; \c -reuse-previous=off regression - - :master_port DROP TABLE the_table; DROP TABLE reference_table; diff --git a/src/test/regress/sql/multi_follower_dml.sql b/src/test/regress/sql/multi_follower_dml.sql index dc03f258c..a3d548b12 100644 --- a/src/test/regress/sql/multi_follower_dml.sql +++ b/src/test/regress/sql/multi_follower_dml.sql @@ -163,6 +163,20 @@ SELECT * FROM reference_table ORDER BY a; INSERT INTO citus_local_table (a, b, z) VALUES (1, 2, 3); SELECT * FROM citus_local_table ORDER BY a; +\c "port=57636 dbname=regression options='-c\ citus.use_secondary_nodes=always\ -c\ citus.cluster_name=second-cluster'" + +-- when an existing read-replica is forked to become +-- another primary node, we sometimes have to use citus.use_secondary_nodes=always +-- even if the node is not in recovery mode. In those cases, allow LOCK +-- command on local / metadata tables, and also certain UDFs +SHOW citus.use_secondary_nodes; +SELECT pg_is_in_recovery(); +SELECT citus_is_coordinator(); +BEGIN; + LOCK TABLE pg_dist_node IN SHARE ROW EXCLUSIVE MODE; + LOCK TABLE local IN SHARE ROW EXCLUSIVE MODE; +COMMIT; + \c -reuse-previous=off regression - - :master_port DROP TABLE the_table; DROP TABLE reference_table; From 6cd7319f12701afd0c20301f35066e9477bccfd6 Mon Sep 17 00:00:00 2001 From: Onder Kalaci Date: Wed, 13 Jul 2022 14:58:30 +0200 Subject: [PATCH 4/4] Add more generic read-replica tests --- .../multi_follower_select_statements.out | 19 +++++++++++++++++++ .../sql/multi_follower_select_statements.sql | 6 ++++++ 2 files changed, 25 insertions(+) diff --git a/src/test/regress/expected/multi_follower_select_statements.out b/src/test/regress/expected/multi_follower_select_statements.out index 42c3058ee..3d0550c85 100644 --- a/src/test/regress/expected/multi_follower_select_statements.out +++ b/src/test/regress/expected/multi_follower_select_statements.out @@ -141,6 +141,25 @@ ORDER BY localhost | 9072 (2 rows) +-- basic helper utilities should work fine +SELECT citus_is_coordinator(); + citus_is_coordinator +--------------------------------------------------------------------- + t +(1 row) + +SELECT count(*) FROM citus_lock_waits; + count +--------------------------------------------------------------------- + 0 +(1 row) + +SELECT count(*) FROM citus_dist_stat_activity WHERE global_pid = citus_backend_gpid(); + count +--------------------------------------------------------------------- + 1 +(1 row) + -- okay, now let's play with nodecluster. If we change the cluster of our follower node -- queries should stat failing again, since there are no worker nodes in the new cluster \c "port=9070 dbname=regression options='-c\ citus.use_secondary_nodes=always\ -c\ citus.cluster_name=second-cluster'" diff --git a/src/test/regress/sql/multi_follower_select_statements.sql b/src/test/regress/sql/multi_follower_select_statements.sql index edaccc869..3b12ea467 100644 --- a/src/test/regress/sql/multi_follower_select_statements.sql +++ b/src/test/regress/sql/multi_follower_select_statements.sql @@ -89,6 +89,12 @@ FROM ORDER BY node_name, node_port; +-- basic helper utilities should work fine +SELECT citus_is_coordinator(); +SELECT count(*) FROM citus_lock_waits; +SELECT count(*) FROM citus_dist_stat_activity WHERE global_pid = citus_backend_gpid(); + + -- okay, now let's play with nodecluster. If we change the cluster of our follower node -- queries should stat failing again, since there are no worker nodes in the new cluster