PG16 - Don't propagate GRANT ROLE with INHERIT/SET option #7190
Conversation
naisila
changed the title
naisila
force-pushed
the
naisila/error_grant
branch
from
f10a97f
to
4975112
Compare
src/test/regress/pg_regress_multi.pl
Outdated
| @@ -509,6 +509,9 @@ sub generate_hba | |||
|
|
|||
| # we disable some restrictions for local objects like local views to not break postgres vanilla test behaviour. | |||
| push(@pgOptions, "citus.enforce_object_restrictions_for_local_objects=false"); | |||
|
|
|||
| # we disable propagating create role statements to not have roles as distributed objects during vanilla tests. | |||
| push(@pgOptions, "citus.enable_create_role_propagation=false"); | |||
There was a problem hiding this comment.
This feels unexpected given ErrorIfGrantRoleWithInheritOrSetOption is called after FilterDistributedRoles ?
There was a problem hiding this comment.
Hmm, ok vanilla tests runs with distributed roles, let me think a bit
There was a problem hiding this comment.
I think using enable_create_role_propagation = false might hide some actual bugs, so I'm really not sure if we should pass it.
Instead, I'd rather change ErrorIfGrantRoleWithInheritOrSetOption to WarnfGrantRoleWithInheritOrSetOption and rely on EnableUnsupportedFeatureMessages.
And, make sure to add HINT to WarnfGrantRoleWithInheritOrSetOption such that remind the user should run the same command on the workers
There was a problem hiding this comment.
Okay, sounds good, let me do that.
I will also extend the PR with some REVOKE tests.
There was a problem hiding this comment.
It turns out that we cannot run GRANT ROLE statements from the worker nodes. So, I will leave it as erroring out for now unless you have a different idea on this?
There was a problem hiding this comment.
you cannot run any DDL from the workers unless you set citus.enable_ddl_propagation to off ? So, that sounds expected to me
There was a problem hiding this comment.
Right, sorry about that. I confused the logic for a moment there. Let me change the PR
Codecov Report
@@ Coverage Diff @@
## main #7190 +/- ##
==========================================
- Coverage 93.22% 93.21% -0.01%
==========================================
Files 274 274
Lines 59376 59386 +10
==========================================
+ Hits 55352 55359 +7
- Misses 4024 4027 +3 |
naisila
force-pushed
the
naisila/error_grant
branch
from
4975112
to
704ce97
Compare
We currently don't support propagating these options in Citus Relevant PG commits: postgres/postgres@e3ce2de postgres/postgres@3d14e17
naisila
force-pushed
the
naisila/error_grant
branch
from
704ce97
to
65e4a4f
Compare
naisila
changed the title
We currently don't support propagating these options in Citus Relevant PG commits: postgres/postgres@e3ce2de postgres/postgres@3d14e17 Limitation: We also need to take care of generated GRANT statements by dependencies in attempt to distribute something else. Specifically, this part of the code in `GenerateGrantRoleStmtsOfRole`: ``` grantRoleStmt->admin_opt = membership->admin_option; ``` In PG16, membership also has `inherit_option` and `set_option` which need to properly be part of the `grantRoleStmt`. We can skip for now since #7164 will take care of this soon, and also this is not an expected use-case.
We currently don't support propagating these options in Citus
Relevant PG commits:
postgres/postgres@e3ce2de
postgres/postgres@3d14e17
Limitation:
We also need to take care of generated GRANT statements by dependencies in attempt to distribute something else. Specifically, this part of the code in
GenerateGrantRoleStmtsOfRole:In PG16, membership also has
inherit_optionandset_optionwhich need to properly be part of thegrantRoleStmt. We can skip for now since #7164 will take care of this soon, and also this is not an expected use-case.#7138