From 8e979f7ac6cd96cfe4f63e0fd1a79f74d42eb640 Mon Sep 17 00:00:00 2001 From: zhjwpku Date: Wed, 10 Jan 2024 18:15:19 +0800 Subject: [PATCH 1/3] [performance improvement] remove duplicate LoadShardList call (#7380) LoadShardList is called twice, which is not neccessary, and there is no need to sort the shard placement list since we only want to know the list length. --- src/backend/distributed/metadata/metadata_cache.c | 2 +- src/backend/distributed/utils/colocation_utils.c | 6 ++---- src/backend/distributed/utils/shardinterval_utils.c | 5 ++--- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/backend/distributed/metadata/metadata_cache.c b/src/backend/distributed/metadata/metadata_cache.c index b8983ba21..555365e68 100644 --- a/src/backend/distributed/metadata/metadata_cache.c +++ b/src/backend/distributed/metadata/metadata_cache.c @@ -381,7 +381,7 @@ EnsureModificationsCanRun(void) /* - * EnsureModificationsCanRunOnRelation firsts calls into EnsureModificationsCanRun() and + * EnsureModificationsCanRunOnRelation first calls into EnsureModificationsCanRun() and * then does one more additional check. The additional check is to give a proper error * message if any relation that is modified is replicated, as replicated tables use * 2PC and 2PC cannot happen when recovery is in progress. diff --git a/src/backend/distributed/utils/colocation_utils.c b/src/backend/distributed/utils/colocation_utils.c index b8db3c80f..c18919527 100644 --- a/src/backend/distributed/utils/colocation_utils.c +++ b/src/backend/distributed/utils/colocation_utils.c @@ -362,10 +362,8 @@ ErrorIfShardPlacementsNotColocated(Oid leftRelationId, Oid rightRelationId) leftRelationName, rightRelationName))); } - List *leftPlacementList = ShardPlacementListSortedByWorker( - leftShardId); - List *rightPlacementList = ShardPlacementListSortedByWorker( - rightShardId); + List *leftPlacementList = ShardPlacementList(leftShardId); + List *rightPlacementList = ShardPlacementList(rightShardId); if (list_length(leftPlacementList) != list_length(rightPlacementList)) { diff --git a/src/backend/distributed/utils/shardinterval_utils.c b/src/backend/distributed/utils/shardinterval_utils.c index 16d43ffdc..124bfbdf1 100644 --- a/src/backend/distributed/utils/shardinterval_utils.c +++ b/src/backend/distributed/utils/shardinterval_utils.c @@ -470,12 +470,11 @@ SingleReplicatedTable(Oid relationId) return false; } - List *shardIntervalList = LoadShardList(relationId); uint64 *shardIdPointer = NULL; - foreach_ptr(shardIdPointer, shardIntervalList) + foreach_ptr(shardIdPointer, shardList) { uint64 shardId = *shardIdPointer; - shardPlacementList = ShardPlacementListSortedByWorker(shardId); + shardPlacementList = ShardPlacementList(shardId); if (list_length(shardPlacementList) != 1) { From 9a91136a3d320ed7264874b9d24886b5822a1d1b Mon Sep 17 00:00:00 2001 From: LightDB Enterprise Postgres <88468605+hslightdb@users.noreply.github.com> Date: Wed, 10 Jan 2024 18:49:53 +0800 Subject: [PATCH 2/3] Fix timeout when underlying socket is changed in a MultiConnection (#7377) When there are multiple localhost entries in /etc/hosts like following /etc/hosts: ``` 127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4 ::1 localhost localhost.localdomain localhost6 localhost6.localdomain6 127.0.0.1 localhost ``` multi_cluster_management check will failed: ``` @@ -857,20 +857,21 @@ ERROR: group 14 already has a primary node -- check that you can add secondaries and unavailable nodes to a group SELECT groupid AS worker_2_group FROM pg_dist_node WHERE nodeport = :worker_2_port \gset SELECT 1 FROM master_add_node('localhost', 9998, groupid => :worker_1_group, noderole => 'secondary'); ?column? ---------- 1 (1 row) SELECT 1 FROM master_add_node('localhost', 9997, groupid => :worker_1_group, noderole => 'unavailable'); +WARNING: could not establish connection after 5000 ms ?column? ---------- 1 (1 row) ``` This actually isn't just a problem in test environments, but could occur as well during actual usage when a hostname in pg_dist_node resolves to multiple IPs and one of those IPs is unreachable. Postgres will then automatically continue with the next IP, but Citus should listen for events on the new socket. Not on the old one. Co-authored-by: chuhx43211 --- .../distributed/connection/connection_management.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/backend/distributed/connection/connection_management.c b/src/backend/distributed/connection/connection_management.c index 64ec1904f..f8e4816ed 100644 --- a/src/backend/distributed/connection/connection_management.c +++ b/src/backend/distributed/connection/connection_management.c @@ -1046,8 +1046,15 @@ FinishConnectionListEstablishment(List *multiConnectionList) continue; } - + bool beforePollSocket = PQsocket(connectionState->connection->pgConn); bool connectionStateChanged = MultiConnectionStatePoll(connectionState); + + if (beforePollSocket != PQsocket(connectionState->connection->pgConn)) + { + /* rebuild the wait events if MultiConnectionStatePoll() changed the socket */ + waitEventSetRebuild = true; + } + if (connectionStateChanged) { if (connectionState->phase != MULTI_CONNECTION_PHASE_CONNECTING) From 00068e07c53644087e153d8837dedd2b86732c26 Mon Sep 17 00:00:00 2001 From: Teja Mupparti Date: Tue, 12 Dec 2023 14:28:43 -0800 Subject: [PATCH 3/3] Fix the incorrect column count after ALTER TABLE, this fixes the bug #7378 (please read the analysis in the bug for more information) --- .../distributed/deparser/ruleutils_15.c | 11 ++- .../distributed/utils/citus_nodefuncs.c | 12 +++- .../expected/multi_alter_table_statements.out | 71 +++++++++++++++++++ .../sql/multi_alter_table_statements.sql | 26 +++++++ 4 files changed, 117 insertions(+), 3 deletions(-) diff --git a/src/backend/distributed/deparser/ruleutils_15.c b/src/backend/distributed/deparser/ruleutils_15.c index a84f8b113..018468d0b 100644 --- a/src/backend/distributed/deparser/ruleutils_15.c +++ b/src/backend/distributed/deparser/ruleutils_15.c @@ -1563,8 +1563,15 @@ set_join_column_names(deparse_namespace *dpns, RangeTblEntry *rte, /* Assert we processed the right number of columns */ #ifdef USE_ASSERT_CHECKING - while (i < colinfo->num_cols && colinfo->colnames[i] == NULL) - i++; + for (int col_index = 0; col_index < colinfo->num_cols; col_index++) + { + /* + * In the above processing-loops, "i" advances only if + * the column is not new, check if this is a new column. + */ + if (colinfo->is_new_col[col_index]) + i++; + } Assert(i == colinfo->num_cols); Assert(j == nnewcolumns); #endif diff --git a/src/backend/distributed/utils/citus_nodefuncs.c b/src/backend/distributed/utils/citus_nodefuncs.c index 55b83e6c5..0b03926f8 100644 --- a/src/backend/distributed/utils/citus_nodefuncs.c +++ b/src/backend/distributed/utils/citus_nodefuncs.c @@ -142,7 +142,17 @@ SetRangeTblExtraData(RangeTblEntry *rte, CitusRTEKind rteKind, char *fragmentSch fauxFunction->funcexpr = (Node *) fauxFuncExpr; /* set the column count to pass ruleutils checks, not used elsewhere */ - fauxFunction->funccolcount = list_length(rte->eref->colnames); + if (rte->relid != 0) + { + Relation rel = RelationIdGetRelation(rte->relid); + fauxFunction->funccolcount = RelationGetNumberOfAttributes(rel); + RelationClose(rel); + } + else + { + fauxFunction->funccolcount = list_length(rte->eref->colnames); + } + fauxFunction->funccolnames = funcColumnNames; fauxFunction->funccoltypes = funcColumnTypes; fauxFunction->funccoltypmods = funcColumnTypeMods; diff --git a/src/test/regress/expected/multi_alter_table_statements.out b/src/test/regress/expected/multi_alter_table_statements.out index c24927504..398fa8f7f 100644 --- a/src/test/regress/expected/multi_alter_table_statements.out +++ b/src/test/regress/expected/multi_alter_table_statements.out @@ -1349,6 +1349,77 @@ SELECT pg_identify_object_as_address(classid, objid, objsubid) from pg_catalog.p (schema,{test_schema_for_sequence_propagation},{}) (1 row) +-- Bug: https://github.com/citusdata/citus/issues/7378 +-- Create a reference table +CREATE TABLE tbl_ref(row_id integer primary key); +INSERT INTO tbl_ref VALUES (1), (2); +SELECT create_reference_table('tbl_ref'); +NOTICE: Copying data from local table... +NOTICE: copying the data has completed +DETAIL: The local data in the table is no longer visible, but is still on disk. +HINT: To remove the local data, run: SELECT truncate_local_data_after_distributing_table($$multi_alter_table_statements.tbl_ref$$) + create_reference_table +--------------------------------------------------------------------- + +(1 row) + +-- Create a distributed table +CREATE TABLE tbl_dist(series_id integer); +INSERT INTO tbl_dist VALUES (1), (1), (2), (2); +SELECT create_distributed_table('tbl_dist', 'series_id'); +NOTICE: Copying data from local table... +NOTICE: copying the data has completed +DETAIL: The local data in the table is no longer visible, but is still on disk. +HINT: To remove the local data, run: SELECT truncate_local_data_after_distributing_table($$multi_alter_table_statements.tbl_dist$$) + create_distributed_table +--------------------------------------------------------------------- + +(1 row) + +-- Create a view that joins the distributed table with the reference table on the distribution key. +CREATE VIEW vw_citus_views as +SELECT d.series_id FROM tbl_dist d JOIN tbl_ref r ON d.series_id = r.row_id; +-- The view initially works fine +SELECT * FROM vw_citus_views ORDER BY 1; + series_id +--------------------------------------------------------------------- + 1 + 1 + 2 + 2 +(4 rows) + +-- Now, alter the table +ALTER TABLE tbl_ref ADD COLUMN category1 varchar(50); +SELECT * FROM vw_citus_views ORDER BY 1; + series_id +--------------------------------------------------------------------- + 1 + 1 + 2 + 2 +(4 rows) + +ALTER TABLE tbl_ref ADD COLUMN category2 varchar(50); +SELECT * FROM vw_citus_views ORDER BY 1; + series_id +--------------------------------------------------------------------- + 1 + 1 + 2 + 2 +(4 rows) + +ALTER TABLE tbl_ref DROP COLUMN category1; +SELECT * FROM vw_citus_views ORDER BY 1; + series_id +--------------------------------------------------------------------- + 1 + 1 + 2 + 2 +(4 rows) + SET client_min_messages TO WARNING; DROP SCHEMA test_schema_for_sequence_propagation CASCADE; DROP TABLE table_without_sequence; diff --git a/src/test/regress/sql/multi_alter_table_statements.sql b/src/test/regress/sql/multi_alter_table_statements.sql index 10e52cb37..0674a68a6 100644 --- a/src/test/regress/sql/multi_alter_table_statements.sql +++ b/src/test/regress/sql/multi_alter_table_statements.sql @@ -727,6 +727,32 @@ ALTER TABLE table_without_sequence ADD COLUMN x BIGINT DEFAULT nextval('test_sch SELECT pg_identify_object_as_address(classid, objid, objsubid) from pg_catalog.pg_dist_object WHERE objid IN ('test_schema_for_sequence_propagation.seq_10'::regclass); SELECT pg_identify_object_as_address(classid, objid, objsubid) from pg_catalog.pg_dist_object WHERE objid IN ('test_schema_for_sequence_propagation'::regnamespace); +-- Bug: https://github.com/citusdata/citus/issues/7378 + +-- Create a reference table +CREATE TABLE tbl_ref(row_id integer primary key); +INSERT INTO tbl_ref VALUES (1), (2); +SELECT create_reference_table('tbl_ref'); + +-- Create a distributed table +CREATE TABLE tbl_dist(series_id integer); +INSERT INTO tbl_dist VALUES (1), (1), (2), (2); +SELECT create_distributed_table('tbl_dist', 'series_id'); + +-- Create a view that joins the distributed table with the reference table on the distribution key. +CREATE VIEW vw_citus_views as +SELECT d.series_id FROM tbl_dist d JOIN tbl_ref r ON d.series_id = r.row_id; + +-- The view initially works fine +SELECT * FROM vw_citus_views ORDER BY 1; +-- Now, alter the table +ALTER TABLE tbl_ref ADD COLUMN category1 varchar(50); +SELECT * FROM vw_citus_views ORDER BY 1; +ALTER TABLE tbl_ref ADD COLUMN category2 varchar(50); +SELECT * FROM vw_citus_views ORDER BY 1; +ALTER TABLE tbl_ref DROP COLUMN category1; +SELECT * FROM vw_citus_views ORDER BY 1; + SET client_min_messages TO WARNING; DROP SCHEMA test_schema_for_sequence_propagation CASCADE; DROP TABLE table_without_sequence;