we validate constraint as well if the statement is alter domain drop constraint (#6125)

src/backend/distributed/commands/domain.c

@@ -254,7 +254,37 @@ AlterDomainStmtObjectAddress(Node *node, bool missing_ok, bool isPostprocess)
 	AlterDomainStmt *stmt = castNode(AlterDomainStmt, node);
 
 	TypeName *domainName = makeTypeNameFromNameList(stmt->typeName);
-	return GetDomainAddressByName(domainName, missing_ok);
+	List *domainObjectAddresses = GetDomainAddressByName(domainName, missing_ok);
+
+	/*  the code-path only supports a single object */
+	Assert(list_length(domainObjectAddresses) == 1);
+
+	/* We have already asserted that we have exactly 1 address in the addresses. */
+	ObjectAddress *address = linitial(domainObjectAddresses);
+
+	Oid domainOid = address->objectId;
+	bool isDropConstraintStmt = (stmt->subtype == 'X');
+	if (!isPostprocess && isDropConstraintStmt && OidIsValid(domainOid))
+	{
+		/*
+		 * we validate constraint if we are not in postprocess yet. It should have
+		 * been already dropped at postprocess, so we do not validate in postprocess.
+		 */
+		char *constraintName = stmt->name;
+		Oid constraintOid = get_domain_constraint_oid(domainOid, constraintName,
+													  missing_ok);
+		if (!OidIsValid(constraintOid))
+		{
+			/*
+			 * Although the domain is valid, the constraint is not. Eventually, PG will
+			 * throw an error. To prevent diverging outputs between Citus and PG, we treat
+			 * the domain as invalid.
+			 */
+			address->objectId = InvalidOid;
+		}
+	}
+
+	return list_make1(address);
 }
 
 

src/backend/distributed/commands/utility_hook.c

@@ -377,7 +377,7 @@ ProcessUtilityInternal(PlannedStmt *pstmt,
 {
 	Node *parsetree = pstmt->utilityStmt;
 	List *ddlJobs = NIL;
-	bool distOpsHasInvalidObject = false;
+	DistOpsValidationState distOpsValidationState = HasNoneValidObject;
 
 	if (IsA(parsetree, ExplainStmt) &&
 		IsA(((ExplainStmt *) parsetree)->query, Query))
@@ -547,15 +547,15 @@ ProcessUtilityInternal(PlannedStmt *pstmt,
 		/*
 		 * Preprocess and qualify steps can cause pg tests to fail because of the
 		 * unwanted citus related warnings or early error logs related to invalid address.
-		 * Therefore, we first check if all addresses in the given statement are valid.
+		 * Therefore, we first check if any address in the given statement is valid.
-		 * Then, we do not execute qualify and preprocess if any address is invalid to
+		 * Then, we do not execute qualify and preprocess if none of the addresses are valid
-		 * prevent before-mentioned citus related messages. PG will complain about the
+		 * to prevent before-mentioned citus related messages. PG will complain about the
 		 * invalid address, so we are safe to not execute qualify and preprocess. Also
 		 * note that we should not guard any step after standardProcess_Utility with
-		 * the flag distOpsHasInvalidObject because PG would have already failed the
+		 * the enum state distOpsValidationState because PG would have already failed the
 		 * transaction.
 		 */
-		distOpsHasInvalidObject = DistOpsHasInvalidObject(parsetree, ops);
+		distOpsValidationState = DistOpsValidityState(parsetree, ops);
 
 		/*
 		 * For some statements Citus defines a Qualify function. The goal of this function
@@ -565,13 +565,15 @@ ProcessUtilityInternal(PlannedStmt *pstmt,
 		 * and fill them out how postgres would resolve them. This makes subsequent
 		 * deserialize calls for the statement portable to other postgres servers, the
 		 * workers in our case.
+		 * If there are no valid objects, let's skip the qualify and
+		 * preprocess, and do not diverge from Postgres in terms of error messages.
 		 */
-		if (ops && ops->qualify && !distOpsHasInvalidObject)
+		if (ops && ops->qualify && distOpsValidationState != HasNoneValidObject)
 		{
 			ops->qualify(parsetree);
 		}
 
-		if (ops && ops->preprocess && !distOpsHasInvalidObject)
+		if (ops && ops->preprocess && distOpsValidationState != HasNoneValidObject)
 		{
 			ddlJobs = ops->preprocess(parsetree, queryString, context);
 		}

src/backend/distributed/utils/citus_depended_object.c

@@ -49,7 +49,6 @@ bool HideCitusDependentObjects = false;
 
 static Node * CreateCitusDependentObjectExpr(int pgMetaTableVarno, int pgMetaTableOid);
 static List * GetCitusDependedObjectArgs(int pgMetaTableVarno, int pgMetaTableOid);
-static bool StatementContainsIfExist(Node *node);
 static bool AlterRoleSetStatementContainsAll(Node *node);
 
 /*
@@ -314,13 +313,10 @@ GetCitusDependedObjectArgs(int pgMetaTableVarno, int pgMetaTableOid)
 
 
 /*
- * DistOpsHasInvalidObject returns true if any address in the given node
+ * DistOpsValidityState returns validation state for given dist ops.
- * is invalid; otherwise, returns false. If ops is null or it has no
- * implemented address method, we return false. We also have some dist ops
- * for which we should not validate and return false.
  */
-bool
+DistOpsValidationState
-DistOpsHasInvalidObject(Node *node, const DistributeObjectOps *ops)
+DistOpsValidityState(Node *node, const DistributeObjectOps *ops)
 {
 	if (ops && ops->operationType == DIST_OPS_CREATE)
 	{
@@ -328,15 +324,7 @@ DistOpsHasInvalidObject(Node *node, const DistributeObjectOps *ops)
 		 * We should not validate CREATE statements because no address exists
 		 * here yet.
 		 */
-		return false;
+		return NoAddressResolutionRequired;
-	}
-	else if (StatementContainsIfExist(node))
-	{
-		/*
-		 * We should not validate '[DROP|ALTER] IF EXISTS' statements because it is ok
-		 * by the semantics even if any object is invalid.
-		 */
-		return false;
 	}
 	else if (AlterRoleSetStatementContainsAll(node))
 	{
@@ -344,7 +332,7 @@ DistOpsHasInvalidObject(Node *node, const DistributeObjectOps *ops)
 		 * We should not validate 'ALTER ROLE ALL [SET|UNSET] because for the role ALL
 		 * AlterRoleSetStmtObjectAddress returns an invalid address even though it should not.
 		 */
-		return false;
+		return NoAddressResolutionRequired;
 	}
 
 	if (ops && ops->address)
@@ -356,117 +344,20 @@ DistOpsHasInvalidObject(Node *node, const DistributeObjectOps *ops)
 		ObjectAddress *objectAddress = NULL;
 		foreach_ptr(objectAddress, objectAddresses)
 		{
-			if (!OidIsValid(objectAddress->objectId))
+			if (OidIsValid(objectAddress->objectId))
 			{
-				return true;
+				/* found one valid object */
-			}
+				return HasAtLeastOneValidObject;
 			}
 		}
 
-	return false;
+		/* no valid objects */
+		return HasNoneValidObject;
 	}
-
+	else
-
-/*
- * StatementContainsIfExist returns true if the statement contains
- * IF EXIST syntax.
- */
-static bool
-StatementContainsIfExist(Node *node)
 	{
-	if (node == NULL)
+		/* if the object doesn't have address defined, we donot validate */
-	{
+		return NoAddressResolutionRequired;
-		return false;
-	}
-
-	switch (nodeTag(node))
-	{
-		case T_DropStmt:
-		{
-			DropStmt *dropStmt = castNode(DropStmt, node);
-			return dropStmt->missing_ok;
-		}
-
-		case T_DropRoleStmt:
-		{
-			DropRoleStmt *dropRoleStmt = castNode(DropRoleStmt, node);
-			return dropRoleStmt->missing_ok;
-		}
-
-		case T_DropdbStmt:
-		{
-			DropdbStmt *dropdbStmt = castNode(DropdbStmt, node);
-			return dropdbStmt->missing_ok;
-		}
-
-		case T_DropTableSpaceStmt:
-		{
-			DropTableSpaceStmt *dropTableSpaceStmt = castNode(DropTableSpaceStmt, node);
-			return dropTableSpaceStmt->missing_ok;
-		}
-
-		case T_DropUserMappingStmt:
-		{
-			DropUserMappingStmt *dropUserMappingStmt = castNode(DropUserMappingStmt,
-																node);
-			return dropUserMappingStmt->missing_ok;
-		}
-
-		case T_DropSubscriptionStmt:
-		{
-			DropSubscriptionStmt *dropSubscriptionStmt = castNode(DropSubscriptionStmt,
-																  node);
-			return dropSubscriptionStmt->missing_ok;
-		}
-
-		case T_AlterTableStmt:
-		{
-			AlterTableStmt *alterTableStmt = castNode(AlterTableStmt, node);
-			return alterTableStmt->missing_ok;
-		}
-
-		case T_AlterDomainStmt:
-		{
-			AlterDomainStmt *alterDomainStmt = castNode(AlterDomainStmt, node);
-			return alterDomainStmt->missing_ok;
-		}
-
-		case T_AlterSeqStmt:
-		{
-			AlterSeqStmt *alterSeqStmt = castNode(AlterSeqStmt, node);
-			return alterSeqStmt->missing_ok;
-		}
-
-		case T_AlterStatsStmt:
-		{
-			AlterStatsStmt *alterStatsStmt = castNode(AlterStatsStmt, node);
-			return alterStatsStmt->missing_ok;
-		}
-
-		case T_RenameStmt:
-		{
-			RenameStmt *renameStmt = castNode(RenameStmt, node);
-			return renameStmt->missing_ok;
-		}
-
-		case T_AlterObjectSchemaStmt:
-		{
-			AlterObjectSchemaStmt *alterObjectSchemaStmt = castNode(AlterObjectSchemaStmt,
-																	node);
-			return alterObjectSchemaStmt->missing_ok;
-		}
-
-		case T_AlterTSConfigurationStmt:
-		{
-			AlterTSConfigurationStmt *alterTSConfigurationStmt = castNode(
-				AlterTSConfigurationStmt, node);
-			return alterTSConfigurationStmt->missing_ok;
-		}
-
-		default:
-		{
-			return false;
-		}
 	}
 }
 

src/include/distributed/citus_depended_object.h

@@ -18,11 +18,20 @@
 
 extern bool HideCitusDependentObjects;
 
+/* DistOpsValidationState to be used to determine validity of dist ops */
+typedef enum DistOpsValidationState
+{
+	HasAtLeastOneValidObject,
+	HasNoneValidObject,
+	NoAddressResolutionRequired
+} DistOpsValidationState;
+
 extern void SetLocalClientMinMessagesIfRunningPGTests(int
 													  clientMinMessageLevel);
 extern void SetLocalHideCitusDependentObjectsDisabledWhenAlreadyEnabled(void);
 extern bool HideCitusDependentObjectsOnQueriesOfPgMetaTables(Node *node, void *context);
 extern bool IsPgLocksTable(RangeTblEntry *rte);
-extern bool DistOpsHasInvalidObject(Node *node, const DistributeObjectOps *ops);
+extern DistOpsValidationState DistOpsValidityState(Node *node, const
+												   DistributeObjectOps *ops);
 
 #endif /* CITUS_DEPENDED_OBJECT_H */

src/test/regress/expected/distributed_domain.out

@@ -636,7 +636,7 @@ SELECT * FROM run_command_on_workers($$ SELECT null::distributed_domain.alter_ad
 (2 rows)
 
 ALTER DOMAIN alter_add_constraint DROP CONSTRAINT IF EXISTS check_distinct;
-NOTICE:  constraint "check_distinct" of domain "distributed_domain.alter_add_constraint" does not exist, skipping
+NOTICE:  constraint "check_distinct" of domain "alter_add_constraint" does not exist, skipping
 ALTER DOMAIN alter_add_constraint ADD CONSTRAINT check_distinct CHECK (sql_is_distinct_from(value, null));
 ALTER DOMAIN alter_add_constraint RENAME CONSTRAINT check_distinct TO check_distinct_renamed;
 ALTER DOMAIN alter_add_constraint DROP CONSTRAINT check_distinct_renamed;