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

Performance Improvement of create login in BBF #3291

Conversation

anju15bharti
Copy link
Contributor

@anju15bharti anju15bharti commented Dec 20, 2024

Description

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.
Without the changes the time taken to create a login increases drastically and linearly:

Flame graph :
kernel

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 :
perf-kernel-new-hook

The average time consumed by create login stmt(with 125 dbs and 125 logins present) : 49 ms

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is under the terms of the Apache 2.0 and PostgreSQL licenses, 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.

@coveralls
Copy link
Collaborator

coveralls commented Dec 20, 2024

Pull Request Test Coverage Report for Build 12581937147

Details

  • 13 of 16 (81.25%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.002%) to 74.867%

Changes Missing Coverage Covered Lines Changed/Added Lines %
contrib/babelfishpg_tsql/src/hooks.c 2 3 66.67%
contrib/babelfishpg_tsql/src/catalog.c 11 13 84.62%
Files with Coverage Reduction New Missed Lines %
contrib/babelfishpg_tds/src/backend/tds/tdscomm.c 2 76.03%
Totals Coverage Status
Change from base Build 12556954627: 0.002%
Covered Lines: 46612
Relevant Lines: 62260

💛 - Coveralls

@@ -1,5 +1,4 @@
-- sla 180000
-- TODO: BABEL-5349
-- sla 55000
Copy link
Contributor

Choose a reason for hiding this comment

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

let us also merge this fix in 17.2 & 16.7. This will help us skip creating before and after files for this test case.

Copy link
Contributor Author

@anju15bharti anju15bharti Jan 2, 2025

Choose a reason for hiding this comment

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

We will still need to create before file for version before 16.7 as it was failing for 16.1 and some other versions as well.
This commit can be cherry-picked into 16.7 and 17.2 branch as well.

memlist = SearchSysCacheList2(AUTHMEMROLEMEM,
ObjectIdGetDatum(role),
ObjectIdGetDatum(member));
for (int i = 0; i < memlist->n_members; i++)
Copy link
Contributor

Choose a reason for hiding this comment

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

let us not assume that memlist will be non-null, we should check for it and fallback to PG in case of null

Copy link
Contributor Author

@anju15bharti anju15bharti Jan 6, 2025

Choose a reason for hiding this comment

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

SearchCatCacheList inside SearchSysCacheList2 ensures that memlist value should not be null as it allocates memory to memlist even if no entry matches in the search and assign memlist->n_members as 0.
And when memlist->n_members will be 0, this API will return false falling back to PG API.

jsudrik pushed a commit to babelfish-for-postgresql/postgresql_modified_for_babelfish that referenced this pull request Jan 6, 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.

Issues Resolved
BABEL-5349
@jsudrik jsudrik merged commit 4602ff1 into babelfish-for-postgresql:BABEL_5_X_DEV Jan 6, 2025
45 checks passed
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 to babelfish-for-postgresql/postgresql_modified_for_babelfish 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 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants