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

Implement DROP OWNED BY propagation #6519

Merged
merged 3 commits into from
Dec 2, 2022

Conversation

TsinghuaLucky912
Copy link
Contributor

@TsinghuaLucky912 TsinghuaLucky912 commented Nov 24, 2022

DESCRIPTION: Fixes a bug that caused DROP OWNED BY to not drop schemas on workers

Dear developers, I have submitted a version of the code to solve the problem that the DROP OWNED BY command is not delivered to the worker node, which causes the failure to create objects such as schemas again.

  1. link: DROP OWNED BY fails to drop the schemas on the workers #5763
  2. fix the problem DROP OWNED BY fails to drop the schemas on the workers #5763 DROP OWNED BY fails to drop the schemas on the workers

Fixes #5763

@TsinghuaLucky912
Copy link
Contributor Author

If this is allowed, I will ban the following content in the regression test. I want to know what everyone thinks of this problem?

SELECT run_command_on_workers($$DROP OWNED BY read_access$$);
                        run_command_on_workers                        
 ---------------------------------------------------------------------
- (localhost,57637,t,"DROP OWNED")
- (localhost,57638,t,"DROP OWNED")
+ (localhost,57637,f,"ERROR:  operation is not allowed on this node")
+ (localhost,57638,f,"ERROR:  operation is not allowed on this node")
 (2 rows)

image

Copy link
Member

@onderkalaci onderkalaci left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

 DROP SCHEMA single_node_ent CASCADE;
 DROP OWNED BY full_access_single_node;
 DROP OWNED BY read_access_single_node;
 SELECT run_command_on_workers($$DROP OWNED BY full_access_single_node$$);
                        run_command_on_workers                        
 ---------------------------------------------------------------------
- (localhost,57637,t,"DROP OWNED")
+ (localhost,57637,f,"ERROR:  operation is not allowed on this node")
 (1 row)

I think you should also adjust the existing regression tests NOT to have SELECT run_command_on_workers($$DROP OWNED BY XXX$$); anymore

{
rolename = RoleSpecString((RoleSpec *) roleNode, true);
}
if (IsA(roleNode, AccessPriv))
Copy link
Member

Choose a reason for hiding this comment

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

src/backend/distributed/commands/owned.c Show resolved Hide resolved
src/backend/distributed/commands/owned.c Show resolved Hide resolved
@TsinghuaLucky912
Copy link
Contributor Author

@onderkalaci Thank you very much for your review, which is very helpful to me

@TsinghuaLucky912
Copy link
Contributor Author

@onderkalaci

Hi, I would like to ask this ci/circleci: check-style — Your tests failed on CircleCI regression testing question:

FAIL: src/include/distributed/deparser.h (File size changed from 10821 to 10822)
FAIL: src/include/distributed/commands.h (File size changed from 37021 to 37033)
FAIL: src/backend/distributed/commands/owned.c (File size changed from 2503 to 2501)

This is the first time I submit code for citus, not sure what this means and how to solve it?

I took a look at the actual size of the file, but couldn't see why:

image

@SaitTalhaNisanci
Copy link
Contributor

It means your code doesn't comply with the style standards of the project, you can follow this to fix it: https://github.com/citusdata/citus/blob/main/CONTRIBUTING.md#following-our-coding-conventions

@TsinghuaLucky912
Copy link
Contributor Author

@SaitTalhaNisanci @onderkalaci Thank you so much for your documentation help, I performed a
Make reindent can be, CI has all passed Thank you again for your help!

@TsinghuaLucky912
Copy link
Contributor Author

@SaitTalhaNisanci @marcocitus @onderkalaci Hello, I just updated my branch (update main) and caused some regression tests to fail, but I found no exceptions on the local environment (pg15), can you help me start CI again? thank you very much!

image

@marcocitus marcocitus merged commit ad6450b into citusdata:main Dec 2, 2022
@marcocitus marcocitus changed the title fix the problem #5763 Implement DROP OWNED BY propagation Dec 2, 2022
@TsinghuaLucky912 TsinghuaLucky912 deleted the DropOwnedStmt branch December 2, 2022 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DROP OWNED BY fails to drop the schemas on the workers
4 participants