Adds parameter grant/revoke propagation support #7421
Conversation
gurkanindibay
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #7421 +/- ##
==========================================
- Coverage 89.71% 89.69% -0.03%
==========================================
Files 282 284 +2
Lines 60581 60547 -34
Branches 7541 7537 -4
==========================================
- Hits 54350 54306 -44
- Misses 4076 4085 +9
- Partials 2155 2156 +1 |
| (void *) command, | ||
| ENABLE_DDL_PROPAGATION); | ||
|
|
||
| return NontransactionalNodeDDLTaskList(REMOTE_NODES, commands); |
There was a problem hiding this comment.
It is not required to be non-transactional. Changed it with NodeDDLTask
There was a problem hiding this comment.
Could you first perform a self-review on this PR because apparently there are some missing things in the PR that we've already discussed before in some other PRs: function comments, usual simplifications that can be made in code, style used in comments / name of the functions etc...
There was a problem hiding this comment.
Worked on and added the missing things that I can see
| AclResult aclresult = pg_parameter_aclcheck(parameterName, granteeOid, mode); | ||
| if (aclresult == ACLCHECK_OK) | ||
| { | ||
| char *query = DeparseTreeNode((Node *) GenerateGrantStmtForRightsWithObjectName( | ||
| OBJECT_PARAMETER_ACL, granteeOid, parameterName, | ||
| modeStr, | ||
| HasAclGrantOption(aclItem, mode))); | ||
|
|
||
| /* remove the semicolon at the end of the query since it is already */ | ||
| /* appended in metadata_sync phase */ | ||
| query[strlen(query) - 1] = '\0'; | ||
|
|
||
| *queries = lappend(*queries, query); | ||
| } |
There was a problem hiding this comment.
Especially cannot quite follow what's happening here, could you simplify GenerateGrantOnParameterFromAclItem & CheckAndAppendQuery a bit?
There was a problem hiding this comment.
I simplified and added comments renamed functions and added some additional functions.
| /* | ||
| * RemoveSemicolonFromEnd removes the semicolon at the end of the query if it exists. | ||
| */ | ||
| static void | ||
| RemoveSemicolonFromEnd(char *query) | ||
| { | ||
| /* remove the semicolon at the end of the query since it is already */ | ||
| /* appended in metadata_sync phase */ | ||
| if (query[strlen(query) - 1] == ';') | ||
| { | ||
| query[strlen(query) - 1] = '\0'; | ||
| } | ||
| } |
There was a problem hiding this comment.
it's okay to have multiple ; at the end of the query, so maybe just remove this because we're anyway experiencing this in other code-paths too
DESCRIPTION: Adds parameter grant propagation support
Below commands are supported
GRANT { { SET | ALTER SYSTEM } [, ... ] | ALL [ PRIVILEGES ] }
ON PARAMETER configuration_parameter [, ...]
TO role_specification [, ...] [ WITH GRANT OPTION ]
[ GRANTED BY role_specification ]
REVOKE [ GRANT OPTION FOR ]
{ { SET | ALTER SYSTEM } [, ...] | ALL [ PRIVILEGES ] }
ON PARAMETER configuration_parameter [, ...]
FROM role_specification [, ...]
[ GRANTED BY role_specification ]
[ CASCADE | RESTRICT ]