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

Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions contrib/babelfishpg_tsql/src/catalog.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "catalog/catalog.h"
#include "catalog/heap.h"
#include "catalog/indexing.h"
#include "catalog/pg_auth_members.h"
#include "catalog/pg_namespace.h"
#include "catalog/pg_authid.h"
#include "catalog/pg_proc.h"
Expand Down Expand Up @@ -6287,3 +6288,49 @@ get_tvp_typename_typeschemaname(char *proc_name, char *target_arg_name, char **t
if(!xactStarted)
CommitTransactionCommand();
}

/*
* Checks if the given member is 'bbf_role_admin' role and has the privilege
* to grant a specified role through direct membership. This privilege is
* determined by the presence of the 'admin_option' flag, which allows the
* member to grant the role to others.
*
* @return true if the member is 'bbf_role_admin' and also has the admin option
* to grant the specified role; otherwise, returns false.
*/
bool
bbf_check_member_has_direct_priv_to_grant_role(Oid member, Oid role)
anju15bharti marked this conversation as resolved.
Show resolved Hide resolved
{
CatCList *memlist;

/*
* TSQL specific behavior and member must be bbf_role_admin.
* If not return false.
*/
if (sql_dialect != SQL_DIALECT_TSQL || !IS_TDS_CONN()
|| member != get_bbf_role_admin_oid())
return false;

/*
* Find all existing tuples for given role
* whose member is bbf_role_admin
*/
memlist = SearchSysCacheList2(AUTHMEMROLEMEM,
anju15bharti marked this conversation as resolved.
Show resolved Hide resolved
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.

{
HeapTuple tup = &memlist->members[i]->tuple;
Form_pg_auth_members form = (Form_pg_auth_members) GETSTRUCT(tup);

/* Return true if admin_option is true */
if (form->admin_option)
{
ReleaseSysCacheList(memlist);
return true;
}
}
anju15bharti marked this conversation as resolved.
Show resolved Hide resolved

ReleaseSysCacheList(memlist);
return false;
}
1 change: 1 addition & 0 deletions contrib/babelfishpg_tsql/src/catalog.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ extern bool IsPLtsqlExtendedCatalog(Oid relationId);
extern bool IsPltsqlToastRelationHook(Relation relation);
extern bool IsPltsqlToastClassHook(Form_pg_class pg_class_tup);
extern void pltsql_drop_relation_refcnt_hook(Relation relation);
extern bool bbf_check_member_has_direct_priv_to_grant_role(Oid member, Oid role);

/*****************************************
* SYS schema
Expand Down
8 changes: 7 additions & 1 deletion contrib/babelfishpg_tsql/src/hooks.c
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ static pltsql_replace_non_determinstic_hook_type prev_pltsql_replace_non_determi
static pltsql_is_partitioned_table_reloptions_allowed_hook_type prev_pltsql_is_partitioned_table_reloptions_allowed_hook = NULL;
static ExecFuncProc_AclCheck_hook_type prev_ExecFuncProc_AclCheck_hook = NULL;
static bbf_execute_grantstmt_as_dbsecadmin_hook_type prev_bbf_execute_grantstmt_as_dbsecadmin_hook = NULL;
static bbf_check_member_has_direct_priv_to_grant_role_hook_type prev_bbf_check_member_has_direct_priv_to_grant_role_hook = NULL;

/*****************************************
* Install / Uninstall
Expand Down Expand Up @@ -522,6 +523,9 @@ InstallExtendedHooks(void)

prev_bbf_execute_grantstmt_as_dbsecadmin_hook = bbf_execute_grantstmt_as_dbsecadmin_hook;
bbf_execute_grantstmt_as_dbsecadmin_hook = handle_grantstmt_for_dbsecadmin;

prev_bbf_check_member_has_direct_priv_to_grant_role_hook = bbf_check_member_has_direct_priv_to_grant_role_hook;
bbf_check_member_has_direct_priv_to_grant_role_hook = bbf_check_member_has_direct_priv_to_grant_role;

pltsql_get_object_identity_event_trigger_hook = pltsql_get_object_identity_event_trigger;

Expand Down Expand Up @@ -600,6 +604,8 @@ UninstallExtendedHooks(void)
pltsql_is_partitioned_table_reloptions_allowed_hook = prev_pltsql_is_partitioned_table_reloptions_allowed_hook;
ExecFuncProc_AclCheck_hook = prev_ExecFuncProc_AclCheck_hook;
bbf_execute_grantstmt_as_dbsecadmin_hook = prev_bbf_execute_grantstmt_as_dbsecadmin_hook;
bbf_check_member_has_direct_priv_to_grant_role_hook = prev_bbf_check_member_has_direct_priv_to_grant_role_hook;


bbf_InitializeParallelDSM_hook = NULL;
bbf_ParallelWorkerMain_hook = NULL;
Expand Down Expand Up @@ -6031,4 +6037,4 @@ remove_db_name_in_schema(const char *object_name, const char *object_type)
pfree(splited_object_name);

return (const char *)pstrdup(object_name + prefix_len);
}
}
9 changes: 9 additions & 0 deletions test/JDBC/expected/db_securityadmin-vu-verify.out
Original file line number Diff line number Diff line change
Expand Up @@ -1312,6 +1312,15 @@ t
~~END~~


-- Wait to sync with another session
anju15bharti marked this conversation as resolved.
Show resolved Hide resolved
SELECT pg_sleep(1);
go
~~START~~
void

~~END~~


-- tsql
DROP LOGIN db_securityadmin_restrictions_login;
GO
1 change: 0 additions & 1 deletion test/JDBC/expected/test_windows_login-vu-prepare.out
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
-- psql
-- TODO: BABEL-5349
do
$$ begin
if not exists (select * from pg_catalog.pg_roles where rolname = 'rds_ad')
Expand Down
91 changes: 91 additions & 0 deletions test/JDBC/expected/test_windows_login_before-17_3-vu-cleanup.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
-- tsql
DROP LOGIN PassWordUser;
GO

-- test for drop login with upn format
DROP LOGIN [aduser@AD];
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the login 'aduser@ad', because it does not exist or you do not have permission.)~~


DROP LOGIN [ad\adUSer];
GO

DROP LOGIN [ad\aduserDB];
GO

DROP LOGIN [ad\Aduserlanguage];
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the login 'aduserlanguage@AD', because it does not exist or you do not have permission.)~~


DROP LOGIN [ad\Aduserdblanguage];
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the login 'aduserdblanguage@AD', because it does not exist or you do not have permission.)~~


DROP LOGIN [ad\AduserdbEnglish];
GO

DROP LOGIN [ad\Aduseroption];
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the login 'aduseroption@AD', because it does not exist or you do not have permission.)~~


DROP LOGIN [babel\aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the login 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa@BABEL', because it does not exist or you do not have permission.)~~


DROP LOGIN [ba.bel\username];
GO

-- test for non-existent login
DROP LOGIN [babel\non_existent_login]
GO
~~ERROR (Code: 33557097)~~

~~ERROR (Message: Cannot drop the login 'non_existent_login@BABEL', because it does not exist or you do not have permission.)~~


-- drop login with different casing
DROP LOGIN UserPassword;
GO

DROP LOGIN [BabeL\DupLicate];
GO

DROP LOGIN [Babel\DuplicateDefaultDB];
GO

-- test drop logins for logins with different language names
-- Arabic
DROP LOGIN [babel\كلب];
GO

-- Mongolian
DROP LOGIN [babel\өглөө];
GO

-- Greek
DROP LOGIN [babel\ελπίδα];
GO

-- Chinese
DROP LOGIN [babel\爱];
GO

DROP DATABASE ad_db;
GO


Loading