Performance Improvement of create login in BBF #507
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: ANJU BHARTI <[email protected]>
Merged
1 task
HarshLunagariya
suggested changes
Dec 20, 2024
src/backend/utils/adt/acl.c
Outdated
Comment on lines
5080
to
5089
| *admin_role = memberid; | ||
|
|
||
| /* | ||
| * Need not to iterate through all the members | ||
| * if admin_role is found. | ||
| */ | ||
| if (sql_dialect == SQL_DIALECT_TSQL) | ||
| { | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
I suggest to restrict this logic to some special roles with significant membership like 'sysadmin' and 'bbf_role_admin'.
Let's get more opinions from perf. experts @surendravishnoi @KushaalShroff
Signed-off-by: ANJU BHARTI <[email protected]>
tanscorpio7
suggested changes
Dec 26, 2024
added 2 commits
December 31, 2024 14:58
Signed-off-by: ANJU BHARTI <[email protected]>
tanscorpio7
suggested changes
Jan 1, 2025
Signed-off-by: ANJU BHARTI <[email protected]>
tanscorpio7
approved these changes
Jan 2, 2025
HarshLunagariya
approved these changes
Jan 2, 2025
KushaalShroff
approved these changes
Jan 2, 2025
surendravishnoi
approved these changes
Jan 6, 2025
jsudrik
merged commit 3b4b4ee
into
babelfish-for-postgresql:BABEL_5_X_DEV__PG_17_X
2 checks passed
jsudrik
pushed a commit
to babelfish-for-postgresql/babelfish_extensions
that referenced
this pull request
Jan 6, 2025
Engine PR : babelfish-for-postgresql/postgresql_modified_for_babelfish#507 In Major version 16 Babelfish introduced a new role bbf_admin_role which is a member of all the roles which created implicitly from TSQL. This increases the Load on any Grant/Revoke operations that happen since in BBF to "select the role through which it gets permission to current user to grant", is inefficient and loops over members of members to find the all members of the role even though the required role is find. This commit optimizes the selection of role with is_admin_option true for babelfish by escaping the search for any TSQL roles conditionally. Signed-off-by: ANJU BHARTI <[email protected]> Issues Resolved BABEL-5349
anju15bharti
added a commit
to amazon-aurora/postgresql_modified_for_babelfish
that referenced
this pull request
Jan 7, 2025
…sql#507) Extension PR : babelfish-for-postgresql/babelfish_extensions#3291 In Major version 16 Babelfish introduced a new role bbf_admin_role which is a member of all the roles which created implicitly from TSQL. This increases the Load on any Grant/Revoke operations that happen since in BBF to "select the role through which it gets permission to current user to grant", is inefficient and loops over members of members to find the all members of the role even though the required role is find. This commit optimizes the selection of role with is_admin_option true for babelfish by escaping the search for any TSQL roles conditionally. Issues Resolved BABEL-5349
anju15bharti
added a commit
to amazon-aurora/babelfish_extensions
that referenced
this pull request
Jan 7, 2025
…sql#3291) Engine PR : babelfish-for-postgresql/postgresql_modified_for_babelfish#507 In Major version 16 Babelfish introduced a new role bbf_admin_role which is a member of all the roles which created implicitly from TSQL. This increases the Load on any Grant/Revoke operations that happen since in BBF to "select the role through which it gets permission to current user to grant", is inefficient and loops over members of members to find the all members of the role even though the required role is find. This commit optimizes the selection of role with is_admin_option true for babelfish by escaping the search for any TSQL roles conditionally. Signed-off-by: ANJU BHARTI <[email protected]> Issues Resolved BABEL-5349
shardgupta
pushed a commit
that referenced
this pull request
Jan 7, 2025
Extension PR : babelfish-for-postgresql/babelfish_extensions#3291 In Major version 16 Babelfish introduced a new role bbf_admin_role which is a member of all the roles which created implicitly from TSQL. This increases the Load on any Grant/Revoke operations that happen since in BBF to "select the role through which it gets permission to current user to grant", is inefficient and loops over members of members to find the all members of the role even though the required role is find. This commit optimizes the selection of role with is_admin_option true for babelfish by escaping the search for any TSQL roles conditionally. Task: BABEL-5349 Signed-off-by: Anju Bharti <[email protected]>
shardgupta
pushed a commit
to babelfish-for-postgresql/babelfish_extensions
that referenced
this pull request
Jan 7, 2025
Engine PR : babelfish-for-postgresql/postgresql_modified_for_babelfish#507 In Major version 16 Babelfish introduced a new role bbf_admin_role which is a member of all the roles which created implicitly from TSQL. This increases the Load on any Grant/Revoke operations that happen since in BBF to "select the role through which it gets permission to current user to grant", is inefficient and loops over members of members to find the all members of the role even though the required role is find. This commit optimizes the selection of role with is_admin_option true for babelfish by escaping the search for any TSQL roles conditionally. Task: BABEL-5349 Signed-off-by: ANJU BHARTI <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Extension PR : babelfish-for-postgresql/babelfish_extensions#3291
In Major version 16 Babelfish introduced a new role bbf_admin_role which is a member of all the roles which created implicitly from TSQL. This increases the Load on any Grant/Revoke operations that happen since in BBF to "select the role through which it gets permission to current user to grant", is inefficient and loops over members of members to find the all members of the role even though the required role is find.
This commit optimizes the selection of role with is_admin_option true for babelfish by escaping the search for any TSQL roles conditionally.
Without the changes the time taken to create a login increases drastically and linearly:
Flame graph :
The average time consumed by create login stmt(with 125 dbs and 125 logins present) : 12836 ms
with these changes we see the time taken to create login increasing linearly but not as drastically as it was before the changes:
Flame graph :
The average time consumed by create login stmt(with 125 dbs and 125 logins present) : 49 ms
Issues Resolved
BABEL-5349
Check List
By submitting this pull request, I confirm that my contribution is under the terms of the PostgreSQL license, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.