Some minor improvements on top of 5314 (#5428)

* Refactor some checks in citus local tables

* all existing citus local tables are auto converted after upgrade

* Update warning messages in CreateCitusLocalTable

* Hide notice msg for auto converting local tables

* Hide hint msg

Co-authored-by: Ahmet Gedemenli <afgedemenli@gmail.com>

src/backend/distributed/commands/citus_add_local_table_to_metadata.c

@@ -52,8 +52,7 @@ static void ErrorIfAddingPartitionTableToMetadata(Oid relationId);
 static void ErrorIfUnsupportedCreateCitusLocalTable(Relation relation);
 static void ErrorIfUnsupportedCitusLocalTableKind(Oid relationId);
 static void ErrorIfUnsupportedCitusLocalColumnDefinition(Relation relation);
-static void NoticeIfAutoConvertingLocalTables(bool autoConverted);
+static void NoticeIfAutoConvertingLocalTables(bool autoConverted, Oid relationId);
-static void NoticeRelationIsAlreadyAddedToMetadata(Oid relationId);
 static CascadeOperationType GetCascadeTypeForCitusLocalTables(bool autoConverted);
 static List * GetShellTableDDLEventsForCitusLocalTable(Oid relationId);
 static uint64 ConvertLocalTableToShard(Oid relationId);
@@ -213,8 +212,6 @@ CreateCitusLocalTable(Oid relationId, bool cascadeViaForeignKeys, bool autoConve
 		UpdateAutoConvertedForConnectedRelations(list_make1_oid(relationId),
 												 autoConverted);
 
-		NoticeRelationIsAlreadyAddedToMetadata(relationId);
-
 		return;
 	}
 
@@ -242,7 +239,7 @@ CreateCitusLocalTable(Oid relationId, bool cascadeViaForeignKeys, bool autoConve
 	 */
 	relation_close(relation, NoLock);
 
-	NoticeIfAutoConvertingLocalTables(autoConverted);
+	NoticeIfAutoConvertingLocalTables(autoConverted, relationId);
 
 	if (TableHasExternalForeignKeys(relationId))
 	{
@@ -565,44 +562,29 @@ ErrorIfUnsupportedCitusLocalColumnDefinition(Relation relation)
  * will be undistributed automatically, if it gets disconnected from reference table(s).
  */
 static void
-NoticeIfAutoConvertingLocalTables(bool autoConverted)
+NoticeIfAutoConvertingLocalTables(bool autoConverted, Oid relationId)
 {
 	if (autoConverted && ShouldEnableLocalReferenceForeignKeys())
 	{
+		char *qualifiedRelationName = generate_qualified_relation_name(relationId);
+
 		/*
 		 * When foreign keys between reference tables and postgres tables are
 		 * enabled, we automatically undistribute citus local tables that are
 		 * not chained with any reference tables back to postgres tables.
 		 * So give a warning to user for that.
 		 */
-		ereport(NOTICE, (errmsg("local tables that are added to metadata but not "
+		ereport(NOTICE, (errmsg("local tables that are added to metadata automatically "
-								"chained with reference tables via foreign keys might "
+								"by citus, but not chained with reference tables via "
-								"be automatically converted back to postgres tables"),
+								"foreign keys might be automatically converted back to "
-						 errhint("Consider setting "
+								"postgres tables"),
-								 "citus.enable_local_reference_table_foreign_keys "
+						 errhint("Executing citus_add_local_table_to_metadata($$%s$$) "
-								 "to 'off' to disable this behavior")));
+								 "prevents this for the given relation, and all of the "
+								 "connected relations", qualifiedRelationName)));
 	}
 }
 
 
-/*
- * NoticeRelationIsAlreadyAddedToMetadata logs a NOTICE message that informs the user
- * that the given relation is already added to metadata.
- * We set the field autoConverted for these cases to false.
- * This function tells the user that the given table will not be removed from metadata,
- * as it was a user request.
- */
-static void
-NoticeRelationIsAlreadyAddedToMetadata(Oid relationId)
-{
-	char *relname = get_rel_name(relationId);
-	ereport(NOTICE, (errmsg("relation \"%s\" is already added to metadata", relname),
-					 errdetail("This relation will not be removed from metadata even if "
-							   "it is not connected to a reference table via foreign "
-							   "key(s), since it is added to metadata by the user")));
-}
-
-
 /*
  * GetCascadeTypeForCitusLocalTables returns CASCADE_AUTO_ADD_LOCAL_TABLE_TO_METADATA
  * if autoConverted is true. Returns CASCADE_USER_ADD_LOCAL_TABLE_TO_METADATA otherwise.

src/backend/distributed/commands/table.c

@@ -77,8 +77,9 @@ static bool RelationIdListContainsCitusTableType(List *relationIdList,
 static bool RelationIdListContainsPostgresTable(List *relationIdList);
 static void ConvertPostgresLocalTablesToCitusLocalTables(
 	AlterTableStmt *alterTableStatement);
-static bool RangeVarListHasRelationConvertedByUser(List *relationRangeVarList,
+static bool RangeVarListHasLocalRelationConvertedByUser(List *relationRangeVarList,
-												   AlterTableStmt *alterTableStatement);
+														AlterTableStmt *
+														alterTableStatement);
 static int CompareRangeVarsByOid(const void *leftElement, const void *rightElement);
 static List * GetAlterTableAddFKeyRightRelationIdList(
 	AlterTableStmt *alterTableStatement);
@@ -1269,13 +1270,9 @@ ConvertPostgresLocalTablesToCitusLocalTables(AlterTableStmt *alterTableStatement
 	 * table to a citus local table.
 	 */
 	relationRangeVarList = SortList(relationRangeVarList, CompareRangeVarsByOid);
-
+	bool containsAnyUserConvertedLocalRelation =
-	bool autoConverted = true;
+		RangeVarListHasLocalRelationConvertedByUser(relationRangeVarList,
-
+													alterTableStatement);
-	if (RangeVarListHasRelationConvertedByUser(relationRangeVarList, alterTableStatement))
-	{
-		autoConverted = false;
-	}
 
 	/*
 	 * Here we should operate on RangeVar objects since relations oid's would
@@ -1299,20 +1296,22 @@ ConvertPostgresLocalTablesToCitusLocalTables(AlterTableStmt *alterTableStatement
 		else if (IsCitusTableType(relationId, CITUS_LOCAL_TABLE))
 		{
 			CitusTableCacheEntry *entry = GetCitusTableCacheEntry(relationId);
-			if (!entry->autoConverted || autoConverted)
+			if (!entry->autoConverted)
 			{
 				/*
-				 * relationRangeVarList has also reference and citus local tables
+				 * This citus local table is already added to the metadata
-				 * involved in this ADD FOREIGN KEY command. Moreover, even if
+				 * by the user, so no further operation needed.
-				 * relationId was belonging to a postgres  local table initially,
+				 */
-				 * we might had already converted it to a citus local table by cascading.
+				continue;
-				 * Note that we cannot skip here, if the relation is marked as
+			}
-				 * auto-converted, but we are marking the relations in the command
+			else if (!containsAnyUserConvertedLocalRelation)
-				 * auto-converted = false. Because in that case, it means that this
+			{
-				 * relation is marked as auto-converted because of a connection
+				/*
-				 * with a reference table, but now it is connected to a Citus
+				 * We are safe to skip this relation because none of the citus local
-				 * Local Table. In that case, we need to mark this relation as
+				 * tables involved are manually added to the metadata by the user.
-				 * auto-converted = false, so cannot do a "continue" here.
+				 * This implies that all the Citus local tables involved are marked
+				 * as autoConverted = true and there is no chance to update
+				 * autoConverted = false.
 				 */
 				continue;
 			}
@@ -1353,6 +1352,24 @@ ConvertPostgresLocalTablesToCitusLocalTables(AlterTableStmt *alterTableStatement
 			}
 			else
 			{
+				/*
+				 * There might be two scenarios:
+				 *
+				 *   a) A user created foreign key from a reference table
+				 *      to Postgres local table(s) or Citus local table(s)
+				 *      where all of the citus local tables involved are auto
+				 *      converted. In that case, we mark the new table as auto
+				 *      converted as well.
+				 *
+				 *   b) A user created foreign key from a reference table
+				 *      to Postgres local table(s) or Citus local table(s)
+				 *      where at least one of the citus local tables
+				 *      involved is not auto converted. In that case, we mark
+				 *      this new Citus local table as autoConverted = false
+				 *      as well. Because our logic is to keep all the connected
+				 *      Citus local tables to have the same autoConverted value.
+				 */
+				bool autoConverted = containsAnyUserConvertedLocalRelation ? false : true;
 				CreateCitusLocalTable(relationId, cascade, autoConverted);
 			}
 		}
@@ -1380,13 +1397,13 @@ ConvertPostgresLocalTablesToCitusLocalTables(AlterTableStmt *alterTableStatement
 
 
 /*
- * RangeVarListHasRelationConvertedByUser takes a list of relations and returns true
+ * RangeVarListHasLocalRelationConvertedByUser takes a list of relations and returns true
  * if any of these relations is marked as auto-converted = false. Returns true otherwise.
  * This function also takes the current alterTableStatement command, to obtain the
  * necessary locks.
  */
 static bool
-RangeVarListHasRelationConvertedByUser(List *relationRangeVarList,
+RangeVarListHasLocalRelationConvertedByUser(List *relationRangeVarList,
 											AlterTableStmt *alterTableStatement)
 {
 	RangeVar *relationRangeVar;

src/backend/distributed/sql/citus--10.2-3--11.0-1.sql

@@ -8,7 +8,11 @@
 
 DROP FUNCTION IF EXISTS pg_catalog.master_apply_delete_command(text);
 DROP FUNCTION pg_catalog.master_get_table_metadata(text);
+
+-- all existing citus local tables are auto converted
+-- none of the other tables can have auto-converted as true
 ALTER TABLE pg_catalog.pg_dist_partition ADD COLUMN autoconverted boolean DEFAULT false;
+UPDATE pg_catalog.pg_dist_partition SET autoconverted = TRUE WHERE partmethod = 'n' AND repmodel = 's';
 
 REVOKE ALL ON FUNCTION start_metadata_sync_to_node(text, integer) FROM PUBLIC;
 REVOKE ALL ON FUNCTION stop_metadata_sync_to_node(text, integer,bool) FROM PUBLIC;

src/test/regress/bin/normalize.sed

@@ -199,9 +199,9 @@ s/citus_local_table_4_idx_[0-9]+/citus_local_table_4_idx_xxxxxx/g
 s/citus_local_table_4_[0-9]+/citus_local_table_4_xxxxxx/g
 s/ERROR:  cannot append to shardId [0-9]+/ERROR:  cannot append to shardId xxxxxx/g
 
-# hide warning/hint message that we get when executing create_citus_local_table
+# hide notice/hint message that we get when converting local tables automatically
-/local tables that are added to metadata but not chained with reference tables via foreign keys might be automatically converted back to postgres tables$/d
+/local tables that are added to metadata automatically by citus, but not chained with reference tables via foreign keys might be automatically converted back to postgres tables$/d
-/Consider setting citus.enable_local_reference_table_foreign_keys to 'off' to disable this behavior$/d
+/Executing citus_add_local_table_to_metadata(.*) prevents this for the given relation, and all of the connected relations$/d
 
 # normalize partitioned table shard constraint name errors for upgrade_partition_constraints_(before|after)
 s/^(ERROR:  child table is missing constraint "\w+)_([0-9])+"/\1_xxxxxx"/g