PG16 update GRANT... ADMIN | INHERIT | SET, and REVOKE

[Jodi-Ann Francis (francisjodi)]

Allowing GRANT ADMIN to now also be INHERIT or SET in support of psql16

GRANT role_name [, ...] TO role_specification [, ...] [ WITH { ADMIN | INHERIT | SET } { OPTION | TRUE | FALSE } ] [ GRANTED BY role_specification ]

Fixes: #7148
Related: #7138

See review changes from #7164

[Naisila Puka (naisila)]

Thanks for your PR. Please make sure Citus builds before pushing changes. Your general idea is correct, but the implementation logic is not complete code-wise. Note that since the last time you worked on a prototype of this command, we have pushed changes #7190 This PR also has many example tests for you to try.

[codecov]

Codecov Report

Merging #7344 (9437d93) into main (dbdde11) will increase coverage by 0.00%.
The diff coverage is 96.29%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7344   +/-   ##
=======================================
  Coverage   89.57%   89.57%           
=======================================
  Files         278      278           
  Lines       59974    59985   +11     
  Branches     7469     7469           
=======================================
+ Hits        53720    53732   +12     
  Misses       4106     4106           
+ Partials     2148     2147    -1     

[Onur Tirtir (onurctirtir)]

Apparently, this PR needs some regression tests.

Also, is there a reason to open a new PR rather than continuing with the existing one?

[Jodi-Ann Francis (francisjodi)]

Apparently, this PR needs some regression tests.

Also, is there a reason to open a new PR rather than continuing with the existing one?

We had multiple merge conflicts on the existing one. We closed that PR and referenced it here

[Naisila Puka (naisila)]

Regression tests are needed, please consider modifying the currently existing tests in pg16.sql
https://github.com/citusdata/citus/blob/main/src/test/regress/sql/pg16.sql
After modifying the test, add the expected output in
https://github.com/citusdata/citus/blob/main/src/test/regress/expected/pg16.out

The implementation now allows Citus to build, but the logic is still not correct, causing the current test failures. See the summary to guide you: https://github.com/citusdata/citus/actions/runs/6870630371?pr=7344
For example, a failure is in https://github.com/citusdata/citus/actions/runs/6870630371/attempts/1#summary-18686009347

[Naisila Puka (naisila)]

https://github.com/citusdata/citus/blob/pg16_grant_inherit/src/test/regress/sql/pg16.sql#L606-L612
We should remove current lines 606 - 612 from pg16.sql test - they were intended for the previously not propagated inherit/set options:

SET citus.enable_ddl_propagation TO off;
GRANT role1 TO role2 WITH ADMIN OPTION, INHERIT FALSE, SET FALSE;
RESET citus.enable_ddl_propagation;

SELECT roleid::regrole::text AS role, member::regrole::text,
admin_option, inherit_option, set_option FROM pg_auth_members
WHERE roleid::regrole::text = 'role1' ORDER BY 1, 2;

[Naisila Puka (naisila)]

https://github.com/citusdata/citus/blob/pg16_grant_inherit/src/test/regress/sql/pg16.sql#L617-L622
In the part under the comment "test REVOKES as well", we should check pg_auth_members in both coordinator and worker.

-- test REVOKES as well
GRANT role1 TO role2;
REVOKE SET OPTION FOR role1 FROM role2;
REVOKE INHERIT OPTION FOR role1 FROM role2;

SELECT roleid::regrole::text AS role, member::regrole::text,
admin_option, inherit_option, set_option FROM pg_auth_members
WHERE roleid::regrole::text = 'role1' ORDER BY 1, 2;

\c - - - :worker_1_port
SELECT roleid::regrole::text AS role, member::regrole::text,
admin_option, inherit_option, set_option FROM pg_auth_members
WHERE roleid::regrole::text = 'role1' ORDER BY 1, 2;

\c - - - :master_port
DROP ROLE role1, role2;

[Naisila Puka (naisila)]

Thank you for your work, your PR is close to merging.
I have a concern for the pg16.sql test file: we keep going back and forth between the worker and the coordinator. Let's do something like this:
First, create all the roles we are planning to use, then run all the commands with examples, and at the end we check pg_auth_members in both the coordinator and the worker. Let's also keep all the comments.
Something like this:

-- let's grep GRANT remote commands
RESET citus.log_remote_commands;
RESET citus.grep_remote_commands;

-- first create all the roles we will use in our examples
CREATE ROLE role6;
CREATE ROLE role7;
CREATE ROLE role9;
CREATE ROLE role10;
...

-- then run all the commands with appropriate comments

-- Grant role with admin and inherit options set to true
GRANT role6 TO role7 WITH ADMIN OPTION, INHERIT TRUE;
-- Grant role with admin option set to true and inherit option set to false
GRANT role9 TO role10 WITH ADMIN OPTION, INHERIT FALSE;
......

-- at the end, we do
SELECT roleid::regrole::text AS role, member::regrole::text, admin_option, inherit_option, set_option
FROM pg_auth_members ORDER BY 1, 2;
\c - - - :worker_1_port
SELECT roleid::regrole::text AS role, member::regrole::text, admin_option, inherit_option, set_option
FROM pg_auth_members ORDER BY 1, 2;

[Naisila Puka (naisila)]

Note that in PreprocessGrantRoleStmt, we EnsurePropagationToCoordinator() - this means the grant command can be run from the workers as well if the coordinator is added to the metadata. You should also add a test that runs the grant command from the worker node after adding the coordinator to the metadata through SELECT citus_set_coordinator_host('localhost', :master_port);

[Naisila Puka (naisila)]

Almost ready to merge. The test file still needs polishing for it to be understandable. The main comment I have is: let's use grep_remote_commands instead of going back and forth between the coordinator and the worker node.

[Naisila Puka (naisila)]

No need for these lines, right? Why are you setting coordinator host?

[Jodi-Ann Francis (francisjodi)]

re this: #7344 (comment)

[Jodi-Ann Francis (francisjodi)]

Onur Tirtir (@onurctirtir) thoughts?

[Hanefi Onaldi (hanefi)]

Jodi-Ann Francis (@francisjodi) that comment you shared (#7344 (comment)) is quite nice. We actually have a portion of the suggestion in our tests now.

How about we run a small portion of GRANT and REVOKE commands on the worker node, and verify that it is propagated to all the nodes?

[Hanefi Onaldi (hanefi)]

I tested the changes here and they are solid.

However, tests may be improved some more to cover the following:

• verifying that the revoke commands are propagated.
• verifying that we can run the GRANT/REVOKE commands on the worker nodes.
• increasing log verbosity in some places by settings some gucs and printing relevant propagated queries.

I do not really like that we need to go back and forth between different nodes in this tests. However there is no easy way to remove that, so that is totally acceptable.

[Hanefi Onaldi (hanefi)]

Jodi-Ann Francis (@francisjodi) that comment you shared (#7344 (comment)) is quite nice. We actually have a portion of the suggestion in our tests now.

How about we run a small portion of GRANT and REVOKE commands on the worker node, and verify that it is propagated to all the nodes?

[Hanefi Onaldi (hanefi)]

Should we add role19 and role20 to this list to unbreak flaky tests?

Suggested change

	DROP ROLE role6, role7, role8, role9, role10, role11, role12,
	role13, role14, role15, role16, role17, role18;
	DROP ROLE role6, role7, role8, role9, role10, role11, role12,
	role13, role14, role15, role16, role17, role18, role19, role20;

Passed it over to another reviewer