From 4ffe436bf9ef5f5b5e75e24506153f01c19747f2 Mon Sep 17 00:00:00 2001 From: aykut-bozkurt <51649454+aykut-bozkurt@users.noreply.github.com> Date: Wed, 3 Aug 2022 23:06:33 +0300 Subject: [PATCH] we validate constraint as well if the statement is alter domain drop constraint (#6125) --- src/backend/distributed/commands/domain.c | 32 +++- .../distributed/commands/utility_hook.c | 18 ++- .../distributed/utils/citus_depended_object.c | 137 ++---------------- .../distributed/citus_depended_object.h | 11 +- .../regress/expected/distributed_domain.out | 2 +- 5 files changed, 66 insertions(+), 134 deletions(-) diff --git a/src/backend/distributed/commands/domain.c b/src/backend/distributed/commands/domain.c index 6c1bea4fd..f14157278 100644 --- a/src/backend/distributed/commands/domain.c +++ b/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); } diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 38ff4a379..6e1ff984a 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/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. - * Then, we do not execute qualify and preprocess if any address is invalid to - * prevent before-mentioned citus related messages. PG will complain about the + * Therefore, we first check if any address in the given statement is valid. + * Then, we do not execute qualify and preprocess if none of the addresses are valid + * 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); } diff --git a/src/backend/distributed/utils/citus_depended_object.c b/src/backend/distributed/utils/citus_depended_object.c index 101db1856..83062ccda 100644 --- a/src/backend/distributed/utils/citus_depended_object.c +++ b/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 - * 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. + * DistOpsValidityState returns validation state for given dist ops. */ -bool -DistOpsHasInvalidObject(Node *node, const DistributeObjectOps *ops) +DistOpsValidationState +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; - } - 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; + return NoAddressResolutionRequired; } 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; } } + + /* no valid objects */ + return HasNoneValidObject; } - - return false; -} - - -/* - * StatementContainsIfExist returns true if the statement contains - * IF EXIST syntax. - */ -static bool -StatementContainsIfExist(Node *node) -{ - if (node == NULL) + else { - 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; - } + /* if the object doesn't have address defined, we donot validate */ + return NoAddressResolutionRequired; } } diff --git a/src/include/distributed/citus_depended_object.h b/src/include/distributed/citus_depended_object.h index 55b1369fb..72ecf322c 100644 --- a/src/include/distributed/citus_depended_object.h +++ b/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 */ diff --git a/src/test/regress/expected/distributed_domain.out b/src/test/regress/expected/distributed_domain.out index c34d11410..5043d4f05 100644 --- a/src/test/regress/expected/distributed_domain.out +++ b/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;