From bf05bf51ecd647ce8af5df73470e04b55ff10334 Mon Sep 17 00:00:00 2001 From: Hanefi Onaldi Date: Mon, 18 Mar 2024 15:06:49 +0300 Subject: [PATCH 1/5] Refactor one helper function (#7562) The code looks simpler and easier to read now. --- src/backend/distributed/metadata/metadata_sync.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/src/backend/distributed/metadata/metadata_sync.c b/src/backend/distributed/metadata/metadata_sync.c index 14f5b4624..31d586e90 100644 --- a/src/backend/distributed/metadata/metadata_sync.c +++ b/src/backend/distributed/metadata/metadata_sync.c @@ -492,19 +492,7 @@ stop_metadata_sync_to_node(PG_FUNCTION_ARGS) bool ClusterHasKnownMetadataWorkers() { - bool workerWithMetadata = false; - - if (!IsCoordinator()) - { - workerWithMetadata = true; - } - - if (workerWithMetadata || HasMetadataWorkers()) - { - return true; - } - - return false; + return !IsCoordinator() || HasMetadataWorkers(); } From d129064280eb96c20773e9bf0bcd4d0a514639eb Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Tue, 19 Mar 2024 14:26:17 +0100 Subject: [PATCH 2/5] Refactor the code that supports node-wide object mgmt commands from non-main dbs (#7544) RunPreprocessNonMainDBCommand and RunPostprocessNonMainDBCommand are the entrypoints for this module. These functions are called from utility_hook.c to support some of the node-wide object management commands from non-main databases. To add support for a new command type, one needs to define a new NonMainDbDistributeObjectOps object and add it to GetNonMainDbDistributeObjectOps. --- .../non_main_db_distribute_object_ops.c | 351 +++++++++++++++ .../distributed/commands/utility_hook.c | 422 +----------------- src/include/distributed/commands.h | 4 + 3 files changed, 370 insertions(+), 407 deletions(-) create mode 100644 src/backend/distributed/commands/non_main_db_distribute_object_ops.c diff --git a/src/backend/distributed/commands/non_main_db_distribute_object_ops.c b/src/backend/distributed/commands/non_main_db_distribute_object_ops.c new file mode 100644 index 000000000..b777936d3 --- /dev/null +++ b/src/backend/distributed/commands/non_main_db_distribute_object_ops.c @@ -0,0 +1,351 @@ +/*------------------------------------------------------------------------- + * + * non_main_db_distribute_object_ops.c + * + * Routines to support node-wide object management commands from non-main + * databases. + * + * RunPreprocessNonMainDBCommand and RunPostprocessNonMainDBCommand are + * the entrypoints for this module. These functions are called from + * utility_hook.c to support some of the node-wide object management + * commands from non-main databases. + * + * To add support for a new command type, one needs to define a new + * NonMainDbDistributeObjectOps object within OperationArray. Also, if + * the command requires marking or unmarking some objects as distributed, + * the necessary operations can be implemented in + * RunPreprocessNonMainDBCommand and RunPostprocessNonMainDBCommand. + * + *------------------------------------------------------------------------- + */ + +#include "postgres.h" + +#include "access/xact.h" +#include "catalog/pg_authid_d.h" +#include "nodes/nodes.h" +#include "nodes/parsenodes.h" +#include "utils/builtins.h" + +#include "distributed/commands.h" +#include "distributed/deparser.h" +#include "distributed/listutils.h" +#include "distributed/metadata_cache.h" +#include "distributed/remote_transaction.h" + + +#define EXECUTE_COMMAND_ON_REMOTE_NODES_AS_USER \ + "SELECT citus_internal.execute_command_on_remote_nodes_as_user(%s, %s)" +#define START_MANAGEMENT_TRANSACTION \ + "SELECT citus_internal.start_management_transaction('%lu')" +#define MARK_OBJECT_DISTRIBUTED \ + "SELECT citus_internal.mark_object_distributed(%d, %s, %d, %s)" +#define UNMARK_OBJECT_DISTRIBUTED \ + "SELECT pg_catalog.citus_unmark_object_distributed(%d, %d, %d, %s)" + + +/* + * NonMainDbDistributeObjectOps contains the necessary callbacks / flags to + * support node-wide object management commands from non-main databases. + * + * cannotBeExecutedInTransaction: + * Indicates whether the statement cannot be executed in a transaction. If + * this is set to true, the statement will be executed directly on the main + * database because there are no transactional visibility issues for such + * commands. + * + * checkSupportedObjectType: + * Callback function that checks whether type of the object referred to by + * given statement is supported. Can be NULL if not applicable for the + * statement type. + */ +typedef struct NonMainDbDistributeObjectOps +{ + bool cannotBeExecutedInTransaction; + bool (*checkSupportedObjectType)(Node *parsetree); +} NonMainDbDistributeObjectOps; + + +/* + * checkSupportedObjectType callbacks for OperationArray. + */ +static bool CreateDbStmtCheckSupportedObjectType(Node *node); +static bool DropDbStmtCheckSupportedObjectType(Node *node); +static bool GrantStmtCheckSupportedObjectType(Node *node); +static bool SecLabelStmtCheckSupportedObjectType(Node *node); + +/* + * OperationArray that holds NonMainDbDistributeObjectOps for different command types. + */ +static const NonMainDbDistributeObjectOps *const OperationArray[] = { + [T_CreateRoleStmt] = &(NonMainDbDistributeObjectOps) { + .cannotBeExecutedInTransaction = false, + .checkSupportedObjectType = NULL + }, + [T_DropRoleStmt] = &(NonMainDbDistributeObjectOps) { + .cannotBeExecutedInTransaction = false, + .checkSupportedObjectType = NULL + }, + [T_AlterRoleStmt] = &(NonMainDbDistributeObjectOps) { + .cannotBeExecutedInTransaction = false, + .checkSupportedObjectType = NULL + }, + [T_GrantRoleStmt] = &(NonMainDbDistributeObjectOps) { + .cannotBeExecutedInTransaction = false, + .checkSupportedObjectType = NULL + }, + [T_CreatedbStmt] = &(NonMainDbDistributeObjectOps) { + .cannotBeExecutedInTransaction = true, + .checkSupportedObjectType = CreateDbStmtCheckSupportedObjectType + }, + [T_DropdbStmt] = &(NonMainDbDistributeObjectOps) { + .cannotBeExecutedInTransaction = true, + .checkSupportedObjectType = DropDbStmtCheckSupportedObjectType + }, + [T_GrantStmt] = &(NonMainDbDistributeObjectOps) { + .cannotBeExecutedInTransaction = false, + .checkSupportedObjectType = GrantStmtCheckSupportedObjectType + }, + [T_SecLabelStmt] = &(NonMainDbDistributeObjectOps) { + .cannotBeExecutedInTransaction = false, + .checkSupportedObjectType = SecLabelStmtCheckSupportedObjectType + }, +}; + + +/* other static function declarations */ +const NonMainDbDistributeObjectOps * GetNonMainDbDistributeObjectOps(Node *parsetree); +static void CreateRoleStmtMarkDistGloballyOnMainDbs(CreateRoleStmt *createRoleStmt); +static void DropRoleStmtUnmarkDistOnLocalMainDb(DropRoleStmt *dropRoleStmt); +static void MarkObjectDistributedGloballyOnMainDbs(Oid catalogRelId, Oid objectId, + char *objectName); +static void UnmarkObjectDistributedOnLocalMainDb(uint16 catalogRelId, Oid objectId); + + +/* + * RunPreprocessNonMainDBCommand runs the necessary commands for a query, in main + * database before query is run on the local node with PrevProcessUtility. + * + * Returns true if previous utility hook needs to be skipped after completing + * preprocess phase. + */ +bool +RunPreprocessNonMainDBCommand(Node *parsetree) +{ + if (IsMainDB) + { + return false; + } + + const NonMainDbDistributeObjectOps *ops = GetNonMainDbDistributeObjectOps(parsetree); + if (!ops) + { + return false; + } + + char *queryString = DeparseTreeNode(parsetree); + + /* + * For the commands that cannot be executed in a transaction, there are no + * transactional visibility issues. We directly route them to main database + * so that we only have to consider one code-path for such commands. + */ + if (ops->cannotBeExecutedInTransaction) + { + IsMainDBCommandInXact = false; + RunCitusMainDBQuery((char *) queryString); + return true; + } + + IsMainDBCommandInXact = true; + + StringInfo mainDBQuery = makeStringInfo(); + appendStringInfo(mainDBQuery, + START_MANAGEMENT_TRANSACTION, + GetCurrentFullTransactionId().value); + RunCitusMainDBQuery(mainDBQuery->data); + + mainDBQuery = makeStringInfo(); + appendStringInfo(mainDBQuery, + EXECUTE_COMMAND_ON_REMOTE_NODES_AS_USER, + quote_literal_cstr(queryString), + quote_literal_cstr(CurrentUserName())); + RunCitusMainDBQuery(mainDBQuery->data); + + if (IsA(parsetree, DropRoleStmt)) + { + DropRoleStmtUnmarkDistOnLocalMainDb((DropRoleStmt *) parsetree); + } + + return false; +} + + +/* + * RunPostprocessNonMainDBCommand runs the necessary commands for a query, in main + * database after query is run on the local node with PrevProcessUtility. + */ +void +RunPostprocessNonMainDBCommand(Node *parsetree) +{ + if (IsMainDB || !GetNonMainDbDistributeObjectOps(parsetree)) + { + return; + } + + if (IsA(parsetree, CreateRoleStmt)) + { + CreateRoleStmtMarkDistGloballyOnMainDbs((CreateRoleStmt *) parsetree); + } +} + + +/* + * GetNonMainDbDistributeObjectOps returns the NonMainDbDistributeObjectOps for given + * command if it's node-wide object management command that's supported from non-main + * databases. + */ +const NonMainDbDistributeObjectOps * +GetNonMainDbDistributeObjectOps(Node *parsetree) +{ + NodeTag tag = nodeTag(parsetree); + if (tag >= lengthof(OperationArray)) + { + return NULL; + } + + const NonMainDbDistributeObjectOps *ops = OperationArray[tag]; + + if (ops == NULL) + { + return NULL; + } + + if (!ops->checkSupportedObjectType || + ops->checkSupportedObjectType(parsetree)) + { + return ops; + } + + return NULL; +} + + +/* + * CreateRoleStmtMarkDistGloballyOnMainDbs marks the role as + * distributed on all main databases globally. + */ +static void +CreateRoleStmtMarkDistGloballyOnMainDbs(CreateRoleStmt *createRoleStmt) +{ + /* object must exist as we've just created it */ + bool missingOk = false; + Oid roleId = get_role_oid(createRoleStmt->role, missingOk); + + MarkObjectDistributedGloballyOnMainDbs(AuthIdRelationId, roleId, + createRoleStmt->role); +} + + +/* + * DropRoleStmtUnmarkDistOnLocalMainDb unmarks the roles as + * distributed on the local main database. + */ +static void +DropRoleStmtUnmarkDistOnLocalMainDb(DropRoleStmt *dropRoleStmt) +{ + RoleSpec *roleSpec = NULL; + foreach_ptr(roleSpec, dropRoleStmt->roles) + { + Oid roleOid = get_role_oid(roleSpec->rolename, + dropRoleStmt->missing_ok); + if (roleOid == InvalidOid) + { + continue; + } + + UnmarkObjectDistributedOnLocalMainDb(AuthIdRelationId, roleOid); + } +} + + +/* + * MarkObjectDistributedGloballyOnMainDbs marks an object as + * distributed on all main databases globally. + */ +static void +MarkObjectDistributedGloballyOnMainDbs(Oid catalogRelId, Oid objectId, char *objectName) +{ + StringInfo mainDBQuery = makeStringInfo(); + appendStringInfo(mainDBQuery, + MARK_OBJECT_DISTRIBUTED, + catalogRelId, + quote_literal_cstr(objectName), + objectId, + quote_literal_cstr(CurrentUserName())); + RunCitusMainDBQuery(mainDBQuery->data); +} + + +/* + * UnmarkObjectDistributedOnLocalMainDb unmarks an object as + * distributed on the local main database. + */ +static void +UnmarkObjectDistributedOnLocalMainDb(uint16 catalogRelId, Oid objectId) +{ + const int subObjectId = 0; + const char *checkObjectExistence = "false"; + + StringInfo query = makeStringInfo(); + appendStringInfo(query, + UNMARK_OBJECT_DISTRIBUTED, + catalogRelId, objectId, + subObjectId, checkObjectExistence); + RunCitusMainDBQuery(query->data); +} + + +/* + * checkSupportedObjectTypes callbacks for OperationArray lie below. + */ +static bool +CreateDbStmtCheckSupportedObjectType(Node *node) +{ + /* + * We don't try to send the query to the main database if the CREATE + * DATABASE command is for the main database itself, this is a very + * rare case but it's exercised by our test suite. + */ + CreatedbStmt *stmt = castNode(CreatedbStmt, node); + return strcmp(stmt->dbname, MainDb) != 0; +} + + +static bool +DropDbStmtCheckSupportedObjectType(Node *node) +{ + /* + * We don't try to send the query to the main database if the DROP + * DATABASE command is for the main database itself, this is a very + * rare case but it's exercised by our test suite. + */ + DropdbStmt *stmt = castNode(DropdbStmt, node); + return strcmp(stmt->dbname, MainDb) != 0; +} + + +static bool +GrantStmtCheckSupportedObjectType(Node *node) +{ + GrantStmt *stmt = castNode(GrantStmt, node); + return stmt->objtype == OBJECT_DATABASE; +} + + +static bool +SecLabelStmtCheckSupportedObjectType(Node *node) +{ + SecLabelStmt *stmt = castNode(SecLabelStmt, node); + return stmt->objtype == OBJECT_ROLE; +} diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index e264713dd..9426e13c0 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -87,69 +87,6 @@ #include "distributed/worker_shard_visibility.h" #include "distributed/worker_transaction.h" -#define EXECUTE_COMMAND_ON_REMOTE_NODES_AS_USER \ - "SELECT citus_internal.execute_command_on_remote_nodes_as_user(%s, %s)" -#define START_MANAGEMENT_TRANSACTION \ - "SELECT citus_internal.start_management_transaction('%lu')" -#define MARK_OBJECT_DISTRIBUTED \ - "SELECT citus_internal.mark_object_distributed(%d, %s, %d, %s)" -#define UNMARK_OBJECT_DISTRIBUTED \ - "SELECT pg_catalog.citus_unmark_object_distributed(%d, %d, %d,%s)" - -/* see NonMainDbDistributedStatementInfo for the explanation of these flags */ -typedef enum DistObjectOperation -{ - NO_DIST_OBJECT_OPERATION, - MARK_DISTRIBUTED_GLOBALLY, - UNMARK_DISTRIBUTED_LOCALLY -} DistObjectOperation; - - -/* - * NonMainDbDistributedStatementInfo is used to determine whether a statement is - * supported from non-main databases and whether it should be marked or unmarked - * as distributed. - * - * When creating a distributed object, we always have to mark such objects as - * "distributed" but while for some object types we can delegate this to main - * database, for some others we have to explicitly send a command to all nodes - * in this code-path to achieve this. Callers need to provide - * MARK_DISTRIBUTED_GLOBALLY when that is the case. - * - * Similarly when dropping a distributed object, we always have to unmark such - * objects as "distributed" and our utility hook on remote nodes achieve this - * via UnmarkNodeWideObjectsDistributed() because the commands that we send to - * workers are executed via main db. However for the local node, this is not the - * case as we're not in the main db. For this reason, callers need to provide - * UNMARK_DISTRIBUTED_LOCALLY to unmark an object for local node as well. - */ -typedef struct NonMainDbDistributedStatementInfo -{ - int statementType; - DistObjectOperation DistObjectOperation; - - /* - * checkSupportedObjectTypes is a callback function that checks whether - * type of the object referred to by given statement is supported. - * - * Can be NULL if not applicable for the statement type. - */ - bool (*checkSupportedObjectTypes)(Node *node); -} NonMainDbDistributedStatementInfo; - -/* - * DistObjectOperationParams is used to pass parameters to the - * MarkObjectDistributedGloballyFromNonMainDb function and - * UnMarkObjectDistributedLocallyFromNonMainDb functions. - */ -typedef struct DistObjectOperationParams -{ - char *name; - Oid id; - uint16 catalogRelId; -} DistObjectOperationParams; - - bool EnableDDLPropagation = true; /* ddl propagation is enabled */ int CreateObjectPropagationMode = CREATE_OBJECT_PROPAGATION_IMMEDIATE; PropSetCmdBehavior PropagateSetCommands = PROPSETCMD_NONE; /* SET prop off */ @@ -179,46 +116,6 @@ static bool IsDropSchemaOrDB(Node *parsetree); static bool ShouldCheckUndistributeCitusLocalTables(void); -/* - * Functions to support commands used to manage node-wide objects from non-main - * databases. - */ -static bool IsCommandToCreateOrDropMainDB(Node *parsetree); -static void RunPreprocessMainDBCommand(Node *parsetree); -static void RunPostprocessMainDBCommand(Node *parsetree); -static bool IsStatementSupportedFromNonMainDb(Node *parsetree); -static bool StatementRequiresMarkDistributedGloballyFromNonMainDb(Node *parsetree); -static bool StatementRequiresUnmarkDistributedLocallyFromNonMainDb(Node *parsetree); -static void MarkObjectDistributedGloballyFromNonMainDb(Node *parsetree); -static void UnMarkObjectDistributedLocallyFromNonMainDb(List *unmarkDistributedList); -static List * GetDistObjectOperationParams(Node *parsetree); - -/* - * checkSupportedObjectTypes callbacks for - * NonMainDbDistributedStatementInfo objects. - */ -static bool NonMainDbCheckSupportedObjectTypeForGrant(Node *node); -static bool NonMainDbCheckSupportedObjectTypeForSecLabel(Node *node); - - -/* - * NonMainDbSupportedStatements is an array of statements that are supported - * from non-main databases. - */ -ObjectType supportedObjectTypesForGrantStmt[] = { OBJECT_DATABASE }; -static const NonMainDbDistributedStatementInfo NonMainDbSupportedStatements[] = { - { T_GrantRoleStmt, NO_DIST_OBJECT_OPERATION, NULL }, - { T_CreateRoleStmt, MARK_DISTRIBUTED_GLOBALLY, NULL }, - { T_DropRoleStmt, UNMARK_DISTRIBUTED_LOCALLY, NULL }, - { T_AlterRoleStmt, NO_DIST_OBJECT_OPERATION, NULL }, - { T_GrantStmt, NO_DIST_OBJECT_OPERATION, NonMainDbCheckSupportedObjectTypeForGrant }, - { T_CreatedbStmt, NO_DIST_OBJECT_OPERATION, NULL }, - { T_DropdbStmt, NO_DIST_OBJECT_OPERATION, NULL }, - { T_SecLabelStmt, NO_DIST_OBJECT_OPERATION, - NonMainDbCheckSupportedObjectTypeForSecLabel }, -}; - - /* * ProcessUtilityParseTree is a convenience method to create a PlannedStmt out of * pieces of a utility statement before invoking ProcessUtility. @@ -350,36 +247,25 @@ citus_ProcessUtility(PlannedStmt *pstmt, if (!CitusHasBeenLoaded()) { /* - * We always execute CREATE/DROP DATABASE from the main database. There are no - * transactional visibility issues, since these commands are non-transactional. - * And this way we only have to consider one codepath when creating databases. - * We don't try to send the query to the main database if the CREATE/DROP DATABASE - * command is for the main database itself, this is a very rare case but it's - * exercised by our test suite. + * Process the command via RunPreprocessNonMainDBCommand and + * RunPostprocessNonMainDBCommand hooks if we're in a non-main database + * and if the command is a node-wide object management command that we + * support from non-main databases. */ - if (!IsMainDB && - !IsCommandToCreateOrDropMainDB(parsetree)) - { - RunPreprocessMainDBCommand(parsetree); - if (IsA(parsetree, CreatedbStmt) || - IsA(parsetree, DropdbStmt)) - { - return; - } + bool shouldSkipPrevUtilityHook = RunPreprocessNonMainDBCommand(parsetree); + + if (!shouldSkipPrevUtilityHook) + { + /* + * Ensure that utility commands do not behave any differently until CREATE + * EXTENSION is invoked. + */ + PrevProcessUtility(pstmt, queryString, false, context, + params, queryEnv, dest, completionTag); } - /* - * Ensure that utility commands do not behave any differently until CREATE - * EXTENSION is invoked. - */ - PrevProcessUtility(pstmt, queryString, false, context, - params, queryEnv, dest, completionTag); - - if (!IsMainDB) - { - RunPostprocessMainDBCommand(parsetree); - } + RunPostprocessNonMainDBCommand(parsetree); return; } @@ -1715,281 +1601,3 @@ DropSchemaOrDBInProgress(void) { return activeDropSchemaOrDBs > 0; } - - -/* - * IsCommandToCreateOrDropMainDB checks if this query creates or drops the - * main database, so we can make an exception and not send this query to - * the main database. - */ -static bool -IsCommandToCreateOrDropMainDB(Node *parsetree) -{ - if (IsA(parsetree, CreatedbStmt)) - { - CreatedbStmt *createdbStmt = castNode(CreatedbStmt, parsetree); - return strcmp(createdbStmt->dbname, MainDb) == 0; - } - else if (IsA(parsetree, DropdbStmt)) - { - DropdbStmt *dropdbStmt = castNode(DropdbStmt, parsetree); - return strcmp(dropdbStmt->dbname, MainDb) == 0; - } - - return false; -} - - -/* - * RunPreprocessMainDBCommand runs the necessary commands for a query, in main - * database before query is run on the local node with PrevProcessUtility - */ -static void -RunPreprocessMainDBCommand(Node *parsetree) -{ - if (!IsStatementSupportedFromNonMainDb(parsetree)) - { - return; - } - - char *queryString = DeparseTreeNode(parsetree); - - if (IsA(parsetree, CreatedbStmt) || - IsA(parsetree, DropdbStmt)) - { - IsMainDBCommandInXact = false; - RunCitusMainDBQuery((char *) queryString); - return; - } - - IsMainDBCommandInXact = true; - - StringInfo mainDBQuery = makeStringInfo(); - appendStringInfo(mainDBQuery, - START_MANAGEMENT_TRANSACTION, - GetCurrentFullTransactionId().value); - RunCitusMainDBQuery(mainDBQuery->data); - - mainDBQuery = makeStringInfo(); - appendStringInfo(mainDBQuery, - EXECUTE_COMMAND_ON_REMOTE_NODES_AS_USER, - quote_literal_cstr(queryString), - quote_literal_cstr(CurrentUserName())); - RunCitusMainDBQuery(mainDBQuery->data); - - if (StatementRequiresUnmarkDistributedLocallyFromNonMainDb(parsetree)) - { - List *unmarkParams = GetDistObjectOperationParams(parsetree); - UnMarkObjectDistributedLocallyFromNonMainDb(unmarkParams); - } -} - - -/* - * RunPostprocessMainDBCommand runs the necessary commands for a query, in main - * database after query is run on the local node with PrevProcessUtility - */ -static void -RunPostprocessMainDBCommand(Node *parsetree) -{ - if (IsStatementSupportedFromNonMainDb(parsetree) && - StatementRequiresMarkDistributedGloballyFromNonMainDb(parsetree)) - { - MarkObjectDistributedGloballyFromNonMainDb(parsetree); - } -} - - -/* - * IsStatementSupportedFromNonMainDb returns true if the statement is supported from a - * non-main database. - */ -static bool -IsStatementSupportedFromNonMainDb(Node *parsetree) -{ - NodeTag type = nodeTag(parsetree); - - for (int i = 0; i < sizeof(NonMainDbSupportedStatements) / - sizeof(NonMainDbSupportedStatements[0]); i++) - { - if (type != NonMainDbSupportedStatements[i].statementType) - { - continue; - } - - return !NonMainDbSupportedStatements[i].checkSupportedObjectTypes || - NonMainDbSupportedStatements[i].checkSupportedObjectTypes(parsetree); - } - - return false; -} - - -/* - * StatementRequiresMarkDistributedGloballyFromNonMainDb returns true if the statement should be marked - * as distributed when executed from a non-main database. - */ -static bool -StatementRequiresMarkDistributedGloballyFromNonMainDb(Node *parsetree) -{ - NodeTag type = nodeTag(parsetree); - - for (int i = 0; i < sizeof(NonMainDbSupportedStatements) / - sizeof(NonMainDbSupportedStatements[0]); i++) - { - if (type == NonMainDbSupportedStatements[i].statementType) - { - return NonMainDbSupportedStatements[i].DistObjectOperation == - MARK_DISTRIBUTED_GLOBALLY; - } - } - - return false; -} - - -/* - * StatementRequiresUnmarkDistributedLocallyFromNonMainDb returns true if the statement should be unmarked - * as distributed when executed from a non-main database. - */ -static bool -StatementRequiresUnmarkDistributedLocallyFromNonMainDb(Node *parsetree) -{ - NodeTag type = nodeTag(parsetree); - - for (int i = 0; i < sizeof(NonMainDbSupportedStatements) / - sizeof(NonMainDbSupportedStatements[0]); i++) - { - if (type == NonMainDbSupportedStatements[i].statementType) - { - return NonMainDbSupportedStatements[i].DistObjectOperation == - UNMARK_DISTRIBUTED_LOCALLY; - } - } - - return false; -} - - -/* - * MarkObjectDistributedGloballyFromNonMainDb marks the given object as distributed on the - * non-main database. - */ -static void -MarkObjectDistributedGloballyFromNonMainDb(Node *parsetree) -{ - List *distObjectOperationParams = - GetDistObjectOperationParams(parsetree); - - DistObjectOperationParams *distObjectOperationParam = NULL; - - foreach_ptr(distObjectOperationParam, distObjectOperationParams) - { - StringInfo mainDBQuery = makeStringInfo(); - appendStringInfo(mainDBQuery, - MARK_OBJECT_DISTRIBUTED, - distObjectOperationParam->catalogRelId, - quote_literal_cstr(distObjectOperationParam->name), - distObjectOperationParam->id, - quote_literal_cstr(CurrentUserName())); - RunCitusMainDBQuery(mainDBQuery->data); - } -} - - -/* - * UnMarkObjectDistributedLocallyFromNonMainDb unmarks the given object as distributed on the - * non-main database. - */ -static void -UnMarkObjectDistributedLocallyFromNonMainDb(List *markObjectDistributedParamList) -{ - DistObjectOperationParams *markObjectDistributedParam = NULL; - int subObjectId = 0; - char *checkObjectExistence = "false"; - foreach_ptr(markObjectDistributedParam, markObjectDistributedParamList) - { - StringInfo query = makeStringInfo(); - appendStringInfo(query, - UNMARK_OBJECT_DISTRIBUTED, - AuthIdRelationId, - markObjectDistributedParam->id, - subObjectId, checkObjectExistence); - RunCitusMainDBQuery(query->data); - } -} - - -/* - * GetDistObjectOperationParams returns DistObjectOperationParams for the target - * object of given parsetree. - */ -List * -GetDistObjectOperationParams(Node *parsetree) -{ - List *paramsList = NIL; - if (IsA(parsetree, CreateRoleStmt)) - { - CreateRoleStmt *stmt = castNode(CreateRoleStmt, parsetree); - DistObjectOperationParams *params = - (DistObjectOperationParams *) palloc(sizeof(DistObjectOperationParams)); - params->name = stmt->role; - params->catalogRelId = AuthIdRelationId; - params->id = get_role_oid(stmt->role, false); - - paramsList = lappend(paramsList, params); - } - else if (IsA(parsetree, DropRoleStmt)) - { - DropRoleStmt *stmt = castNode(DropRoleStmt, parsetree); - RoleSpec *roleSpec; - foreach_ptr(roleSpec, stmt->roles) - { - DistObjectOperationParams *params = (DistObjectOperationParams *) palloc( - sizeof(DistObjectOperationParams)); - - Oid roleOid = get_role_oid(roleSpec->rolename, true); - - if (roleOid == InvalidOid) - { - continue; - } - - params->id = roleOid; - params->name = roleSpec->rolename; - params->catalogRelId = AuthIdRelationId; - - paramsList = lappend(paramsList, params); - } - } - else - { - elog(ERROR, "unsupported statement type"); - } - - return paramsList; -} - - -/* - * NonMainDbCheckSupportedObjectTypeForGrant implements checkSupportedObjectTypes - * callback for GrantStmt. - */ -static bool -NonMainDbCheckSupportedObjectTypeForGrant(Node *node) -{ - GrantStmt *stmt = castNode(GrantStmt, node); - return stmt->objtype == OBJECT_DATABASE; -} - - -/* - * NonMainDbCheckSupportedObjectTypeForSecLabel implements checkSupportedObjectTypes - * callback for SecLabel. - */ -static bool -NonMainDbCheckSupportedObjectTypeForSecLabel(Node *node) -{ - SecLabelStmt *stmt = castNode(SecLabelStmt, node); - return stmt->objtype == OBJECT_ROLE; -} diff --git a/src/include/distributed/commands.h b/src/include/distributed/commands.h index de15553e7..084308a8f 100644 --- a/src/include/distributed/commands.h +++ b/src/include/distributed/commands.h @@ -104,6 +104,10 @@ typedef struct DistributeObjectOps const DistributeObjectOps * GetDistributeObjectOps(Node *node); +/* functions to support node-wide object management commands from non-main dbs */ +extern bool RunPreprocessNonMainDBCommand(Node *parsetree); +extern void RunPostprocessNonMainDBCommand(Node *parsetree); + /* * Flags that can be passed to GetForeignKeyOids to indicate * which foreign key constraint OIDs are to be extracted From 0acb5f6e8616fb5a7dc3e91a5c6ef91cf85beb3f Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Wed, 20 Mar 2024 01:10:12 +0100 Subject: [PATCH 3/5] Fix assertion failure in maintenance daemon during Citus upgrades (#7537) Fixes https://github.com/citusdata/citus/issues/7536. Note to reviewer: Before this commit, the following results in an assertion failure when executed locally and this won't be the case anymore: ```console make -C src/test/regress/ check-citus-upgrade-local citus-old-version=v10.2.0 ``` Note that this doesn't happen on CI as we don't enable assertions there. --------- Co-authored-by: Jelte Fennema-Nio --- .../transaction/transaction_recovery.c | 26 ++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/backend/distributed/transaction/transaction_recovery.c b/src/backend/distributed/transaction/transaction_recovery.c index 653b962db..c31dc85a2 100644 --- a/src/backend/distributed/transaction/transaction_recovery.c +++ b/src/backend/distributed/transaction/transaction_recovery.c @@ -34,6 +34,7 @@ #include "utils/fmgroids.h" #include "utils/memutils.h" #include "utils/rel.h" +#include "utils/syscache.h" #include "utils/xid8.h" #include "pg_version_constants.h" @@ -261,11 +262,28 @@ RecoverWorkerTransactions(WorkerNode *workerNode) continue; } - /* Check if the transaction is created by an outer transaction from a non-main database */ bool outerXidIsNull = false; - Datum outerXidDatum = heap_getattr(heapTuple, - Anum_pg_dist_transaction_outerxid, - tupleDescriptor, &outerXidIsNull); + Datum outerXidDatum = 0; + if (EnableVersionChecks || + SearchSysCacheExistsAttName(DistTransactionRelationId(), "outer_xid")) + { + /* Check if the transaction is created by an outer transaction from a non-main database */ + outerXidDatum = heap_getattr(heapTuple, + Anum_pg_dist_transaction_outerxid, + tupleDescriptor, &outerXidIsNull); + } + else + { + /* + * Normally we don't try to recover prepared transactions when the + * binary version doesn't match the sql version. However, we skip + * those checks in regression tests by disabling + * citus.enable_version_checks. And when this is the case, while + * the C code looks for "outer_xid" attribute, pg_dist_transaction + * doesn't yet have it. + */ + Assert(!EnableVersionChecks); + } TransactionId outerXid = 0; if (!outerXidIsNull) From fdd658acecced724f275429094d4d381c1b9fe4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emel=20=C5=9Eim=C5=9Fek?= Date: Wed, 20 Mar 2024 11:06:05 +0300 Subject: [PATCH 4/5] Fix crash caused by some form of ALTER TABLE ADD COLUMN statements. (#7522) DESCRIPTION: Fixes a crash caused by some form of ALTER TABLE ADD COLUMN statements. When adding multiple columns, if one of the ADD COLUMN statements contains a FOREIGN constraint ommitting the referenced columns in the statement, a SEGFAULT occurs. For instance, the following statement results in a crash: ``` ALTER TABLE lt ADD COLUMN new_col1 bool, ADD COLUMN new_col2 int references rt; ``` Fixes #7520. --- src/backend/distributed/commands/table.c | 10 +++++++--- src/backend/distributed/deparser/deparse_table_stmts.c | 2 +- src/include/distributed/deparser.h | 2 ++ src/test/regress/expected/alter_table_add_column.out | 9 +++++++++ src/test/regress/sql/alter_table_add_column.sql | 4 ++++ 5 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/backend/distributed/commands/table.c b/src/backend/distributed/commands/table.c index 074a789ed..30b028b79 100644 --- a/src/backend/distributed/commands/table.c +++ b/src/backend/distributed/commands/table.c @@ -3053,11 +3053,15 @@ ErrorUnsupportedAlterTableAddColumn(Oid relationId, AlterTableCmd *command, else if (constraint->contype == CONSTR_FOREIGN) { RangeVar *referencedTable = constraint->pktable; - char *referencedColumn = strVal(lfirst(list_head(constraint->pk_attrs))); Oid referencedRelationId = RangeVarGetRelid(referencedTable, NoLock, false); - appendStringInfo(errHint, "FOREIGN KEY (%s) REFERENCES %s(%s)", colName, - get_rel_name(referencedRelationId), referencedColumn); + appendStringInfo(errHint, "FOREIGN KEY (%s) REFERENCES %s", colName, + get_rel_name(referencedRelationId)); + + if (list_length(constraint->pk_attrs) > 0) + { + AppendColumnNameList(errHint, constraint->pk_attrs); + } if (constraint->fk_del_action == FKCONSTR_ACTION_SETNULL) { diff --git a/src/backend/distributed/deparser/deparse_table_stmts.c b/src/backend/distributed/deparser/deparse_table_stmts.c index e976b0e2f..5d184fa66 100644 --- a/src/backend/distributed/deparser/deparse_table_stmts.c +++ b/src/backend/distributed/deparser/deparse_table_stmts.c @@ -121,7 +121,7 @@ AppendAlterTableStmt(StringInfo buf, AlterTableStmt *stmt) * AppendColumnNameList converts a list of columns into comma separated string format * (colname_1, colname_2, .., colname_n). */ -static void +void AppendColumnNameList(StringInfo buf, List *columns) { appendStringInfoString(buf, " ("); diff --git a/src/include/distributed/deparser.h b/src/include/distributed/deparser.h index 437a9fd8e..4d4005c19 100644 --- a/src/include/distributed/deparser.h +++ b/src/include/distributed/deparser.h @@ -121,6 +121,8 @@ extern void AppendGrantedByInGrant(StringInfo buf, GrantStmt *stmt); extern void AppendGrantSharedPrefix(StringInfo buf, GrantStmt *stmt); extern void AppendGrantSharedSuffix(StringInfo buf, GrantStmt *stmt); +extern void AppendColumnNameList(StringInfo buf, List *columns); + /* Common deparser utils */ typedef struct DefElemOptionFormat diff --git a/src/test/regress/expected/alter_table_add_column.out b/src/test/regress/expected/alter_table_add_column.out index 61e7319d9..0408aeeab 100644 --- a/src/test/regress/expected/alter_table_add_column.out +++ b/src/test/regress/expected/alter_table_add_column.out @@ -44,6 +44,15 @@ ERROR: cannot execute ADD COLUMN command with PRIMARY KEY, UNIQUE, FOREIGN and DETAIL: Adding a column with a constraint in one command is not supported because all constraints in Citus must have explicit names HINT: You can issue each command separately such as ALTER TABLE referencing ADD COLUMN test_8 data_type; ALTER TABLE referencing ADD CONSTRAINT constraint_name CHECK (check_expression); ALTER TABLE referencing ADD COLUMN test_8 integer CONSTRAINT check_test_8 CHECK (test_8 > 0); +-- error out properly even if the REFERENCES does not include the column list of the referenced table +ALTER TABLE referencing ADD COLUMN test_9 bool, ADD COLUMN test_10 int REFERENCES referenced; +ERROR: cannot execute ADD COLUMN command with PRIMARY KEY, UNIQUE, FOREIGN and CHECK constraints +DETAIL: Adding a column with a constraint in one command is not supported because all constraints in Citus must have explicit names +HINT: You can issue each command separately such as ALTER TABLE referencing ADD COLUMN test_10 data_type; ALTER TABLE referencing ADD CONSTRAINT constraint_name FOREIGN KEY (test_10) REFERENCES referenced; +ALTER TABLE referencing ADD COLUMN test_9 bool, ADD COLUMN test_10 int REFERENCES referenced(int_col); +ERROR: cannot execute ADD COLUMN command with PRIMARY KEY, UNIQUE, FOREIGN and CHECK constraints +DETAIL: Adding a column with a constraint in one command is not supported because all constraints in Citus must have explicit names +HINT: You can issue each command separately such as ALTER TABLE referencing ADD COLUMN test_10 data_type; ALTER TABLE referencing ADD CONSTRAINT constraint_name FOREIGN KEY (test_10) REFERENCES referenced (int_col ); -- try to add test_6 again, but with IF NOT EXISTS ALTER TABLE referencing ADD COLUMN IF NOT EXISTS test_6 text; NOTICE: column "test_6" of relation "referencing" already exists, skipping diff --git a/src/test/regress/sql/alter_table_add_column.sql b/src/test/regress/sql/alter_table_add_column.sql index 255e7714f..355667842 100644 --- a/src/test/regress/sql/alter_table_add_column.sql +++ b/src/test/regress/sql/alter_table_add_column.sql @@ -41,6 +41,10 @@ ALTER TABLE referencing ADD COLUMN "test_\'!7" "simple_!\'custom_type"; ALTER TABLE referencing ADD COLUMN test_8 integer CHECK (test_8 > 0); ALTER TABLE referencing ADD COLUMN test_8 integer CONSTRAINT check_test_8 CHECK (test_8 > 0); +-- error out properly even if the REFERENCES does not include the column list of the referenced table +ALTER TABLE referencing ADD COLUMN test_9 bool, ADD COLUMN test_10 int REFERENCES referenced; +ALTER TABLE referencing ADD COLUMN test_9 bool, ADD COLUMN test_10 int REFERENCES referenced(int_col); + -- try to add test_6 again, but with IF NOT EXISTS ALTER TABLE referencing ADD COLUMN IF NOT EXISTS test_6 text; ALTER TABLE referencing ADD COLUMN IF NOT EXISTS test_6 integer; From 3929a5b2a656d754327e78f2845b962b72f91a3e Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Wed, 20 Mar 2024 14:38:33 +0300 Subject: [PATCH 5/5] Fix incorrect "VALID UNTIL" assumption made for roles in node activation (#7534) Fixes https://github.com/citusdata/citus/issues/7533. DESCRIPTION: Fixes incorrect `VALID UNTIL` setting assumption made for roles when syncing them to new nodes --- src/backend/distributed/commands/role.c | 13 ++++++------- .../expected/create_role_propagation.out | 18 +++++++++--------- .../expected/metadata_sync_from_non_maindb.out | 15 ++++++--------- src/test/regress/expected/seclabel.out | 4 ++-- .../regress/expected/upgrade_post_11_after.out | 14 ++++++++++++++ .../sql/metadata_sync_from_non_maindb.sql | 3 --- src/test/regress/sql/upgrade_post_11_after.sql | 15 +++++++++++++++ 7 files changed, 52 insertions(+), 30 deletions(-) diff --git a/src/backend/distributed/commands/role.c b/src/backend/distributed/commands/role.c index f2b567e6e..7f5f697f2 100644 --- a/src/backend/distributed/commands/role.c +++ b/src/backend/distributed/commands/role.c @@ -491,18 +491,17 @@ GenerateRoleOptionsList(HeapTuple tuple) options = lappend(options, makeDefElem("password", NULL, -1)); } - /* load valid unitl data from the heap tuple, use default of infinity if not set */ + /* load valid until data from the heap tuple */ Datum rolValidUntilDatum = SysCacheGetAttr(AUTHNAME, tuple, Anum_pg_authid_rolvaliduntil, &isNull); - char *rolValidUntil = "infinity"; if (!isNull) { - rolValidUntil = pstrdup((char *) timestamptz_to_str(rolValidUntilDatum)); - } + char *rolValidUntil = pstrdup((char *) timestamptz_to_str(rolValidUntilDatum)); - Node *validUntilStringNode = (Node *) makeString(rolValidUntil); - DefElem *validUntilOption = makeDefElem("validUntil", validUntilStringNode, -1); - options = lappend(options, validUntilOption); + Node *validUntilStringNode = (Node *) makeString(rolValidUntil); + DefElem *validUntilOption = makeDefElem("validUntil", validUntilStringNode, -1); + options = lappend(options, validUntilOption); + } return options; } diff --git a/src/test/regress/expected/create_role_propagation.out b/src/test/regress/expected/create_role_propagation.out index 90f2690ce..4d594ddab 100644 --- a/src/test/regress/expected/create_role_propagation.out +++ b/src/test/regress/expected/create_role_propagation.out @@ -121,17 +121,17 @@ SELECT 1 FROM master_add_node('localhost', :worker_2_port); SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, (rolpassword != '') as pass_not_empty, rolvaliduntil FROM pg_authid WHERE rolname LIKE 'create\_%' ORDER BY rolname; rolname | rolsuper | rolinherit | rolcreaterole | rolcreatedb | rolcanlogin | rolreplication | rolbypassrls | rolconnlimit | pass_not_empty | rolvaliduntil --------------------------------------------------------------------- - create_group | f | t | f | f | f | f | f | -1 | | infinity - create_group_2 | f | t | f | f | f | f | f | -1 | | infinity - create_role | f | t | f | f | f | f | f | -1 | | infinity - create_role"edge | f | t | f | f | f | f | f | -1 | | infinity - create_role'edge | f | t | f | f | f | f | f | -1 | | infinity - create_role_2 | f | t | f | f | f | f | f | -1 | | infinity - create_role_sysid | f | t | f | f | f | f | f | -1 | | infinity + create_group | f | t | f | f | f | f | f | -1 | | + create_group_2 | f | t | f | f | f | f | f | -1 | | + create_role | f | t | f | f | f | f | f | -1 | | + create_role"edge | f | t | f | f | f | f | f | -1 | | + create_role'edge | f | t | f | f | f | f | f | -1 | | + create_role_2 | f | t | f | f | f | f | f | -1 | | + create_role_sysid | f | t | f | f | f | f | f | -1 | | create_role_with_everything | t | t | t | t | t | t | t | 105 | t | Thu May 04 17:00:00 2045 PDT create_role_with_nothing | f | f | f | f | f | f | f | 3 | t | Mon May 04 17:00:00 2015 PDT - create_user | f | t | f | f | t | f | f | -1 | | infinity - create_user_2 | f | t | f | f | t | f | f | -1 | | infinity + create_user | f | t | f | f | t | f | f | -1 | | + create_user_2 | f | t | f | f | t | f | f | -1 | | (11 rows) SELECT roleid::regrole::text AS role, member::regrole::text, grantor::regrole::text, admin_option FROM pg_auth_members WHERE roleid::regrole::text LIKE 'create\_%' ORDER BY 1, 2; diff --git a/src/test/regress/expected/metadata_sync_from_non_maindb.out b/src/test/regress/expected/metadata_sync_from_non_maindb.out index 6630b39bd..2aac507bd 100644 --- a/src/test/regress/expected/metadata_sync_from_non_maindb.out +++ b/src/test/regress/expected/metadata_sync_from_non_maindb.out @@ -188,7 +188,6 @@ select 1 from citus_add_node('localhost', :worker_2_port); 1 (1 row) --- XXX: date is not correct on one of the workers due to https://github.com/citusdata/citus/issues/7533 select result FROM run_command_on_all_nodes($$ SELECT array_to_json(array_agg(row_to_json(t))) FROM ( @@ -200,11 +199,11 @@ select result FROM run_command_on_all_nodes($$ ORDER BY rolname ) t $$); - result + result --------------------------------------------------------------------- [{"rolname":"test_role1","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":true,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":true,"date":null},{"rolname":"test_role2-needs\\!escape","rolsuper":true,"rolinherit":true,"rolcreaterole":true,"rolcreatedb":true,"rolcanlogin":true,"rolreplication":true,"rolbypassrls":true,"rolconnlimit":10,"pass_not_empty":null,"date":"2023-01-01"},{"rolname":"test_role3","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":null,"date":null}] [{"rolname":"test_role1","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":true,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":true,"date":null},{"rolname":"test_role2-needs\\!escape","rolsuper":true,"rolinherit":true,"rolcreaterole":true,"rolcreatedb":true,"rolcanlogin":true,"rolreplication":true,"rolbypassrls":true,"rolconnlimit":10,"pass_not_empty":null,"date":"2023-01-01"},{"rolname":"test_role3","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":null,"date":null}] - [{"rolname":"test_role1","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":true,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":true,"date":"infinity"},{"rolname":"test_role2-needs\\!escape","rolsuper":true,"rolinherit":true,"rolcreaterole":true,"rolcreatedb":true,"rolcanlogin":true,"rolreplication":true,"rolbypassrls":true,"rolconnlimit":10,"pass_not_empty":null,"date":"2023-01-01"},{"rolname":"test_role3","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":null,"date":"infinity"}] + [{"rolname":"test_role1","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":true,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":true,"date":null},{"rolname":"test_role2-needs\\!escape","rolsuper":true,"rolinherit":true,"rolcreaterole":true,"rolcreatedb":true,"rolcanlogin":true,"rolreplication":true,"rolbypassrls":true,"rolconnlimit":10,"pass_not_empty":null,"date":"2023-01-01"},{"rolname":"test_role3","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":null,"date":null}] (3 rows) --test for alter user @@ -229,7 +228,6 @@ select 1 from citus_add_node('localhost', :worker_2_port); 1 (1 row) --- XXX: date is not correct on one of the workers due to https://github.com/citusdata/citus/issues/7533 select result FROM run_command_on_all_nodes($$ SELECT array_to_json(array_agg(row_to_json(t))) FROM ( @@ -241,11 +239,11 @@ select result FROM run_command_on_all_nodes($$ ORDER BY rolname ) t $$); - result + result --------------------------------------------------------------------- [{"rolname":"test_role1","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":true,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":true,"date":null},{"rolname":"test_role2-needs\\!escape","rolsuper":false,"rolinherit":false,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":5,"pass_not_empty":null,"date":"2024-01-01"},{"rolname":"test_role3","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":null,"date":null}] [{"rolname":"test_role1","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":true,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":true,"date":null},{"rolname":"test_role2-needs\\!escape","rolsuper":false,"rolinherit":false,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":5,"pass_not_empty":null,"date":"2024-01-01"},{"rolname":"test_role3","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":null,"date":null}] - [{"rolname":"test_role1","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":true,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":true,"date":"infinity"},{"rolname":"test_role2-needs\\!escape","rolsuper":false,"rolinherit":false,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":5,"pass_not_empty":null,"date":"2024-01-01"},{"rolname":"test_role3","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":null,"date":"infinity"}] + [{"rolname":"test_role1","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":true,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":true,"date":null},{"rolname":"test_role2-needs\\!escape","rolsuper":false,"rolinherit":false,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":5,"pass_not_empty":null,"date":"2024-01-01"},{"rolname":"test_role3","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":null,"date":null}] (3 rows) --test for drop user @@ -266,7 +264,6 @@ select 1 from citus_add_node('localhost', :worker_2_port); 1 (1 row) --- XXX: date is not correct on one of the workers due to https://github.com/citusdata/citus/issues/7533 select result FROM run_command_on_all_nodes($$ SELECT array_to_json(array_agg(row_to_json(t))) FROM ( @@ -278,11 +275,11 @@ select result FROM run_command_on_all_nodes($$ ORDER BY rolname ) t $$); - result + result --------------------------------------------------------------------- - [{"rolname":"test_role1","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":true,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":true,"date":"infinity"},{"rolname":"test_role2-needs\\!escape","rolsuper":false,"rolinherit":false,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":5,"pass_not_empty":null,"date":"2024-01-01"},{"rolname":"test_role3","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":null,"date":"infinity"}] + [{"rolname":"test_role1","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":true,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":true,"date":null},{"rolname":"test_role2-needs\\!escape","rolsuper":false,"rolinherit":false,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":5,"pass_not_empty":null,"date":"2024-01-01"},{"rolname":"test_role3","rolsuper":false,"rolinherit":true,"rolcreaterole":false,"rolcreatedb":false,"rolcanlogin":false,"rolreplication":false,"rolbypassrls":false,"rolconnlimit":-1,"pass_not_empty":null,"date":null}] (3 rows) -- Clean up: drop the database on worker node 2 diff --git a/src/test/regress/expected/seclabel.out b/src/test/regress/expected/seclabel.out index ae6589734..ca6c6f984 100644 --- a/src/test/regress/expected/seclabel.out +++ b/src/test/regress/expected/seclabel.out @@ -167,9 +167,9 @@ SELECT node_type, result FROM get_citus_tests_label_provider_labels('"user 2"') SET citus.log_remote_commands TO on; SET citus.grep_remote_commands = '%SECURITY LABEL%'; SELECT 1 FROM citus_add_node('localhost', :worker_2_port); -NOTICE: issuing SELECT worker_create_or_alter_role('user1', 'CREATE ROLE user1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''', 'ALTER ROLE user1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''');SECURITY LABEL FOR "citus '!tests_label_provider" ON ROLE user1 IS 'citus_classified' +NOTICE: issuing SELECT worker_create_or_alter_role('user1', 'CREATE ROLE user1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL', 'ALTER ROLE user1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL');SECURITY LABEL FOR "citus '!tests_label_provider" ON ROLE user1 IS 'citus_classified' DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx -NOTICE: issuing SELECT worker_create_or_alter_role('user 2', 'CREATE ROLE "user 2" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''', 'ALTER ROLE "user 2" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''');SECURITY LABEL FOR "citus '!tests_label_provider" ON ROLE "user 2" IS 'citus ''!unclassified' +NOTICE: issuing SELECT worker_create_or_alter_role('user 2', 'CREATE ROLE "user 2" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL', 'ALTER ROLE "user 2" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL');SECURITY LABEL FOR "citus '!tests_label_provider" ON ROLE "user 2" IS 'citus ''!unclassified' DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx ?column? --------------------------------------------------------------------- diff --git a/src/test/regress/expected/upgrade_post_11_after.out b/src/test/regress/expected/upgrade_post_11_after.out index 422bc846f..49bd20432 100644 --- a/src/test/regress/expected/upgrade_post_11_after.out +++ b/src/test/regress/expected/upgrade_post_11_after.out @@ -67,6 +67,20 @@ SELECT 1 FROM run_command_on_workers($$SELECT pg_reload_conf()$$); 1 (2 rows) +-- In the version that we use for upgrade tests (v10.2.0), we propagate +-- "valid until" to the workers as "infinity" even if it's not set. And +-- given that "postgres" role is created in the older version, "valid until" +-- is set to "infinity" on the workers while this is not the case for +-- coordinator. See https://github.com/citusdata/citus/issues/7533. +-- +-- We're fixing this for new versions of Citus and we'll probably backport +-- this to some older versions too. However, v10.2.0 won't ever have this +-- fix. +-- +-- For this reason, here we set "valid until" to "infinity" for all the +-- nodes so that below query doesn't report any difference between the +-- metadata on coordinator and workers. +ALTER ROLE postgres WITH VALID UNTIL 'infinity'; -- make sure that the metadata is consistent across all nodes -- we exclude the distributed_object_data as they are -- not sorted in the same order (as OIDs differ on the nodes) diff --git a/src/test/regress/sql/metadata_sync_from_non_maindb.sql b/src/test/regress/sql/metadata_sync_from_non_maindb.sql index 67e1e98d1..62760c6cc 100644 --- a/src/test/regress/sql/metadata_sync_from_non_maindb.sql +++ b/src/test/regress/sql/metadata_sync_from_non_maindb.sql @@ -100,7 +100,6 @@ create role test_role3; \c regression - - :master_port select 1 from citus_add_node('localhost', :worker_2_port); --- XXX: date is not correct on one of the workers due to https://github.com/citusdata/citus/issues/7533 select result FROM run_command_on_all_nodes($$ SELECT array_to_json(array_agg(row_to_json(t))) FROM ( @@ -128,7 +127,6 @@ LIMIT 5 VALID UNTIL '2024-01-01'; \c regression - - :master_port select 1 from citus_add_node('localhost', :worker_2_port); --- XXX: date is not correct on one of the workers due to https://github.com/citusdata/citus/issues/7533 select result FROM run_command_on_all_nodes($$ SELECT array_to_json(array_agg(row_to_json(t))) FROM ( @@ -153,7 +151,6 @@ DROP ROLE test_role3; \c regression - - :master_port select 1 from citus_add_node('localhost', :worker_2_port); --- XXX: date is not correct on one of the workers due to https://github.com/citusdata/citus/issues/7533 select result FROM run_command_on_all_nodes($$ SELECT array_to_json(array_agg(row_to_json(t))) FROM ( diff --git a/src/test/regress/sql/upgrade_post_11_after.sql b/src/test/regress/sql/upgrade_post_11_after.sql index ba9b12f3b..6d948ec34 100644 --- a/src/test/regress/sql/upgrade_post_11_after.sql +++ b/src/test/regress/sql/upgrade_post_11_after.sql @@ -27,6 +27,21 @@ SET datestyle = "ISO, YMD"; SELECT 1 FROM run_command_on_workers($$ALTER SYSTEM SET datestyle = "ISO, YMD";$$); SELECT 1 FROM run_command_on_workers($$SELECT pg_reload_conf()$$); +-- In the version that we use for upgrade tests (v10.2.0), we propagate +-- "valid until" to the workers as "infinity" even if it's not set. And +-- given that "postgres" role is created in the older version, "valid until" +-- is set to "infinity" on the workers while this is not the case for +-- coordinator. See https://github.com/citusdata/citus/issues/7533. +-- +-- We're fixing this for new versions of Citus and we'll probably backport +-- this to some older versions too. However, v10.2.0 won't ever have this +-- fix. +-- +-- For this reason, here we set "valid until" to "infinity" for all the +-- nodes so that below query doesn't report any difference between the +-- metadata on coordinator and workers. +ALTER ROLE postgres WITH VALID UNTIL 'infinity'; + -- make sure that the metadata is consistent across all nodes -- we exclude the distributed_object_data as they are -- not sorted in the same order (as OIDs differ on the nodes)