From 51009d01913b9ddc1df426bd4df5bb22ecfe15fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrkan=20=C4=B0ndibay?= Date: Wed, 28 Feb 2024 11:58:28 +0300 Subject: [PATCH 1/6] Add support for alter/drop role propagation from non-main databases (#7461) DESCRIPTION: Adds support for distributed `ALTER/DROP ROLE` commands from the databases where Citus is not installed --------- Co-authored-by: Onur Tirtir --- .../distributed/commands/utility_hook.c | 212 +++++++++++++----- src/backend/distributed/metadata/distobject.c | 14 +- .../distributed/sql/citus--12.1-1--12.2-1.sql | 2 + .../sql/downgrades/citus--12.2-1--12.1-1.sql | 3 + .../12.2-1.sql | 7 + .../latest.sql | 7 +- .../metadata_sync_from_non_maindb.out | 143 ++++++++++++ src/test/regress/expected/multi_extension.out | 62 ++--- .../role_operations_from_non_maindb.out | 138 ++++++++++++ .../expected/upgrade_list_citus_objects.out | 2 +- src/test/regress/multi_schedule | 2 +- .../sql/metadata_sync_from_non_maindb.sql | 105 ++++++++- .../sql/role_operations_from_non_maindb.sql | 106 +++++++++ 13 files changed, 713 insertions(+), 90 deletions(-) create mode 100644 src/backend/distributed/sql/udfs/citus_unmark_object_distributed/12.2-1.sql create mode 100644 src/test/regress/expected/role_operations_from_non_maindb.out create mode 100644 src/test/regress/sql/role_operations_from_non_maindb.sql diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 7dff9cbf6..e264713dd 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -93,20 +93,40 @@ "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 as - * distributed explicitly (*). + * supported from non-main databases and whether it should be marked or unmarked + * as distributed. * - * (*) 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. + * 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; - bool explicitlyMarkAsDistributed; + DistObjectOperation DistObjectOperation; /* * checkSupportedObjectTypes is a callback function that checks whether @@ -118,15 +138,16 @@ typedef struct NonMainDbDistributedStatementInfo } NonMainDbDistributedStatementInfo; /* - * MarkObjectDistributedParams is used to pass parameters to the - * MarkObjectDistributedFromNonMainDb function. + * DistObjectOperationParams is used to pass parameters to the + * MarkObjectDistributedGloballyFromNonMainDb function and + * UnMarkObjectDistributedLocallyFromNonMainDb functions. */ -typedef struct MarkObjectDistributedParams +typedef struct DistObjectOperationParams { char *name; Oid id; uint16 catalogRelId; -} MarkObjectDistributedParams; +} DistObjectOperationParams; bool EnableDDLPropagation = true; /* ddl propagation is enabled */ @@ -166,9 +187,11 @@ static bool IsCommandToCreateOrDropMainDB(Node *parsetree); static void RunPreprocessMainDBCommand(Node *parsetree); static void RunPostprocessMainDBCommand(Node *parsetree); static bool IsStatementSupportedFromNonMainDb(Node *parsetree); -static bool StatementRequiresMarkDistributedFromNonMainDb(Node *parsetree); -static void MarkObjectDistributedFromNonMainDb(Node *parsetree); -static MarkObjectDistributedParams GetMarkObjectDistributedParams(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 @@ -184,12 +207,15 @@ static bool NonMainDbCheckSupportedObjectTypeForSecLabel(Node *node); */ ObjectType supportedObjectTypesForGrantStmt[] = { OBJECT_DATABASE }; static const NonMainDbDistributedStatementInfo NonMainDbSupportedStatements[] = { - { T_GrantRoleStmt, false, NULL }, - { T_CreateRoleStmt, true, NULL }, - { T_GrantStmt, false, NonMainDbCheckSupportedObjectTypeForGrant }, - { T_CreatedbStmt, false, NULL }, - { T_DropdbStmt, false, NULL }, - { T_SecLabelStmt, false, NonMainDbCheckSupportedObjectTypeForSecLabel }, + { 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 }, }; @@ -1743,12 +1769,19 @@ RunPreprocessMainDBCommand(Node *parsetree) 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); + } } @@ -1760,9 +1793,9 @@ static void RunPostprocessMainDBCommand(Node *parsetree) { if (IsStatementSupportedFromNonMainDb(parsetree) && - StatementRequiresMarkDistributedFromNonMainDb(parsetree)) + StatementRequiresMarkDistributedGloballyFromNonMainDb(parsetree)) { - MarkObjectDistributedFromNonMainDb(parsetree); + MarkObjectDistributedGloballyFromNonMainDb(parsetree); } } @@ -1793,11 +1826,11 @@ IsStatementSupportedFromNonMainDb(Node *parsetree) /* - * StatementRequiresMarkDistributedFromNonMainDb returns true if the statement should be marked + * StatementRequiresMarkDistributedGloballyFromNonMainDb returns true if the statement should be marked * as distributed when executed from a non-main database. */ static bool -StatementRequiresMarkDistributedFromNonMainDb(Node *parsetree) +StatementRequiresMarkDistributedGloballyFromNonMainDb(Node *parsetree) { NodeTag type = nodeTag(parsetree); @@ -1806,7 +1839,8 @@ StatementRequiresMarkDistributedFromNonMainDb(Node *parsetree) { if (type == NonMainDbSupportedStatements[i].statementType) { - return NonMainDbSupportedStatements[i].explicitlyMarkAsDistributed; + return NonMainDbSupportedStatements[i].DistObjectOperation == + MARK_DISTRIBUTED_GLOBALLY; } } @@ -1815,47 +1849,125 @@ StatementRequiresMarkDistributedFromNonMainDb(Node *parsetree) /* - * MarkObjectDistributedFromNonMainDb marks the given object as distributed on the - * non-main database. + * StatementRequiresUnmarkDistributedLocallyFromNonMainDb returns true if the statement should be unmarked + * as distributed when executed from a non-main database. */ -static void -MarkObjectDistributedFromNonMainDb(Node *parsetree) +static bool +StatementRequiresUnmarkDistributedLocallyFromNonMainDb(Node *parsetree) { - MarkObjectDistributedParams markObjectDistributedParams = - GetMarkObjectDistributedParams(parsetree); - StringInfo mainDBQuery = makeStringInfo(); - appendStringInfo(mainDBQuery, - MARK_OBJECT_DISTRIBUTED, - markObjectDistributedParams.catalogRelId, - quote_literal_cstr(markObjectDistributedParams.name), - markObjectDistributedParams.id, - quote_literal_cstr(CurrentUserName())); - RunCitusMainDBQuery(mainDBQuery->data); + 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; } /* - * GetMarkObjectDistributedParams returns MarkObjectDistributedParams for the target + * 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. */ -static MarkObjectDistributedParams -GetMarkObjectDistributedParams(Node *parsetree) +List * +GetDistObjectOperationParams(Node *parsetree) { + List *paramsList = NIL; if (IsA(parsetree, CreateRoleStmt)) { CreateRoleStmt *stmt = castNode(CreateRoleStmt, parsetree); - MarkObjectDistributedParams info = { - .name = stmt->role, - .catalogRelId = AuthIdRelationId, - .id = get_role_oid(stmt->role, false) - }; + DistObjectOperationParams *params = + (DistObjectOperationParams *) palloc(sizeof(DistObjectOperationParams)); + params->name = stmt->role; + params->catalogRelId = AuthIdRelationId; + params->id = get_role_oid(stmt->role, false); - return info; + 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"); } - /* Add else if branches for other statement types */ - - elog(ERROR, "unsupported statement type"); + return paramsList; } diff --git a/src/backend/distributed/metadata/distobject.c b/src/backend/distributed/metadata/distobject.c index 007d07bdc..ff5b2c7a9 100644 --- a/src/backend/distributed/metadata/distobject.c +++ b/src/backend/distributed/metadata/distobject.c @@ -98,10 +98,10 @@ mark_object_distributed(PG_FUNCTION_ARGS) /* - * citus_unmark_object_distributed(classid oid, objid oid, objsubid int) + * citus_unmark_object_distributed(classid oid, objid oid, objsubid int,checkobjectexistence bool) * - * removes the entry for an object address from pg_dist_object. Only removes the entry if - * the object does not exist anymore. + * Removes the entry for an object address from pg_dist_object. If checkobjectexistence is true, + * throws an error if the object still exists. */ Datum citus_unmark_object_distributed(PG_FUNCTION_ARGS) @@ -109,6 +109,12 @@ citus_unmark_object_distributed(PG_FUNCTION_ARGS) Oid classid = PG_GETARG_OID(0); Oid objid = PG_GETARG_OID(1); int32 objsubid = PG_GETARG_INT32(2); + bool checkObjectExistence = true; + if (!PG_ARGISNULL(3)) + { + checkObjectExistence = PG_GETARG_BOOL(3); + } + ObjectAddress address = { 0 }; ObjectAddressSubSet(address, classid, objid, objsubid); @@ -119,7 +125,7 @@ citus_unmark_object_distributed(PG_FUNCTION_ARGS) PG_RETURN_VOID(); } - if (ObjectExists(&address)) + if (checkObjectExistence && ObjectExists(&address)) { ereport(ERROR, (errmsg("object still exists"), errdetail("the %s \"%s\" still exists", diff --git a/src/backend/distributed/sql/citus--12.1-1--12.2-1.sql b/src/backend/distributed/sql/citus--12.1-1--12.2-1.sql index 68823b3be..2d5f88676 100644 --- a/src/backend/distributed/sql/citus--12.1-1--12.2-1.sql +++ b/src/backend/distributed/sql/citus--12.1-1--12.2-1.sql @@ -7,6 +7,8 @@ #include "udfs/start_management_transaction/12.2-1.sql" #include "udfs/execute_command_on_remote_nodes_as_user/12.2-1.sql" #include "udfs/mark_object_distributed/12.2-1.sql" +DROP FUNCTION pg_catalog.citus_unmark_object_distributed(oid, oid, int); +#include "udfs/citus_unmark_object_distributed/12.2-1.sql" #include "udfs/commit_management_command_2pc/12.2-1.sql" ALTER TABLE pg_catalog.pg_dist_transaction ADD COLUMN outer_xid xid8; diff --git a/src/backend/distributed/sql/downgrades/citus--12.2-1--12.1-1.sql b/src/backend/distributed/sql/downgrades/citus--12.2-1--12.1-1.sql index 5b2828cfe..581c65ea8 100644 --- a/src/backend/distributed/sql/downgrades/citus--12.2-1--12.1-1.sql +++ b/src/backend/distributed/sql/downgrades/citus--12.2-1--12.1-1.sql @@ -18,6 +18,9 @@ DROP FUNCTION citus_internal.mark_object_distributed( classId Oid, objectName text, objectId Oid, connectionUser text ); +DROP FUNCTION pg_catalog.citus_unmark_object_distributed(oid,oid,int,boolean); +#include "../udfs/citus_unmark_object_distributed/10.0-1.sql" + DROP FUNCTION citus_internal.commit_management_command_2pc(); ALTER TABLE pg_catalog.pg_dist_transaction DROP COLUMN outer_xid; diff --git a/src/backend/distributed/sql/udfs/citus_unmark_object_distributed/12.2-1.sql b/src/backend/distributed/sql/udfs/citus_unmark_object_distributed/12.2-1.sql new file mode 100644 index 000000000..3c1b1bdec --- /dev/null +++ b/src/backend/distributed/sql/udfs/citus_unmark_object_distributed/12.2-1.sql @@ -0,0 +1,7 @@ +CREATE FUNCTION pg_catalog.citus_unmark_object_distributed(classid oid, objid oid, objsubid int, checkobjectexistence boolean DEFAULT true) + RETURNS void + LANGUAGE C STRICT + AS 'MODULE_PATHNAME', $$citus_unmark_object_distributed$$; +COMMENT ON FUNCTION pg_catalog.citus_unmark_object_distributed(classid oid, objid oid, objsubid int, checkobjectexistence boolean) + IS 'Removes an object from citus.pg_dist_object after deletion. If checkobjectexistence is true, object existence check performed.' + 'Otherwise, object existence check is skipped.'; diff --git a/src/backend/distributed/sql/udfs/citus_unmark_object_distributed/latest.sql b/src/backend/distributed/sql/udfs/citus_unmark_object_distributed/latest.sql index 3f60c60c3..3c1b1bdec 100644 --- a/src/backend/distributed/sql/udfs/citus_unmark_object_distributed/latest.sql +++ b/src/backend/distributed/sql/udfs/citus_unmark_object_distributed/latest.sql @@ -1,6 +1,7 @@ -CREATE FUNCTION pg_catalog.citus_unmark_object_distributed(classid oid, objid oid, objsubid int) +CREATE FUNCTION pg_catalog.citus_unmark_object_distributed(classid oid, objid oid, objsubid int, checkobjectexistence boolean DEFAULT true) RETURNS void LANGUAGE C STRICT AS 'MODULE_PATHNAME', $$citus_unmark_object_distributed$$; -COMMENT ON FUNCTION pg_catalog.citus_unmark_object_distributed(classid oid, objid oid, objsubid int) - IS 'remove an object address from citus.pg_dist_object once the object has been deleted'; +COMMENT ON FUNCTION pg_catalog.citus_unmark_object_distributed(classid oid, objid oid, objsubid int, checkobjectexistence boolean) + IS 'Removes an object from citus.pg_dist_object after deletion. If checkobjectexistence is true, object existence check performed.' + 'Otherwise, object existence check is skipped.'; 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 91ca1c82d..6630b39bd 100644 --- a/src/test/regress/expected/metadata_sync_from_non_maindb.out +++ b/src/test/regress/expected/metadata_sync_from_non_maindb.out @@ -164,6 +164,149 @@ revoke CONNECT on database metadata_sync_2pc_db from "grant_role2pc'_user2"; revoke CREATE on database metadata_sync_2pc_db from "grant_role2pc'_user1"; \c regression drop user "grant_role2pc'_user1","grant_role2pc'_user2","grant_role2pc'_user3",grant_role2pc_user4,grant_role2pc_user5; +--test for user operations +--test for create user +\c regression - - :master_port +select 1 from citus_remove_node('localhost', :worker_2_port); + ?column? +--------------------------------------------------------------------- + 1 +(1 row) + +\c metadata_sync_2pc_db - - :master_port +CREATE ROLE test_role1 WITH LOGIN PASSWORD 'password1'; +\c metadata_sync_2pc_db - - :worker_1_port +CREATE USER "test_role2-needs\!escape" +WITH + SUPERUSER CREATEDB CREATEROLE INHERIT LOGIN REPLICATION BYPASSRLS CONNECTION +LIMIT 10 VALID UNTIL '2023-01-01' IN ROLE test_role1; +create role test_role3; +\c regression - - :master_port +select 1 from citus_add_node('localhost', :worker_2_port); + ?column? +--------------------------------------------------------------------- + 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 ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape','test_role3') + ORDER BY rolname + ) t +$$); + 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"}] +(3 rows) + +--test for alter user +select 1 from citus_remove_node('localhost', :worker_2_port); + ?column? +--------------------------------------------------------------------- + 1 +(1 row) + +\c metadata_sync_2pc_db - - :master_port +-- Test ALTER ROLE with various options +ALTER ROLE test_role1 WITH PASSWORD 'new_password1'; +\c metadata_sync_2pc_db - - :worker_1_port +ALTER USER "test_role2-needs\!escape" +WITH + NOSUPERUSER NOCREATEDB NOCREATEROLE NOINHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION +LIMIT 5 VALID UNTIL '2024-01-01'; +\c regression - - :master_port +select 1 from citus_add_node('localhost', :worker_2_port); + ?column? +--------------------------------------------------------------------- + 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 ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape','test_role3') + ORDER BY rolname + ) t +$$); + 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"}] +(3 rows) + +--test for drop user +select 1 from citus_remove_node('localhost', :worker_2_port); + ?column? +--------------------------------------------------------------------- + 1 +(1 row) + +\c metadata_sync_2pc_db - - :worker_1_port +DROP ROLE test_role1, "test_role2-needs\!escape"; +\c metadata_sync_2pc_db - - :master_port +DROP ROLE test_role3; +\c regression - - :master_port +select 1 from citus_add_node('localhost', :worker_2_port); + ?column? +--------------------------------------------------------------------- + 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 ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape','test_role3') + ORDER BY rolname + ) t +$$); + 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"}] +(3 rows) + +-- Clean up: drop the database on worker node 2 +\c regression - - :worker_2_port +DROP ROLE if exists test_role1, "test_role2-needs\!escape", test_role3; +\c regression - - :master_port +select result FROM run_command_on_all_nodes($$ + SELECT array_to_json(array_agg(row_to_json(t))) + FROM ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape','test_role3') + ORDER BY rolname + ) t +$$); + result +--------------------------------------------------------------------- + + + +(3 rows) + set citus.enable_create_database_propagation to on; drop database metadata_sync_2pc_db; drop schema metadata_sync_2pc_schema; diff --git a/src/test/regress/expected/multi_extension.out b/src/test/regress/expected/multi_extension.out index 0aecd652f..aaafce715 100644 --- a/src/test/regress/expected/multi_extension.out +++ b/src/test/regress/expected/multi_extension.out @@ -1420,37 +1420,39 @@ SELECT * FROM multi_extension.print_extension_changes(); -- Snapshot of state at 12.2-1 ALTER EXTENSION citus UPDATE TO '12.2-1'; SELECT * FROM multi_extension.print_extension_changes(); - previous_object | current_object + previous_object | current_object --------------------------------------------------------------------- - | function citus_internal.acquire_citus_advisory_object_class_lock(integer,cstring) void - | function citus_internal.add_colocation_metadata(integer,integer,integer,regtype,oid) void - | function citus_internal.add_object_metadata(text,text[],text[],integer,integer,boolean) void - | function citus_internal.add_partition_metadata(regclass,"char",text,integer,"char") void - | function citus_internal.add_placement_metadata(bigint,bigint,integer,bigint) void - | function citus_internal.add_shard_metadata(regclass,bigint,"char",text,text) void - | function citus_internal.add_tenant_schema(oid,integer) void - | function citus_internal.adjust_local_clock_to_remote(cluster_clock) void - | function citus_internal.commit_management_command_2pc() void - | function citus_internal.database_command(text) void - | function citus_internal.delete_colocation_metadata(integer) void - | function citus_internal.delete_partition_metadata(regclass) void - | function citus_internal.delete_placement_metadata(bigint) void - | function citus_internal.delete_shard_metadata(bigint) void - | function citus_internal.delete_tenant_schema(oid) void - | function citus_internal.execute_command_on_remote_nodes_as_user(text,text) void - | function citus_internal.global_blocked_processes() SETOF record - | function citus_internal.is_replication_origin_tracking_active() boolean - | function citus_internal.local_blocked_processes() SETOF record - | function citus_internal.mark_node_not_synced(integer,integer) void - | function citus_internal.mark_object_distributed(oid,text,oid,text) void - | function citus_internal.start_management_transaction(xid8) void - | function citus_internal.start_replication_origin_tracking() void - | function citus_internal.stop_replication_origin_tracking() void - | function citus_internal.unregister_tenant_schema_globally(oid,text) void - | function citus_internal.update_none_dist_table_metadata(oid,"char",bigint,boolean) void - | function citus_internal.update_placement_metadata(bigint,integer,integer) void - | function citus_internal.update_relation_colocation(oid,integer) void -(28 rows) + function citus_unmark_object_distributed(oid,oid,integer) void | + | function citus_internal.acquire_citus_advisory_object_class_lock(integer,cstring) void + | function citus_internal.add_colocation_metadata(integer,integer,integer,regtype,oid) void + | function citus_internal.add_object_metadata(text,text[],text[],integer,integer,boolean) void + | function citus_internal.add_partition_metadata(regclass,"char",text,integer,"char") void + | function citus_internal.add_placement_metadata(bigint,bigint,integer,bigint) void + | function citus_internal.add_shard_metadata(regclass,bigint,"char",text,text) void + | function citus_internal.add_tenant_schema(oid,integer) void + | function citus_internal.adjust_local_clock_to_remote(cluster_clock) void + | function citus_internal.commit_management_command_2pc() void + | function citus_internal.database_command(text) void + | function citus_internal.delete_colocation_metadata(integer) void + | function citus_internal.delete_partition_metadata(regclass) void + | function citus_internal.delete_placement_metadata(bigint) void + | function citus_internal.delete_shard_metadata(bigint) void + | function citus_internal.delete_tenant_schema(oid) void + | function citus_internal.execute_command_on_remote_nodes_as_user(text,text) void + | function citus_internal.global_blocked_processes() SETOF record + | function citus_internal.is_replication_origin_tracking_active() boolean + | function citus_internal.local_blocked_processes() SETOF record + | function citus_internal.mark_node_not_synced(integer,integer) void + | function citus_internal.mark_object_distributed(oid,text,oid,text) void + | function citus_internal.start_management_transaction(xid8) void + | function citus_internal.start_replication_origin_tracking() void + | function citus_internal.stop_replication_origin_tracking() void + | function citus_internal.unregister_tenant_schema_globally(oid,text) void + | function citus_internal.update_none_dist_table_metadata(oid,"char",bigint,boolean) void + | function citus_internal.update_placement_metadata(bigint,integer,integer) void + | function citus_internal.update_relation_colocation(oid,integer) void + | function citus_unmark_object_distributed(oid,oid,integer,boolean) void +(30 rows) DROP TABLE multi_extension.prev_objects, multi_extension.extension_diff; -- show running version diff --git a/src/test/regress/expected/role_operations_from_non_maindb.out b/src/test/regress/expected/role_operations_from_non_maindb.out new file mode 100644 index 000000000..3b51c89b0 --- /dev/null +++ b/src/test/regress/expected/role_operations_from_non_maindb.out @@ -0,0 +1,138 @@ +-- Create a new database +set citus.enable_create_database_propagation to on; +CREATE DATABASE role_operations_test_db; +SET citus.superuser TO 'postgres'; +-- Connect to the new database +\c role_operations_test_db +-- Test CREATE ROLE with various options +CREATE ROLE test_role1 WITH LOGIN PASSWORD 'password1'; +\c role_operations_test_db - - :worker_1_port +CREATE USER "test_role2-needs\!escape" +WITH + SUPERUSER CREATEDB CREATEROLE INHERIT LOGIN REPLICATION BYPASSRLS CONNECTION +LIMIT 10 VALID UNTIL '2023-01-01' IN ROLE test_role1; +\c regression - - :master_port +select result FROM run_command_on_all_nodes($$ + SELECT array_to_json(array_agg(row_to_json(t))) + FROM ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape') + ORDER BY rolname + ) t +$$); + 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_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_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"}] +(3 rows) + +select result FROM run_command_on_all_nodes($$ + SELECT array_to_json(array_agg(row_to_json(t))) + FROM ( + SELECT r.rolname + FROM pg_dist_object d + JOIN pg_roles r ON d.objid = r.oid + WHERE r.rolname IN ('test_role1', 'test_role2-needs\!escape') + order by r.rolname + ) t +$$); + result +--------------------------------------------------------------------- + [{"rolname":"test_role1"},{"rolname":"test_role2-needs\\!escape"}] + [{"rolname":"test_role1"},{"rolname":"test_role2-needs\\!escape"}] + [{"rolname":"test_role1"},{"rolname":"test_role2-needs\\!escape"}] +(3 rows) + +\c role_operations_test_db - - :master_port +-- Test ALTER ROLE with various options +ALTER ROLE test_role1 WITH PASSWORD 'new_password1'; +\c role_operations_test_db - - :worker_1_port +ALTER USER "test_role2-needs\!escape" +WITH + NOSUPERUSER NOCREATEDB NOCREATEROLE NOINHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION +LIMIT 5 VALID UNTIL '2024-01-01'; +\c regression - - :master_port +select result FROM run_command_on_all_nodes($$ + SELECT array_to_json(array_agg(row_to_json(t))) + FROM ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape') + ORDER BY rolname + ) t +$$); + 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_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_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"}] +(3 rows) + +\c role_operations_test_db - - :master_port +-- Test DROP ROLE +DROP ROLE no_such_role; -- fails nicely +ERROR: role "no_such_role" does not exist +DROP ROLE IF EXISTS no_such_role; -- doesn't fail +NOTICE: role "no_such_role" does not exist, skipping +CREATE ROLE new_role; +DROP ROLE IF EXISTS no_such_role, new_role; -- doesn't fail +NOTICE: role "no_such_role" does not exist, skipping +DROP ROLE IF EXISTS test_role1, "test_role2-needs\!escape"; +\c regression - - :master_port +--verify that roles and dist_object are dropped +select result FROM run_command_on_all_nodes($$ + SELECT array_to_json(array_agg(row_to_json(t))) + FROM ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape','new_role','no_such_role') + ORDER BY rolname + ) t +$$); + result +--------------------------------------------------------------------- + + + +(3 rows) + +select result FROM run_command_on_all_nodes($$ + SELECT array_to_json(array_agg(row_to_json(t))) + FROM ( + SELECT r.rolname + FROM pg_roles r + WHERE r.rolname IN ('test_role1', 'test_role2-needs\!escape','new_role','no_such_role') + order by r.rolname + ) t +$$); + result +--------------------------------------------------------------------- + + + +(3 rows) + +SELECT result FROM run_command_on_all_nodes($$ + SELECT count(*) leaked_pg_dist_object_records_for_roles + FROM pg_dist_object LEFT JOIN pg_authid ON (objid = oid) + WHERE classid = 1260 AND oid IS NULL +$$); + result +--------------------------------------------------------------------- + 0 + 0 + 0 +(3 rows) + +-- Clean up: drop the database +set citus.enable_create_database_propagation to on; +DROP DATABASE role_operations_test_db; +reset citus.enable_create_database_propagation; diff --git a/src/test/regress/expected/upgrade_list_citus_objects.out b/src/test/regress/expected/upgrade_list_citus_objects.out index 4f17695be..ca31b222b 100644 --- a/src/test/regress/expected/upgrade_list_citus_objects.out +++ b/src/test/regress/expected/upgrade_list_citus_objects.out @@ -174,7 +174,7 @@ ORDER BY 1; function citus_text_send_as_jsonb(text) function citus_total_relation_size(regclass,boolean) function citus_truncate_trigger() - function citus_unmark_object_distributed(oid,oid,integer) + function citus_unmark_object_distributed(oid,oid,integer,boolean) function citus_update_node(integer,text,integer,boolean,integer) function citus_update_shard_statistics(bigint) function citus_update_table_statistics(regclass) diff --git a/src/test/regress/multi_schedule b/src/test/regress/multi_schedule index 85de7b8b8..3fec50aac 100644 --- a/src/test/regress/multi_schedule +++ b/src/test/regress/multi_schedule @@ -108,7 +108,7 @@ test: object_propagation_debug test: undistribute_table test: run_command_on_all_nodes test: background_task_queue_monitor -test: other_databases grant_role_from_non_maindb seclabel_non_maindb +test: other_databases grant_role_from_non_maindb role_operations_from_non_maindb seclabel_non_maindb test: citus_internal_access # Causal clock test 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 93445be27..67e1e98d1 100644 --- a/src/test/regress/sql/metadata_sync_from_non_maindb.sql +++ b/src/test/regress/sql/metadata_sync_from_non_maindb.sql @@ -80,9 +80,112 @@ revoke CREATE on database metadata_sync_2pc_db from "grant_role2pc'_user1"; \c regression drop user "grant_role2pc'_user1","grant_role2pc'_user2","grant_role2pc'_user3",grant_role2pc_user4,grant_role2pc_user5; +--test for user operations + +--test for create user +\c regression - - :master_port +select 1 from citus_remove_node('localhost', :worker_2_port); + +\c metadata_sync_2pc_db - - :master_port +CREATE ROLE test_role1 WITH LOGIN PASSWORD 'password1'; + +\c metadata_sync_2pc_db - - :worker_1_port +CREATE USER "test_role2-needs\!escape" +WITH + SUPERUSER CREATEDB CREATEROLE INHERIT LOGIN REPLICATION BYPASSRLS CONNECTION +LIMIT 10 VALID UNTIL '2023-01-01' IN ROLE test_role1; + +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 ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape','test_role3') + ORDER BY rolname + ) t +$$); + +--test for alter user +select 1 from citus_remove_node('localhost', :worker_2_port); +\c metadata_sync_2pc_db - - :master_port +-- Test ALTER ROLE with various options +ALTER ROLE test_role1 WITH PASSWORD 'new_password1'; + +\c metadata_sync_2pc_db - - :worker_1_port +ALTER USER "test_role2-needs\!escape" +WITH + NOSUPERUSER NOCREATEDB NOCREATEROLE NOINHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION +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 ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape','test_role3') + ORDER BY rolname + ) t +$$); + +--test for drop user +select 1 from citus_remove_node('localhost', :worker_2_port); + +\c metadata_sync_2pc_db - - :worker_1_port +DROP ROLE test_role1, "test_role2-needs\!escape"; + +\c metadata_sync_2pc_db - - :master_port +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 ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape','test_role3') + ORDER BY rolname + ) t +$$); + +-- Clean up: drop the database on worker node 2 +\c regression - - :worker_2_port +DROP ROLE if exists test_role1, "test_role2-needs\!escape", test_role3; + +\c regression - - :master_port + +select result FROM run_command_on_all_nodes($$ + SELECT array_to_json(array_agg(row_to_json(t))) + FROM ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape','test_role3') + ORDER BY rolname + ) t +$$); + set citus.enable_create_database_propagation to on; drop database metadata_sync_2pc_db; drop schema metadata_sync_2pc_schema; - reset citus.enable_create_database_propagation; reset search_path; diff --git a/src/test/regress/sql/role_operations_from_non_maindb.sql b/src/test/regress/sql/role_operations_from_non_maindb.sql new file mode 100644 index 000000000..5f569208b --- /dev/null +++ b/src/test/regress/sql/role_operations_from_non_maindb.sql @@ -0,0 +1,106 @@ +-- Create a new database +set citus.enable_create_database_propagation to on; +CREATE DATABASE role_operations_test_db; +SET citus.superuser TO 'postgres'; +-- Connect to the new database +\c role_operations_test_db +-- Test CREATE ROLE with various options +CREATE ROLE test_role1 WITH LOGIN PASSWORD 'password1'; + +\c role_operations_test_db - - :worker_1_port +CREATE USER "test_role2-needs\!escape" +WITH + SUPERUSER CREATEDB CREATEROLE INHERIT LOGIN REPLICATION BYPASSRLS CONNECTION +LIMIT 10 VALID UNTIL '2023-01-01' IN ROLE test_role1; + +\c regression - - :master_port + +select result FROM run_command_on_all_nodes($$ + SELECT array_to_json(array_agg(row_to_json(t))) + FROM ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape') + ORDER BY rolname + ) t +$$); + +select result FROM run_command_on_all_nodes($$ + SELECT array_to_json(array_agg(row_to_json(t))) + FROM ( + SELECT r.rolname + FROM pg_dist_object d + JOIN pg_roles r ON d.objid = r.oid + WHERE r.rolname IN ('test_role1', 'test_role2-needs\!escape') + order by r.rolname + ) t +$$); + +\c role_operations_test_db - - :master_port +-- Test ALTER ROLE with various options +ALTER ROLE test_role1 WITH PASSWORD 'new_password1'; + +\c role_operations_test_db - - :worker_1_port +ALTER USER "test_role2-needs\!escape" +WITH + NOSUPERUSER NOCREATEDB NOCREATEROLE NOINHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION +LIMIT 5 VALID UNTIL '2024-01-01'; + +\c regression - - :master_port +select result FROM run_command_on_all_nodes($$ + SELECT array_to_json(array_agg(row_to_json(t))) + FROM ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape') + ORDER BY rolname + ) t +$$); + +\c role_operations_test_db - - :master_port +-- Test DROP ROLE +DROP ROLE no_such_role; -- fails nicely +DROP ROLE IF EXISTS no_such_role; -- doesn't fail + +CREATE ROLE new_role; +DROP ROLE IF EXISTS no_such_role, new_role; -- doesn't fail +DROP ROLE IF EXISTS test_role1, "test_role2-needs\!escape"; + +\c regression - - :master_port +--verify that roles and dist_object are dropped +select result FROM run_command_on_all_nodes($$ + SELECT array_to_json(array_agg(row_to_json(t))) + FROM ( + SELECT rolname, rolsuper, rolinherit, rolcreaterole, rolcreatedb, + rolcanlogin, rolreplication, rolbypassrls, rolconnlimit, + (rolpassword != '') as pass_not_empty, DATE(rolvaliduntil) + FROM pg_authid + WHERE rolname in ('test_role1', 'test_role2-needs\!escape','new_role','no_such_role') + ORDER BY rolname + ) t +$$); + +select result FROM run_command_on_all_nodes($$ + SELECT array_to_json(array_agg(row_to_json(t))) + FROM ( + SELECT r.rolname + FROM pg_roles r + WHERE r.rolname IN ('test_role1', 'test_role2-needs\!escape','new_role','no_such_role') + order by r.rolname + ) t +$$); + +SELECT result FROM run_command_on_all_nodes($$ + SELECT count(*) leaked_pg_dist_object_records_for_roles + FROM pg_dist_object LEFT JOIN pg_authid ON (objid = oid) + WHERE classid = 1260 AND oid IS NULL +$$); + +-- Clean up: drop the database +set citus.enable_create_database_propagation to on; +DROP DATABASE role_operations_test_db; +reset citus.enable_create_database_propagation; From d59c93bc504ad32621d66583de6b65f936b0ed13 Mon Sep 17 00:00:00 2001 From: sminux Date: Tue, 5 Mar 2024 10:49:35 +0300 Subject: [PATCH 2/6] fix bad copy-paste rightComparisonLimit (#7547) DESCRIPTION: change for #7543 --- src/backend/columnar/columnar_tableam.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/columnar/columnar_tableam.c b/src/backend/columnar/columnar_tableam.c index 40486d08f..ca3a5f4c4 100644 --- a/src/backend/columnar/columnar_tableam.c +++ b/src/backend/columnar/columnar_tableam.c @@ -2946,7 +2946,7 @@ MajorVersionsCompatibleColumnar(char *leftVersion, char *rightVersion) } else { - rightComparisionLimit = strlen(leftVersion); + rightComparisionLimit = strlen(rightVersion); } /* we can error out early if hypens are not in the same position */ From edcdbe67b11c6ec5023dd18e537f3b876f51187d Mon Sep 17 00:00:00 2001 From: eaydingol <60466783+eaydingol@users.noreply.github.com> Date: Wed, 6 Mar 2024 14:46:49 +0300 Subject: [PATCH 3/6] Fix: store the previous shard cost for order verification (#7550) Store the previous shard cost so that the invariant checking performs as expected. --- src/backend/distributed/operations/shard_rebalancer.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/backend/distributed/operations/shard_rebalancer.c b/src/backend/distributed/operations/shard_rebalancer.c index d1868d3c4..03dc4c1b8 100644 --- a/src/backend/distributed/operations/shard_rebalancer.c +++ b/src/backend/distributed/operations/shard_rebalancer.c @@ -384,6 +384,7 @@ CheckRebalanceStateInvariants(const RebalanceState *state) Assert(shardCost->cost <= prevShardCost->cost); } totalCost += shardCost->cost; + prevShardCost = shardCost; } /* Check that utilization field is up to date. */ From f0043b64a17382e340bed5c61ec0025d0af34379 Mon Sep 17 00:00:00 2001 From: Karina <55838532+Green-Chan@users.noreply.github.com> Date: Thu, 7 Mar 2024 13:08:19 +0300 Subject: [PATCH 4/6] Fix server crash when trying to execute activate_node_snapshot() on a single-node cluster (#7552) This fixes #7551 reported by Egor Chindyaskin Function activate_node_snapshot() is not meant to be called on a cluster without worker nodes. This commit adds ERROR report for such case to prevent server crash. --- src/backend/distributed/test/metadata_sync.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/backend/distributed/test/metadata_sync.c b/src/backend/distributed/test/metadata_sync.c index dec20c772..ce025cff9 100644 --- a/src/backend/distributed/test/metadata_sync.c +++ b/src/backend/distributed/test/metadata_sync.c @@ -50,6 +50,13 @@ activate_node_snapshot(PG_FUNCTION_ARGS) * so we are using first primary worker node just for test purposes. */ WorkerNode *dummyWorkerNode = GetFirstPrimaryWorkerNode(); + if (dummyWorkerNode == NULL) + { + ereport(ERROR, (errmsg("no worker nodes found"), + errdetail("Function activate_node_snapshot is meant to be " + "used when running tests on a multi-node cluster " + "with workers."))); + } /* * Create MetadataSyncContext which is used throughout nodes' activation. From 12f56438fc85a676a79b1a2886cc69ab872c9d14 Mon Sep 17 00:00:00 2001 From: copetol <40788226+copetol@users.noreply.github.com> Date: Fri, 8 Mar 2024 16:21:42 +0300 Subject: [PATCH 5/6] Fix segfault when using certain DO block in function (#7554) When using a CASE WHEN expression in the body of the function that is used in the DO block, a segmentation fault occured. This fixes that. Fixes #7381 --------- Co-authored-by: Konstantin Morozov --- .../planner/function_call_delegation.c | 4 +++ .../expected/function_with_case_when.out | 32 +++++++++++++++++++ src/test/regress/multi_schedule | 1 + .../regress/sql/function_with_case_when.sql | 27 ++++++++++++++++ 4 files changed, 64 insertions(+) create mode 100644 src/test/regress/expected/function_with_case_when.out create mode 100644 src/test/regress/sql/function_with_case_when.sql diff --git a/src/backend/distributed/planner/function_call_delegation.c b/src/backend/distributed/planner/function_call_delegation.c index f17b02347..4a79dc25a 100644 --- a/src/backend/distributed/planner/function_call_delegation.c +++ b/src/backend/distributed/planner/function_call_delegation.c @@ -91,6 +91,10 @@ bool InDelegatedFunctionCall = false; static bool contain_param_walker(Node *node, void *context) { + if (node == NULL) + { + return false; + } if (IsA(node, Param)) { Param *paramNode = (Param *) node; diff --git a/src/test/regress/expected/function_with_case_when.out b/src/test/regress/expected/function_with_case_when.out new file mode 100644 index 000000000..18df5be0a --- /dev/null +++ b/src/test/regress/expected/function_with_case_when.out @@ -0,0 +1,32 @@ +CREATE SCHEMA function_with_case; +SET search_path TO function_with_case; +-- create function +CREATE OR REPLACE FUNCTION test_err(v1 text) + RETURNS text + LANGUAGE plpgsql + SECURITY DEFINER +AS $function$ + +begin + return v1 || ' - ok'; +END; +$function$; +do $$ declare + lNewValues text; + val text; +begin + val = 'test'; + lNewValues = test_err(v1 => case when val::text = 'test'::text then 'yes' else 'no' end); + raise notice 'lNewValues= %', lNewValues; +end;$$ ; +NOTICE: lNewValues= yes - ok +CONTEXT: PL/pgSQL function inline_code_block line XX at RAISE +-- call function +SELECT test_err('test'); + test_err +--------------------------------------------------------------------- + test - ok +(1 row) + +DROP SCHEMA function_with_case CASCADE; +NOTICE: drop cascades to function test_err(text) diff --git a/src/test/regress/multi_schedule b/src/test/regress/multi_schedule index 3fec50aac..67a6e23a8 100644 --- a/src/test/regress/multi_schedule +++ b/src/test/regress/multi_schedule @@ -110,6 +110,7 @@ test: run_command_on_all_nodes test: background_task_queue_monitor test: other_databases grant_role_from_non_maindb role_operations_from_non_maindb seclabel_non_maindb test: citus_internal_access +test: function_with_case_when # Causal clock test test: clock diff --git a/src/test/regress/sql/function_with_case_when.sql b/src/test/regress/sql/function_with_case_when.sql new file mode 100644 index 000000000..03c6678e4 --- /dev/null +++ b/src/test/regress/sql/function_with_case_when.sql @@ -0,0 +1,27 @@ +CREATE SCHEMA function_with_case; +SET search_path TO function_with_case; + +-- create function +CREATE OR REPLACE FUNCTION test_err(v1 text) + RETURNS text + LANGUAGE plpgsql + SECURITY DEFINER +AS $function$ + +begin + return v1 || ' - ok'; +END; +$function$; +do $$ declare + lNewValues text; + val text; +begin + val = 'test'; + lNewValues = test_err(v1 => case when val::text = 'test'::text then 'yes' else 'no' end); + raise notice 'lNewValues= %', lNewValues; +end;$$ ; + +-- call function +SELECT test_err('test'); + +DROP SCHEMA function_with_case CASCADE; From 8afa2d0386ff303be29060b709f88994b9648b6c Mon Sep 17 00:00:00 2001 From: eaydingol <60466783+eaydingol@users.noreply.github.com> Date: Sun, 10 Mar 2024 10:20:08 +0300 Subject: [PATCH 6/6] Change the order in which the locks are acquired (#7542) This PR changes the order in which the locks are acquired (for the target and reference tables), when a modify request is initiated from a worker node that is not the "FirstWorkerNode". To prevent concurrent writes, locks are acquired on the first worker node for the replicated tables. When the update statement originates from the first worker node, it acquires the lock on the reference table(s) first, followed by the target table(s). However, if the update statement is initiated in another worker node, the lock requests are sent to the first worker in a different order. This PR unifies the modification order on the first worker node. With the third commit, independent of the node that received the request, the locks are acquired for the modified table and then the reference tables on the first node. The first commit shows a sample output for the test prior to the fix. Fixes #7477 --------- Co-authored-by: Jelte Fennema-Nio --- src/backend/distributed/utils/resource_lock.c | 28 ++++++--- src/test/regress/expected/issue_7477.out | 62 +++++++++++++++++++ src/test/regress/multi_schedule | 2 +- src/test/regress/sql/issue_7477.sql | 44 +++++++++++++ 4 files changed, 127 insertions(+), 9 deletions(-) create mode 100644 src/test/regress/expected/issue_7477.out create mode 100644 src/test/regress/sql/issue_7477.sql diff --git a/src/backend/distributed/utils/resource_lock.c b/src/backend/distributed/utils/resource_lock.c index 13e88a16e..8ac269e43 100644 --- a/src/backend/distributed/utils/resource_lock.c +++ b/src/backend/distributed/utils/resource_lock.c @@ -707,13 +707,27 @@ SerializeNonCommutativeWrites(List *shardIntervalList, LOCKMODE lockMode) } List *replicatedShardList = NIL; - if (AnyTableReplicated(shardIntervalList, &replicatedShardList)) - { - if (ClusterHasKnownMetadataWorkers() && !IsFirstWorkerNode()) - { - LockShardListResourcesOnFirstWorker(lockMode, replicatedShardList); - } + bool anyTableReplicated = AnyTableReplicated(shardIntervalList, &replicatedShardList); + /* + * Acquire locks on the modified table. + * If the table is replicated, the locks are first acquired on the first worker node then locally. + * But if we're already on the first worker, acquiring on the first worker node and locally are the same operation. + * So we only acquire locally in that case. + */ + if (anyTableReplicated && ClusterHasKnownMetadataWorkers() && !IsFirstWorkerNode()) + { + LockShardListResourcesOnFirstWorker(lockMode, replicatedShardList); + } + LockShardListResources(shardIntervalList, lockMode); + + /* + * Next, acquire locks on the reference tables that are referenced by a foreign key if there are any. + * Note that LockReferencedReferenceShardResources() first acquires locks on the first worker, + * then locally. + */ + if (anyTableReplicated) + { ShardInterval *firstShardInterval = (ShardInterval *) linitial(replicatedShardList); if (ReferenceTableShardId(firstShardInterval->shardId)) @@ -728,8 +742,6 @@ SerializeNonCommutativeWrites(List *shardIntervalList, LOCKMODE lockMode) LockReferencedReferenceShardResources(firstShardInterval->shardId, lockMode); } } - - LockShardListResources(shardIntervalList, lockMode); } diff --git a/src/test/regress/expected/issue_7477.out b/src/test/regress/expected/issue_7477.out new file mode 100644 index 000000000..224d85c6e --- /dev/null +++ b/src/test/regress/expected/issue_7477.out @@ -0,0 +1,62 @@ +--- Test for updating a table that has a foreign key reference to another reference table. +--- Issue #7477: Distributed deadlock after issuing a simple UPDATE statement +--- https://github.com/citusdata/citus/issues/7477 +CREATE TABLE table1 (id INT PRIMARY KEY); +SELECT create_reference_table('table1'); + create_reference_table +--------------------------------------------------------------------- + +(1 row) + +INSERT INTO table1 VALUES (1); +CREATE TABLE table2 ( + id INT, + info TEXT, + CONSTRAINT table1_id_fk FOREIGN KEY (id) REFERENCES table1 (id) + ); +SELECT create_reference_table('table2'); + create_reference_table +--------------------------------------------------------------------- + +(1 row) + +INSERT INTO table2 VALUES (1, 'test'); +--- Runs the update command in parallel on workers. +--- Due to bug #7477, before the fix, the result is non-deterministic +--- and have several rows of the form: +--- localhost | 57638 | f | ERROR: deadlock detected +--- localhost | 57637 | f | ERROR: deadlock detected +--- localhost | 57637 | f | ERROR: canceling the transaction since it was involved in a distributed deadlock +SELECT * FROM master_run_on_worker( + ARRAY['localhost', 'localhost','localhost', 'localhost','localhost', + 'localhost','localhost', 'localhost','localhost', 'localhost']::text[], + ARRAY[57638, 57637, 57637, 57638, 57637, 57638, 57637, 57638, 57638, 57637]::int[], + ARRAY['UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1' + ]::text[], + true); + node_name | node_port | success | result +--------------------------------------------------------------------- + localhost | 57638 | t | UPDATE 1 + localhost | 57637 | t | UPDATE 1 + localhost | 57637 | t | UPDATE 1 + localhost | 57638 | t | UPDATE 1 + localhost | 57637 | t | UPDATE 1 + localhost | 57638 | t | UPDATE 1 + localhost | 57637 | t | UPDATE 1 + localhost | 57638 | t | UPDATE 1 + localhost | 57638 | t | UPDATE 1 + localhost | 57637 | t | UPDATE 1 +(10 rows) + +--- cleanup +DROP TABLE table2; +DROP TABLE table1; diff --git a/src/test/regress/multi_schedule b/src/test/regress/multi_schedule index 67a6e23a8..af5921e60 100644 --- a/src/test/regress/multi_schedule +++ b/src/test/regress/multi_schedule @@ -103,7 +103,7 @@ test: multi_dropped_column_aliases foreign_key_restriction_enforcement test: binary_protocol test: alter_table_set_access_method test: alter_distributed_table -test: issue_5248 issue_5099 issue_5763 issue_6543 issue_6758 +test: issue_5248 issue_5099 issue_5763 issue_6543 issue_6758 issue_7477 test: object_propagation_debug test: undistribute_table test: run_command_on_all_nodes diff --git a/src/test/regress/sql/issue_7477.sql b/src/test/regress/sql/issue_7477.sql new file mode 100644 index 000000000..b9c1578e9 --- /dev/null +++ b/src/test/regress/sql/issue_7477.sql @@ -0,0 +1,44 @@ + +--- Test for updating a table that has a foreign key reference to another reference table. +--- Issue #7477: Distributed deadlock after issuing a simple UPDATE statement +--- https://github.com/citusdata/citus/issues/7477 + +CREATE TABLE table1 (id INT PRIMARY KEY); +SELECT create_reference_table('table1'); +INSERT INTO table1 VALUES (1); + +CREATE TABLE table2 ( + id INT, + info TEXT, + CONSTRAINT table1_id_fk FOREIGN KEY (id) REFERENCES table1 (id) + ); +SELECT create_reference_table('table2'); +INSERT INTO table2 VALUES (1, 'test'); + +--- Runs the update command in parallel on workers. +--- Due to bug #7477, before the fix, the result is non-deterministic +--- and have several rows of the form: +--- localhost | 57638 | f | ERROR: deadlock detected +--- localhost | 57637 | f | ERROR: deadlock detected +--- localhost | 57637 | f | ERROR: canceling the transaction since it was involved in a distributed deadlock + +SELECT * FROM master_run_on_worker( + ARRAY['localhost', 'localhost','localhost', 'localhost','localhost', + 'localhost','localhost', 'localhost','localhost', 'localhost']::text[], + ARRAY[57638, 57637, 57637, 57638, 57637, 57638, 57637, 57638, 57638, 57637]::int[], + ARRAY['UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1', + 'UPDATE table2 SET info = ''test_update'' WHERE id = 1' + ]::text[], + true); + +--- cleanup +DROP TABLE table2; +DROP TABLE table1;