From f1ecbc3a53bfc5190a5fc06d22f2e040604287e6 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Sun, 17 Jan 2021 20:43:33 +0300 Subject: [PATCH] Fix segfault when adding/dropping fkey from ref to citus local via remote exec (#4528) --- src/backend/distributed/commands/table.c | 5 ++-- .../distributed/metadata/metadata_utility.c | 24 +++++++++++++++++++ src/include/distributed/metadata_utility.h | 1 + .../expected/ref_citus_local_fkeys.out | 6 +++++ .../regress/sql/ref_citus_local_fkeys.sql | 7 ++++++ 5 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/backend/distributed/commands/table.c b/src/backend/distributed/commands/table.c index e39d3d073..60c59ba32 100644 --- a/src/backend/distributed/commands/table.c +++ b/src/backend/distributed/commands/table.c @@ -2049,8 +2049,9 @@ SetInterShardDDLTaskPlacementList(Task *task, ShardInterval *leftShardInterval, * to a citus local table, then we will execute ADD/DROP constraint * command only for coordinator placement of reference table. */ - task->taskPlacementList = GroupShardPlacementsForTableOnGroup(leftRelationId, - COORDINATOR_GROUP_ID); + uint64 leftShardId = leftShardInterval->shardId; + task->taskPlacementList = ActiveShardPlacementListOnGroup(leftShardId, + COORDINATOR_GROUP_ID); } else { diff --git a/src/backend/distributed/metadata/metadata_utility.c b/src/backend/distributed/metadata/metadata_utility.c index 7a6ef8a38..244d9aea8 100644 --- a/src/backend/distributed/metadata/metadata_utility.c +++ b/src/backend/distributed/metadata/metadata_utility.c @@ -963,6 +963,30 @@ NodeGroupHasShardPlacements(int32 groupId, bool onlyConsiderActivePlacements) } +/* + * ActiveShardPlacementListOnGroup returns a list of active shard placements + * that are sitting on group with groupId for given shardId. + */ +List * +ActiveShardPlacementListOnGroup(uint64 shardId, int32 groupId) +{ + List *activeShardPlacementListOnGroup = NIL; + + List *activePlacementList = ActiveShardPlacementList(shardId); + ShardPlacement *shardPlacement = NULL; + foreach_ptr(shardPlacement, activePlacementList) + { + if (shardPlacement->groupId == groupId) + { + activeShardPlacementListOnGroup = lappend(activeShardPlacementListOnGroup, + shardPlacement); + } + } + + return activeShardPlacementListOnGroup; +} + + /* * ActiveShardPlacementList finds shard placements for the given shardId from * system catalogs, chooses placements that are in active state, and returns diff --git a/src/include/distributed/metadata_utility.h b/src/include/distributed/metadata_utility.h index 2fc8474ff..452b2bd22 100644 --- a/src/include/distributed/metadata_utility.h +++ b/src/include/distributed/metadata_utility.h @@ -188,6 +188,7 @@ extern ShardInterval * CopyShardInterval(ShardInterval *srcInterval); extern uint64 ShardLength(uint64 shardId); extern bool NodeGroupHasShardPlacements(int32 groupId, bool onlyConsiderActivePlacements); +extern List * ActiveShardPlacementListOnGroup(uint64 shardId, int32 groupId); extern List * ActiveShardPlacementList(uint64 shardId); extern ShardPlacement * ActiveShardPlacement(uint64 shardId, bool missingOk); extern List * BuildShardPlacementList(ShardInterval *shardInterval); diff --git a/src/test/regress/expected/ref_citus_local_fkeys.out b/src/test/regress/expected/ref_citus_local_fkeys.out index 6f3c33ad1..44d723404 100644 --- a/src/test/regress/expected/ref_citus_local_fkeys.out +++ b/src/test/regress/expected/ref_citus_local_fkeys.out @@ -169,6 +169,12 @@ ROLLBACK; -- .. and we allow such foreign keys with NO ACTION behavior ALTER TABLE reference_table ADD CONSTRAINT fkey_ref_to_local FOREIGN KEY(r1) REFERENCES citus_local_table(l1) ON DELETE NO ACTION; NOTICE: executing the command locally: SELECT worker_apply_inter_shard_ddl_command (1506003, 'ref_citus_local_fkeys', 1506002, 'ref_citus_local_fkeys', 'ALTER TABLE reference_table ADD CONSTRAINT fkey_ref_to_local FOREIGN KEY(r1) REFERENCES citus_local_table(l1) ON DELETE NO ACTION;') +-- show that adding/dropping foreign keys from reference to citus local +-- tables works fine with remote execution too +SET citus.enable_local_execution TO OFF; +ALTER TABLE reference_table DROP CONSTRAINT fkey_ref_to_local; +ALTER TABLE reference_table ADD CONSTRAINT fkey_ref_to_local FOREIGN KEY(r1) REFERENCES citus_local_table(l1) ON DELETE NO ACTION; +SET citus.enable_local_execution TO ON; -- show that we are checking for foreign key constraint after defining, this should fail INSERT INTO reference_table VALUES (4); NOTICE: executing the command locally: INSERT INTO ref_citus_local_fkeys.reference_table_1506003 (r1) VALUES (4) diff --git a/src/test/regress/sql/ref_citus_local_fkeys.sql b/src/test/regress/sql/ref_citus_local_fkeys.sql index 25a73eb23..6a23e43ef 100644 --- a/src/test/regress/sql/ref_citus_local_fkeys.sql +++ b/src/test/regress/sql/ref_citus_local_fkeys.sql @@ -113,6 +113,13 @@ ROLLBACK; -- .. and we allow such foreign keys with NO ACTION behavior ALTER TABLE reference_table ADD CONSTRAINT fkey_ref_to_local FOREIGN KEY(r1) REFERENCES citus_local_table(l1) ON DELETE NO ACTION; +-- show that adding/dropping foreign keys from reference to citus local +-- tables works fine with remote execution too +SET citus.enable_local_execution TO OFF; +ALTER TABLE reference_table DROP CONSTRAINT fkey_ref_to_local; +ALTER TABLE reference_table ADD CONSTRAINT fkey_ref_to_local FOREIGN KEY(r1) REFERENCES citus_local_table(l1) ON DELETE NO ACTION; +SET citus.enable_local_execution TO ON; + -- show that we are checking for foreign key constraint after defining, this should fail INSERT INTO reference_table VALUES (4);