Adds REASSIGN OWNED BY propagation #7319

gurkanindibay · 2023-11-02T08:16:15Z

DESCRIPTION: Adds REASSIGN OWNED BY propagation

This pull request introduces the propagation of the "Reassign owned by" statement. It accommodates both local and distributed roles for both the old and new assignments. However, when the old role is a local role, it undergoes filtering and is not propagated. On the other hand, if the new role is a local role, the process involves first creating the role on worker nodes before propagating the "Reassign owned" statement.

codecov · 2023-11-02T08:25:09Z

Codecov Report

Merging #7319 (29e77ed) into main (181b8ab) will decrease coverage by 6.43%.
The diff coverage is 94.66%.

❗ Current head 29e77ed differs from pull request most recent head f49d429. Consider uploading reports for the commit f49d429 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7319      +/-   ##
==========================================
- Coverage   89.61%   83.19%   -6.43%     
==========================================
  Files         278      350      +72     
  Lines       60129    62749    +2620     
  Branches     7490     8131     +641     
==========================================
- Hits        53882    52201    -1681     
- Misses       4104     8170    +4066     
- Partials     2143     2378     +235

onurctirtir

REASSIGN	TO non-distributed role	TO distributed role
OWNED BY all-non-distributed roles	C1	C2
OWNED BY some-distributed roles	C3	C4
OWNED BY all-distributed roles	C5	C6

Going through all the possible scenarios, it's obvious that while Citus should not return any DDLJobs for C1, it needs to return a DDLJob for C6.

However, I'm confused about the other scenarios, namely, C2, C3, C4 and C5 i.e., the cases where DDL both references non-distributed roles as well as distributed ones, either in OWNED BY clause or in TO clause.

As an example, for C4, would it really make sense to only reassign the objects owned by distributed ones to the one given in TO clause, or would it be confusing on user's end? We have two options here; either do what this PR proposes (filter the distributed roles in provided in OWNED BY claue and forget about the others), or automatically propagate non-distributed roles before sending REASSIGN OWNED BY command to workers.

I think choosing the first option would cause role objects to get out-of-sync on different nodes so might not be the ideal approach, so I'd like to talk more about this in weekly sync: @gurkanindibay, @JelteF.

Or, for C5, I'd expect the solution proposed in this PR to fail as it would incorrectly search for the role provided in TO clause on worker nodes. Do we have any tests for that?

src/backend/distributed/commands/owned.c

onurctirtir · 2023-11-06T18:53:31Z

src/backend/distributed/commands/owned.c

+
+
+List *
+PreprocessReassignOwnedStmt(Node *node, const char *queryString,


Should we have this in Postprocess instead?

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.

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

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)

onurctirtir · 2023-11-10T11:10:15Z

REASSIGN TO non-distributed role TO distributed role
OWNED BY all-non-distributed roles C1 C2
OWNED BY some-distributed roles C3 C4
OWNED BY all-distributed roles C5 C6
Going through all the possible scenarios, it's obvious that while Citus should not return any DDLJobs for C1, it needs to return a DDLJob for C6.

However, I'm confused about the other scenarios, namely, C2, C3, C4 and C5 i.e., the cases where DDL both references non-distributed roles as well as distributed ones, either in OWNED BY clause or in TO clause.

As an example, for C4, would it really make sense to only reassign the objects owned by distributed ones to the one given in TO clause, or would it be confusing on user's end? We have two options here; either do what this PR proposes (filter the distributed roles in provided in OWNED BY claue and forget about the others), or automatically propagate non-distributed roles before sending REASSIGN OWNED BY command to workers.

I think choosing the first option would cause role objects to get out-of-sync on different nodes so might not be the ideal approach, so I'd like to talk more about this in weekly sync: @gurkanindibay, @JelteF.

Or, for C5, I'd expect the solution proposed in this PR to fail as it would incorrectly search for the role provided in TO clause on worker nodes. Do we have any tests for that?

As we've discussed earlier this week, let's not filter out local roles but rely on workers to throw an error if the role doesn't exist on a worker node.

onurctirtir · 2023-11-20T17:13:42Z

src/backend/distributed/commands/owned.c

+	ReassignOwnedStmt *stmt = castNode(ReassignOwnedStmt, node);
+	List *allReassignRoles = stmt->roles;
+
+	List *distributedReassignRoles = FilterDistributedRoles(allReassignRoles);


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

We decided to keep the filtering logic

onurctirtir · 2023-11-20T17:13:53Z

src/backend/distributed/commands/owned.c

+
+
+List *
+PreprocessReassignOwnedStmt(Node *node, const char *queryString,


need to move this down to Postprocess phase

…xistOnAllNodes

naisila

Since we haven't still decided yet on the logic, I only added some comments for the tests. Thank you.

naisila · 2023-11-23T10:10:31Z

src/test/regress/sql/reassure_owned.sql

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


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;

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

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

gurkanindibay · 2023-11-27T06:37:40Z

naisila

Looking at the changes, I realize that it is much better to open a PR against this branch for the changed EnsureDependencies function. It was hard to follow in this PR. I already left comments here, but can you consider that? Especially if we will ask for an extra review from Nils or someone else.

src/backend/distributed/commands/dependencies.c

naisila · 2023-11-27T11:21:58Z

src/backend/distributed/commands/dependencies.c


 static void EnsureDependenciesCanBeDistributed(const ObjectAddress *relationAddress);
 static void ErrorIfCircularDependencyExists(const ObjectAddress *objectAddress);
 static int ObjectAddressComparator(const void *a, const void *b);
 static void EnsureDependenciesExistOnAllNodes(const ObjectAddress *target);
+static void EnsureRequiredObjectExistOnAllNodes(const ObjectAddress *target,


Naming suggestion: Let's use the name of the struct RequiredObjectSet inside the name of this function

Suggested change

static void EnsureRequiredObjectExistOnAllNodes(const ObjectAddress *target,

static void EnsureRequiredObjectSetExistsOnAllNodes(const ObjectAddress *target,

@onurctirtir what's your opinion?

the suggestion makes sense to me

naisila · 2023-11-27T11:30:13Z

src/backend/distributed/commands/dependencies.c

-	List *dependenciesWithCommands = NIL;
+	Assert(requiredObjectSet == REQUIRE_ONLY_DEPENDENCIES ||
+		   requiredObjectSet == REQUIRE_OBJECT_AND_DEPENDENCIES);
+


Before doing anything else, here we can first check if the target object is in fact distributed: if it is we can return early here, regardless of the RequiredObjectSet

I tested to put IsAnyObjectDistributed method however got test failures so I removed

I believe lastly we agreed on adding the following check?

Suggested change

if (IsAnyObjectDistributed(list_make1((ObjectAddress *) target)))

{

return;

}

This is the test error when we use IsAnyObjectDistributed(list_make1((ObjectAddress *) target))
https://github.com/citusdata/citus/actions/runs/7321494377

naisila · 2023-11-27T11:37:15Z

src/backend/distributed/commands/owned.c

+	int saveNestLevel = NewGUCNestLevel();
+	set_config_option("citus.enable_create_role_propagation", "on",
+					  (superuser() ? PGC_SUSET : PGC_USERSET), PGC_S_SESSION,
+					  GUC_ACTION_LOCAL, true, 0, false);


We never do anything when calling EnsureDependencies function, and sometimes we may need to propagate roles in that function. Whatever we did before, the same can be applied here. So, I think we don't need these lines.

This block is required to set the GUC into previous state. I've used it in citus_internal_database_command

citus/src/backend/distributed/metadata/metadata_sync.c

Line 4009 in 18a8783

int saveNestLevel = NewGUCNestLevel();

src/backend/distributed/commands/dependencies.c

src/backend/distributed/commands/distribute_object_ops.c

onurctirtir · 2023-12-25T09:40:45Z

src/backend/distributed/deparser/deparse_owned_stmts.c

+	appendStringInfo(buf, "REASSIGN OWNED BY ");
+
+	AppendRoleList(buf, stmt->roles);
+	char const *newRoleName = RoleSpecString(stmt->newrole, true);


minor;

Suggested change

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

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

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);

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

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

src/backend/distributed/deparser/deparse_owned_stmts.c

src/backend/distributed/commands/owned.c

onurctirtir · 2023-12-25T10:40:43Z

src/backend/distributed/commands/dependencies.c

-	List *dependenciesWithCommands = NIL;
+	Assert(requiredObjectSet == REQUIRE_ONLY_DEPENDENCIES ||
+		   requiredObjectSet == REQUIRE_OBJECT_AND_DEPENDENCIES);
+


I believe lastly we agreed on adding the following check?

Suggested change

if (IsAnyObjectDistributed(list_make1((ObjectAddress *) target)))

{

return;

}

src/test/regress/sql/reassign_owned.sql

onurctirtir · 2023-12-25T10:54:05Z

(TODO for myself: Test the PR manually once #7319 (comment) is resolved.)

onurctirtir

Looks very good to me, please consider those last two minor comments too, thanks!

onurctirtir · 2023-12-28T08:41:50Z

src/test/regress/expected/reassign_owned.out

+set citus.enable_create_role_propagation to off;
+set citus.enable_alter_role_propagation to off;
+set citus.enable_alter_role_set_propagation to off;


awesome test!

onurctirtir · 2023-12-28T08:44:08Z

src/test/regress/sql/reassign_owned.sql

+show citus.enable_create_role_propagation;
+show citus.enable_alter_role_propagation;
+show citus.enable_alter_role_set_propagation;


show stmts are not needed

I'm rollbacking GUC state to previous one so I wanted to test GUC state if it is rollbacked

oh that really makes sense

onurctirtir · 2023-12-28T08:51:36Z

src/backend/distributed/deparser/deparse_owned_stmts.c

+	appendStringInfo(buf, "REASSIGN OWNED BY ");
+
+	AppendRoleList(buf, stmt->roles);
+	char const *newRoleName = RoleSpecString(stmt->newrole, true);


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

gurkanindibay added 2 commits November 1, 2023 01:46

Adds REASSIGN OWNED BY propagation

d6ecac4

Merge branch 'main' into reassign_owned_prop

aff3f3f

onurctirtir self-requested a review November 6, 2023 18:52

onurctirtir requested changes Nov 6, 2023

View reviewed changes

gurkanindibay added 4 commits November 10, 2023 14:23

Merge branch 'main' into reassign_owned_prop

6426087

Fixes affected test

76e5de9

Fixes failing test

da900cf

Merge branch 'main' into reassign_owned_prop

557dd71

onurctirtir reviewed Nov 20, 2023

View reviewed changes

onurctirtir requested a review from naisila November 21, 2023 12:46

gurkanindibay added 4 commits November 22, 2023 12:13

Fixes review notes

f342793

Refactor EnsureDependenciesExistOnAllNodes to introduce EnsureObjectE…

f255643

…xistOnAllNodes

Adds dependencies

f1f7313

Fixes indentation

b598b48

naisila reviewed Nov 23, 2023

View reviewed changes

gurkanindibay added 5 commits November 26, 2023 16:51

Merge remote-tracking branch 'origin/main' into reassign_owned_prop

e39c15f

Fixes role propagation issue

aed2035

Adds escape chars for a role

c7a98ac

Fixes test flakyness

2fec02c

Fixes flakiness of tests

7632e5d

gurkanindibay requested review from naisila and onurctirtir November 27, 2023 07:03

naisila reviewed Nov 27, 2023

View reviewed changes

gurkanindibay added 3 commits November 28, 2023 09:19

Sets role flags

3600f22

Adds distributed check for new dependency function

7433b79

Fixes indentation

4ac5d0d

onurctirtir mentioned this pull request Nov 29, 2023

DDL Support #4812

Open

57 tasks

halilozanakgul reviewed Dec 14, 2023

View reviewed changes

src/backend/distributed/commands/dependencies.c Outdated Show resolved Hide resolved

gurkanindibay added 6 commits December 17, 2023 09:19

Fixes build error

9f82b16

Removes dependency check

6adac3a

Fixes method comments

e825d29

Fixes indentation

b5b6ecd

Merge branch 'main' into reassign_owned_prop

c199c01

Fixes test error

059081e

onurctirtir reviewed Dec 25, 2023

View reviewed changes

src/test/regress/sql/reassign_owned.sql Show resolved Hide resolved

onurctirtir reviewed Dec 25, 2023

View reviewed changes

gurkanindibay added 5 commits December 25, 2023 17:03

Fixes review comments

e01d85b

Adds distributed check to dependency resolution

b62a958

Fixes the flakiness of tests

9314ac8

Renames test output

80b90d6

Fixes test errors

f2ebc68

gurkanindibay requested a review from onurctirtir December 26, 2023 08:32

gurkanindibay added 3 commits December 26, 2023 12:43

Fixes review issues

f49d429

Merge branch 'main' into reassign_owned_prop

29e77ed

Fixes test output

5aa542e

onurctirtir approved these changes Dec 28, 2023

View reviewed changes

gurkanindibay merged commit c3579ee into main Dec 28, 2023

gurkanindibay deleted the reassign_owned_prop branch December 28, 2023 12:16



		List *
		PreprocessReassignOwnedStmt(Node node, const char queryString,

	static void EnsureRequiredObjectExistOnAllNodes(const ObjectAddress *target,
	static void EnsureRequiredObjectSetExistsOnAllNodes(const ObjectAddress *target,

+	if (IsAnyObjectDistributed(list_make1((ObjectAddress *) target)))
+	{
+		return;
+	}

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

Adds REASSIGN OWNED BY propagation #7319

Adds REASSIGN OWNED BY propagation #7319

Conversation

gurkanindibay commented Nov 2, 2023 • edited Loading

codecov bot commented Nov 2, 2023 • edited Loading

Codecov Report

onurctirtir left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurctirtir commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naisila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gurkanindibay commented Nov 27, 2023 • edited Loading

Pre Review Request Changes

naisila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gurkanindibay Dec 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurctirtir Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurctirtir commented Dec 25, 2023 • edited Loading

onurctirtir left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurctirtir Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

gurkanindibay commented Nov 2, 2023 •

edited

Loading

codecov bot commented Nov 2, 2023 •

edited

Loading

onurctirtir left a comment •

edited

Loading

gurkanindibay commented Nov 27, 2023 •

edited

Loading

gurkanindibay Dec 25, 2023 •

edited

Loading

onurctirtir Dec 28, 2023 •

edited

Loading

onurctirtir commented Dec 25, 2023 •

edited

Loading

onurctirtir left a comment •

edited

Loading

onurctirtir Dec 28, 2023 •

edited

Loading