Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds REASSIGN OWNED BY propagation #7319

Merged
merged 32 commits into from
Dec 28, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
d6ecac4
Adds REASSIGN OWNED BY propagation
gurkanindibay Oct 31, 2023
aff3f3f
Merge branch 'main' into reassign_owned_prop
gurkanindibay Nov 2, 2023
6426087
Merge branch 'main' into reassign_owned_prop
gurkanindibay Nov 10, 2023
76e5de9
Fixes affected test
gurkanindibay Nov 19, 2023
da900cf
Fixes failing test
gurkanindibay Nov 19, 2023
557dd71
Merge branch 'main' into reassign_owned_prop
gurkanindibay Nov 20, 2023
f342793
Fixes review notes
gurkanindibay Nov 22, 2023
f255643
Refactor EnsureDependenciesExistOnAllNodes to introduce EnsureObjectE…
gurkanindibay Nov 22, 2023
f1f7313
Adds dependencies
gurkanindibay Nov 22, 2023
b598b48
Fixes indentation
gurkanindibay Nov 22, 2023
e39c15f
Merge remote-tracking branch 'origin/main' into reassign_owned_prop
gurkanindibay Nov 26, 2023
aed2035
Fixes role propagation issue
gurkanindibay Nov 26, 2023
c7a98ac
Adds escape chars for a role
gurkanindibay Nov 26, 2023
2fec02c
Fixes test flakyness
gurkanindibay Nov 26, 2023
7632e5d
Fixes flakiness of tests
gurkanindibay Nov 26, 2023
3600f22
Sets role flags
gurkanindibay Nov 28, 2023
7433b79
Adds distributed check for new dependency function
gurkanindibay Nov 28, 2023
4ac5d0d
Fixes indentation
gurkanindibay Nov 28, 2023
9f82b16
Fixes build error
gurkanindibay Dec 17, 2023
6adac3a
Removes dependency check
gurkanindibay Dec 17, 2023
e825d29
Fixes method comments
gurkanindibay Dec 17, 2023
b5b6ecd
Fixes indentation
gurkanindibay Dec 17, 2023
c199c01
Merge branch 'main' into reassign_owned_prop
gurkanindibay Dec 19, 2023
059081e
Fixes test error
gurkanindibay Dec 17, 2023
e01d85b
Fixes review comments
gurkanindibay Dec 25, 2023
b62a958
Adds distributed check to dependency resolution
gurkanindibay Dec 25, 2023
9314ac8
Fixes the flakiness of tests
gurkanindibay Dec 25, 2023
80b90d6
Renames test output
gurkanindibay Dec 25, 2023
f2ebc68
Fixes test errors
gurkanindibay Dec 25, 2023
f49d429
Fixes review issues
gurkanindibay Dec 26, 2023
29e77ed
Merge branch 'main' into reassign_owned_prop
gurkanindibay Dec 26, 2023
5aa542e
Fixes test output
gurkanindibay Dec 25, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/backend/distributed/commands/distribute_object_ops.c
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,17 @@ static DistributeObjectOps Any_CreateRole = {
.address = CreateRoleStmtObjectAddress,
.markDistributed = true,
};

static DistributeObjectOps Any_ReassignOwned = {
.deparse = DeparseReassignOwnedStmt,
.qualify = NULL,
.preprocess = PreprocessReassignOwnedStmt,
.postprocess = NULL,
.operationType = DIST_OPS_DROP,
.address = NULL,
.markDistributed = false,
};

static DistributeObjectOps Any_DropOwned = {
.deparse = DeparseDropOwnedStmt,
.qualify = NULL,
Expand Down Expand Up @@ -1826,6 +1837,11 @@ GetDistributeObjectOps(Node *node)
return &Any_DropOwned;
}

case T_ReassignOwnedStmt:
{
return &Any_ReassignOwned;
}

case T_DropStmt:
{
DropStmt *stmt = castNode(DropStmt, node);
Expand Down
33 changes: 33 additions & 0 deletions src/backend/distributed/commands/owned.c
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,36 @@ PreprocessDropOwnedStmt(Node *node, const char *queryString,

return NodeDDLTaskList(NON_COORDINATOR_NODES, commands);
}


List *
PreprocessReassignOwnedStmt(Node *node, const char *queryString,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have this in Postprocess instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no reason to move this to Postprocess actually. It just prepares the command.
Normally when you check other statements who is in the nature of Alter like PreprocessAlterDatabaseStmt, we use Preprocess
Create needs to have Post since address only exists after local execution, Drop needs to be Pre since address is missing after local drop execution.
For Alter style statements, there is no such requirement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's get this into postprocess as we've discussed in group chat, due to reasons discussed there

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's get this into postprocess as we've discussed in group chat, due to reasons discussed there

(discussion in group chat was still on-going indeed)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to move this down to Postprocess phase

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

ProcessUtilityContext processUtilityContext)
{
ReassignOwnedStmt *stmt = castNode(ReassignOwnedStmt, node);
List *allReassignRoles = stmt->roles;

List *distributedReassignRoles = FilterDistributedRoles(allReassignRoles);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should remove filtering logic as we've discussed two weeks ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided to keep the filtering logic


if (list_length(distributedReassignRoles) <= 0)
{
return NIL;
}

if (!ShouldPropagate())
{
return NIL;
}

EnsureCoordinator();

stmt->roles = distributedReassignRoles;
char *sql = DeparseTreeNode((Node *) stmt);
stmt->roles = allReassignRoles;

List *commands = list_make3(DISABLE_DDL_PROPAGATION,
sql,
ENABLE_DDL_PROPAGATION);

return NodeDDLTaskList(NON_COORDINATOR_NODES, commands);
}
24 changes: 24 additions & 0 deletions src/backend/distributed/deparser/deparse_owned_stmts.c
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,27 @@ AppendRoleList(StringInfo buf, List *roleList)
}
}
}


static void
AppendReassignOwnedStmt(StringInfo buf, ReassignOwnedStmt *stmt)
{
appendStringInfo(buf, "REASSIGN OWNED BY ");

AppendRoleList(buf, stmt->roles);
char const *newRoleName = RoleSpecString(stmt->newrole, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor;

Suggested change
char const *newRoleName = RoleSpecString(stmt->newrole, true);
char *newRoleName = RoleSpecString(stmt->newrole, true);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without const I'm getting compile warning below

deparser/deparse_owned_stmts.c:94:22: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
94 | char *newRoleName = RoleSpecString(stmt->newrole, true);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's do const char instead of char const as we usually do then?

Copy link
Member

@onurctirtir onurctirtir Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please let's not forget changing this, see #7319 (comment)

appendStringInfo(buf, " TO %s", quote_identifier(newRoleName));
}


char *
DeparseReassignOwnedStmt(Node *node)
{
ReassignOwnedStmt *stmt = castNode(ReassignOwnedStmt, node);
StringInfoData buf = { 0 };
initStringInfo(&buf);

AppendReassignOwnedStmt(&buf, stmt);

return buf.data;
}
2 changes: 2 additions & 0 deletions src/include/distributed/commands.h
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,8 @@ extern List * CreateExtensionStmtObjectAddress(Node *stmt, bool missing_ok, bool
/* owned.c - forward declarations */
extern List * PreprocessDropOwnedStmt(Node *node, const char *queryString,
ProcessUtilityContext processUtilityContext);
extern List * PreprocessReassignOwnedStmt(Node *node, const char *queryString,
ProcessUtilityContext processUtilityContext);

/* policy.c - forward declarations */
extern List * CreatePolicyCommands(Oid relationId);
Expand Down
1 change: 1 addition & 0 deletions src/include/distributed/deparser.h
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ extern void QualifyAlterRoleSetStmt(Node *stmt);
extern char * DeparseCreateRoleStmt(Node *stmt);
extern char * DeparseDropRoleStmt(Node *stmt);
extern char * DeparseGrantRoleStmt(Node *stmt);
extern char * DeparseReassignOwnedStmt(Node *node);

/* forward declarations for deparse_owned_stmts.c */
extern char * DeparseDropOwnedStmt(Node *node);
Expand Down
93 changes: 93 additions & 0 deletions src/test/regress/expected/reassure_owned.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
CREATE ROLE role1;
CREATE ROLE role2;
GRANT CREATE ON SCHEMA public TO role1;
SET ROLE role1;
CREATE TABLE public.test_table (col1 int);
RESET ROLE;
select create_distributed_table('test_table', 'col1');
create_distributed_table
---------------------------------------------------------------------

(1 row)

SELECT result from run_command_on_all_nodes(
$$
SELECT jsonb_agg(to_jsonb(q2.*)) FROM (
SELECT
schemaname,
tablename,
tableowner
FROM
pg_tables
WHERE
tablename = 'test_table'
) q2
$$
) ORDER BY result;
result
---------------------------------------------------------------------
[{"tablename": "test_table", "schemaname": "public", "tableowner": "role1"}]
[{"tablename": "test_table", "schemaname": "public", "tableowner": "role1"}]
[{"tablename": "test_table", "schemaname": "public", "tableowner": "role1"}]
(3 rows)

set citus.log_remote_commands to on;
set citus.grep_remote_commands = '%REASSIGN OWNED BY%';
REASSIGN OWNED BY role1 TO role2;
NOTICE: issuing REASSIGN OWNED BY role1 TO role2
DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx
NOTICE: issuing REASSIGN OWNED BY role1 TO role2
DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx
SET citus.log_remote_commands = false;
SELECT result from run_command_on_all_nodes(
$$
SELECT jsonb_agg(to_jsonb(q2.*)) FROM (
SELECT
schemaname,
tablename,
tableowner
FROM
pg_tables
WHERE
tablename = 'test_table'
) q2
$$
) ORDER BY result;
result
---------------------------------------------------------------------
[{"tablename": "test_table", "schemaname": "public", "tableowner": "role2"}]
[{"tablename": "test_table", "schemaname": "public", "tableowner": "role2"}]
[{"tablename": "test_table", "schemaname": "public", "tableowner": "role2"}]
(3 rows)

SET citus.log_remote_commands = true;
DROP OWNED BY role1, role2;
SET citus.log_remote_commands = false;
SELECT result from run_command_on_all_nodes(
$$
SELECT jsonb_agg(to_jsonb(q2.*)) FROM (
SELECT
schemaname,
tablename,
tableowner
FROM
pg_tables
WHERE
tablename = 'test_table'
) q2
$$
) ORDER BY result;
result
---------------------------------------------------------------------



(3 rows)

SET citus.log_remote_commands = true;
set citus.grep_remote_commands = '%DROP ROLE%';
drop role role1, role2 ;
NOTICE: issuing DROP ROLE role1, role2
DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx
NOTICE: issuing DROP ROLE role1, role2
DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx
1 change: 1 addition & 0 deletions src/test/regress/multi_1_schedule
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ test: grant_on_database_propagation
test: alter_database_propagation

test: citus_shards
test: reassure_owned

# ----------
# multi_citus_tools tests utility functions written for citus tools
Expand Down
70 changes: 70 additions & 0 deletions src/test/regress/sql/reassure_owned.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
CREATE ROLE role1;
CREATE ROLE role2;

GRANT CREATE ON SCHEMA public TO role1;

SET ROLE role1;
CREATE TABLE public.test_table (col1 int);
RESET ROLE;
select create_distributed_table('test_table', 'col1');

SELECT result from run_command_on_all_nodes(
$$
SELECT jsonb_agg(to_jsonb(q2.*)) FROM (
SELECT
schemaname,
tablename,
tableowner
FROM
pg_tables
WHERE
tablename = 'test_table'
) q2
$$
) ORDER BY result;

set citus.log_remote_commands to on;
set citus.grep_remote_commands = '%REASSIGN OWNED BY%';
REASSIGN OWNED BY role1 TO role2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Regardless of what we decide to do with the scenarios C1,C2,C3,C4,C5,C6,
    we should have at least one test for each scenario. And also we should ensure to have test cases with >1 role in the BY clause, e.g. 2 roles: REASSIGN OWNED BY role0, role1 TO role2;

  2. As always, let's also add tests where the role names have escaped characters :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests for distributed and non-distributed roles with escape characters



SET citus.log_remote_commands = false;
SELECT result from run_command_on_all_nodes(
$$
SELECT jsonb_agg(to_jsonb(q2.*)) FROM (
SELECT
schemaname,
tablename,
tableowner
FROM
pg_tables
WHERE
tablename = 'test_table'
) q2
$$
) ORDER BY result;



SET citus.log_remote_commands = true;
DROP OWNED BY role1, role2;

SET citus.log_remote_commands = false;
SELECT result from run_command_on_all_nodes(
$$
SELECT jsonb_agg(to_jsonb(q2.*)) FROM (
SELECT
schemaname,
tablename,
tableowner
FROM
pg_tables
WHERE
tablename = 'test_table'
) q2
$$
) ORDER BY result;

SET citus.log_remote_commands = true;
set citus.grep_remote_commands = '%DROP ROLE%';
drop role role1, role2 ;