Merge pull request #2651 from citusdata/fix_constraint_naming

Address constraint naming issue

cr: @pykello

src/backend/distributed/relay/relay_event_utility.c

@@ -51,7 +51,6 @@
 #include "utils/relcache.h"
 
 /* Local functions forward declarations */
-static void AppendShardIdToConstraintName(AlterTableCmd *command, uint64 shardId);
 static bool UpdateWholeRowColumnReferencesWalker(Node *node, uint64 *shardId);
 
 /* exports for SQL callable functions */
@@ -85,6 +84,7 @@ RelayEventExtendNames(Node *parseTree, char *schemaName, uint64 shardId)
 			 */
 
 			AlterTableStmt *alterTableStmt = (AlterTableStmt *) parseTree;
+			Oid relationId = InvalidOid;
 			char **relationName = &(alterTableStmt->relation->relname);
 			char **relationSchemaName = &(alterTableStmt->relation->schemaname);
 
@@ -104,6 +104,7 @@ RelayEventExtendNames(Node *parseTree, char *schemaName, uint64 shardId)
 				if (command->subtype == AT_AddConstraint)
 				{
 					Constraint *constraint = (Constraint *) command->def;
+					char **constraintName = &(constraint->conname);
 
 					if (constraint->contype == CONSTR_PRIMARY && constraint->indexname)
 					{
@@ -111,15 +112,30 @@ RelayEventExtendNames(Node *parseTree, char *schemaName, uint64 shardId)
 						AppendShardIdToName(indexName, shardId);
 					}
 
-					AppendShardIdToConstraintName(command, shardId);
+					AppendShardIdToName(constraintName, shardId);
 				}
-				else if (command->subtype == AT_DropConstraint)
+				else if (command->subtype == AT_DropConstraint ||
+						 command->subtype == AT_ValidateConstraint)
 				{
-					AppendShardIdToConstraintName(command, shardId);
+					char **constraintName = &(command->name);
+					Oid constraintOid = InvalidOid;
+					const bool constraintMissingOk = true;
+
+					if (!OidIsValid(relationId))
+					{
+						const bool rvMissingOk = false;
+						relationId = RangeVarGetRelid(alterTableStmt->relation,
+													  AccessShareLock,
+													  rvMissingOk);
 					}
-				else if (command->subtype == AT_ValidateConstraint)
+
+					constraintOid = get_relation_constraint_oid(relationId,
+																command->name,
+																constraintMissingOk);
+					if (!OidIsValid(constraintOid))
 					{
-					AppendShardIdToConstraintName(command, shardId);
+						AppendShardIdToName(constraintName, shardId);
+					}
 				}
 				else if (command->subtype == AT_ClusterOn)
 				{
@@ -585,30 +601,6 @@ RelayEventExtendNamesForInterShardCommands(Node *parseTree, uint64 leftShardId,
 }
 
 
-/*
- * AppendShardIdToConstraintName extends given constraint name with given
- * shardId. Note that we only extend constraint names if they correspond to
- * indexes, and the caller should verify that index correspondence before
- * calling this function.
- */
-static void
-AppendShardIdToConstraintName(AlterTableCmd *command, uint64 shardId)
-{
-	if (command->subtype == AT_AddConstraint)
-	{
-		Constraint *constraint = (Constraint *) command->def;
-		char **constraintName = &(constraint->conname);
-		AppendShardIdToName(constraintName, shardId);
-	}
-	else if (command->subtype == AT_DropConstraint ||
-			 command->subtype == AT_ValidateConstraint)
-	{
-		char **constraintName = &(command->name);
-		AppendShardIdToName(constraintName, shardId);
-	}
-}
-
-
 /*
  * UpdateWholeRowColumnReferencesWalker extends ColumnRef nodes that end with A_Star
  * with the given shardId.

src/test/regress/expected/multi_alter_table_add_constraints.out

@@ -424,6 +424,11 @@ ALTER TABLE products ADD CHECK(product_no <> 0);
 ERROR:  cannot create constraint without a name on a distributed table
 ALTER TABLE products ADD EXCLUDE USING btree (product_no with =);
 ERROR:  cannot create constraint without a name on a distributed table
+-- ... with names, we can add/drop the constraints just fine
+ALTER TABLE products ADD CONSTRAINT nonzero_product_no CHECK(product_no <> 0);
+ALTER TABLE products ADD CONSTRAINT uniq_product_no EXCLUDE USING btree (product_no with =);
+ALTER TABLE products DROP CONSTRAINT nonzero_product_no;
+ALTER TABLE products DROP CONSTRAINT uniq_product_no;
 DROP TABLE products;
 -- Tests with transactions 
 CREATE TABLE products (

src/test/regress/expected/multi_create_table_constraints.out

@@ -350,6 +350,20 @@ SELECT "Constraint", "Definition" FROM table_checks WHERE relid='public.check_ex
 (2 rows)
 
 \c - - - :master_port
+-- Index-based constraints are created with shard-extended names, but others
+-- (e.g. expression-based table CHECK constraints) do _not_ have shardids in
+-- their object names, _at least originally as designed_. At some point, we
+-- mistakenly started extending _all_ constraint names, but _only_ for ALTER
+-- TABLE ... ADD CONSTRAINT commands (yes, even non-index constraints). So now
+-- the _same_ constraint definition could result in a non-extended name if made
+-- using CREATE TABLE and another name if made using AT ... ADD CONSTRAINT. So
+-- DROP CONSTRAINT started erroring because _it_ was also changed to always do
+-- shard-id extension. We've fixed that by looking for the non-extended name
+-- first and using it for DROP or VALIDATE commands that could be targeting it.
+-- As for the actual test: drop a constraint created by CREATE TABLE ... CHECK,
+-- which per the above description would have been created with a non-extended
+-- object name, but previously would have failed DROP as DROP does extension.
+ALTER TABLE check_example DROP CONSTRAINT check_example_other_col_check;
 -- drop unnecessary tables
 DROP TABLE pk_on_non_part_col, uq_on_non_part_col CASCADE;
 DROP TABLE pk_on_part_col, uq_part_col, uq_two_columns CASCADE;

src/test/regress/sql/multi_alter_table_add_constraints.sql

@@ -367,6 +367,13 @@ ALTER TABLE products ADD PRIMARY KEY(product_no);
 ALTER TABLE products ADD CHECK(product_no <> 0);
 ALTER TABLE products ADD EXCLUDE USING btree (product_no with =);
 
+-- ... with names, we can add/drop the constraints just fine
+ALTER TABLE products ADD CONSTRAINT nonzero_product_no CHECK(product_no <> 0);
+ALTER TABLE products ADD CONSTRAINT uniq_product_no EXCLUDE USING btree (product_no with =);
+
+ALTER TABLE products DROP CONSTRAINT nonzero_product_no;
+ALTER TABLE products DROP CONSTRAINT uniq_product_no;
+
 DROP TABLE products;
 
 

src/test/regress/sql/multi_create_table_constraints.sql

@@ -230,6 +230,22 @@ SELECT "Column", "Type", "Definition" FROM index_attrs WHERE
 SELECT "Constraint", "Definition" FROM table_checks WHERE relid='public.check_example_365056'::regclass;
 \c - - - :master_port
 
+-- Index-based constraints are created with shard-extended names, but others
+-- (e.g. expression-based table CHECK constraints) do _not_ have shardids in
+-- their object names, _at least originally as designed_. At some point, we
+-- mistakenly started extending _all_ constraint names, but _only_ for ALTER
+-- TABLE ... ADD CONSTRAINT commands (yes, even non-index constraints). So now
+-- the _same_ constraint definition could result in a non-extended name if made
+-- using CREATE TABLE and another name if made using AT ... ADD CONSTRAINT. So
+-- DROP CONSTRAINT started erroring because _it_ was also changed to always do
+-- shard-id extension. We've fixed that by looking for the non-extended name
+-- first and using it for DROP or VALIDATE commands that could be targeting it.
+
+-- As for the actual test: drop a constraint created by CREATE TABLE ... CHECK,
+-- which per the above description would have been created with a non-extended
+-- object name, but previously would have failed DROP as DROP does extension.
+ALTER TABLE check_example DROP CONSTRAINT check_example_other_col_check;
+
 -- drop unnecessary tables
 DROP TABLE pk_on_non_part_col, uq_on_non_part_col CASCADE;
 DROP TABLE pk_on_part_col, uq_part_col, uq_two_columns CASCADE;