Adding Grantor Information in sys.babelfish_schema_permissions catalog by shreyeah38 · Pull Request #73 · amazon-aurora/babelfish_extensions

shreyeah38 · 2025-06-09T10:24:27Z

Description

The following PR adds grantor information in sys.babelfish_schema_permissions catalog, addressing a current limitation in Babelfish.

The functionality is achieved by hook implementation and reversing the flow of GRANT / REVOKE statement in Babelfish. This enhancement enables users to view grantor of various permissions.

Another functionality added is the storage of database-level CONNECT permission granted to users. This PR also provides a distinction between the storage of normal permissions and permissions with grant option in sys.babelfish_schema_permissions catalog.

Part of Jira - BABEL-5690

Engine PR : amazon-aurora/postgresql_modified_for_babelfish#132

Signed-off-by: Shreya Rai shreyaxr@amazon.com

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

…ermissions

test/JDBC/input/db_accessadmin-vu-verify.mix

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

contrib/babelfishpg_tsql/src/hooks.c

contrib/babelfishpg_tsql/src/catalog.c

contrib/babelfishpg_tsql/src/pl_handler.c

test/JDBC/expected/db_owner-vu-verify.out

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

…updated primary key Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

shalinilohia50 · 2025-06-13T06:13:48Z

contrib/babelfishpg_tsql/sql/ownership.sql

  function_args TEXT COLLATE "C",
  grantor sys.NVARCHAR(128) COLLATE sys.database_default,
-  PRIMARY KEY(dbid, schema_name, object_name, grantee, object_type)
+  PRIMARY KEY(dbid, schema_name, object_name, permission, grantee, object_type, grantor)


Why is permission a primary key?

Now we store "grant" permissions and "grant with options" permissions in 2 different rows. If there's no primary key on permission if would lead to an error when all the parameters are same except the permission.

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

contrib/babelfishpg_tsql/src/catalog.c

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

anju15bharti · 2025-06-18T12:33:53Z

contrib/babelfishpg_tsql/src/catalog.c

+			new_record_bbf_schema[Anum_bbf_schema_perms_permission - 1] = Int32GetDatum(current_permission);
+			new_record_repl_bbf_schema[Anum_bbf_schema_perms_permission - 1] = true;
+
+			new_tuple = heap_modify_tuple(tuple_bbf_schema,


We are not releasing tuple_bbf_schema, Make sure to release this using ReleaseSysCache.

Also in this file, I see multiple such instances where we haven't actually released it.

…e, function or procedure Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

coveralls · 2025-06-19T02:02:51Z

Pull Request Test Coverage Report for Build 15847758092

Details

248 of 269 (92.19%) changed or added relevant lines in 4 files are covered.
28 unchanged lines in 5 files lost coverage.
Overall coverage decreased (-0.02%) to 75.434%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
contrib/babelfishpg_tsql/src/pl_exec-2.c	26	27	96.3%
contrib/babelfishpg_tsql/src/catalog.c	91	101	90.1%
contrib/babelfishpg_tsql/src/hooks.c	114	124	91.94%

Files with Coverage Reduction	New Missed Lines	%
contrib/babelfishpg_tsql/src/hooks.c	1	84.87%
contrib/babelfishpg_tsql/src/pl_handler.c	1	90.14%
contrib/babelfishpg_tsql/src/pl_funcs.c	2	19.04%
contrib/babelfishpg_tsql/src/catalog.c	11	86.1%
contrib/babelfishpg_tsql/src/pltsql_utils.c	13	90.98%

Totals
Change from base Build 15531425462:	-0.02%
Covered Lines:	48866
Relevant Lines:	64780

💛 - Coveralls

contrib/babelfishpg_tsql/src/hooks.c

Deepesh125 · 2025-06-19T16:17:31Z

contrib/babelfishpg_tsql/src/hooks.c

+				/* 
+				 * Special database roles should throw an error. 
+				 */
+				if(strcmp(grantee, PUBLIC_ROLE_NAME) != 0)
+				{	
+					throw_error_for_fixed_db_role((char *) grantee, dbname);
+				}


please explain this

Deepesh125 · 2025-06-20T06:03:24Z

contrib/babelfishpg_tsql/src/hooks.c

+						/* 
+						 * Remove all entries from normal grants 
+						 */
+						update_privileges_of_object(logical_schema, object_name, old_priv_normal_grant, grantee, OBJ_RELATION, false, grantor, false);


Blocking: There is high chances of race condition here.

shalinilohia50 · 2025-06-20T06:42:46Z

contrib/babelfishpg_tsql/src/hooks.c

+ * The following hook aims to add/remove entries into BBF schema permissions catalog
+ */
+static bool
+update_bbf_schema_permissions_catalog(AclMode privileges, bool is_grant, List *grantees,


Nit:

Please add explanatory description and comments.

Indentation

The following hook -> Function update_bbf_schema_permissions_catalog

BBF -> Babelfish [Lets not use acronym in description]

Thanks, will make the changes

contrib/babelfishpg_tsql/src/hooks.c

shalinilohia50 · 2025-06-20T07:09:03Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+	/*
+	 * If grantor ends with "_bbfobj", remove the suffix as this is an internal BBF role. 
+	 */


How can the grantor be _bbfobj for grant on schema in T-SQL?

This is the case when the schema_owner is the member of db_owner user role, this sets the grantor to be _bbfobj

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Imran-imtiaz48

This script demonstrates a well-structured and advanced implementation of database permission mapping using Common Table Expressions (CTEs), bitwise operations, and role-based access logic in a Babelfish-compatible PostgreSQL environment. The use of GRANT, CREATE OR REPLACE VIEW, and JOIN LATERAL shows a solid understanding of both SQL Server-style and PostgreSQL-style syntax. The permission_mapping CTE is thoughtfully constructed to map bit values to permission names and type codes, enhancing readability and maintainability. The main view, sys.database_permissions, effectively combines permissions from multiple object types (database, schema, object) while dynamically resolving principals and ensuring correct filtering through pg_has_role checks. One suggestion for improvement would be to modularize repeated expressions (e.g., pg_has_role(...)) into reusable components or helper functions if the environment allows. Also, ensuring consistent and thorough commenting would help future developers quickly understand the complex logic. Overall, this is a strong and comprehensive implementation of a permissions view with well-handled edge cases and system compatibility.

shreyaxr3 added 2 commits June 9, 2025 10:23

Added full support for sys.database_permissions view

0b68081

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Merge remote-tracking branch 'upstream/BABEL_5_X_DEV' into database_p…

066eca5

…ermissions

shreyeah38 changed the title ~~Added full support for sys.database_permissions view~~ Adding Grantor Information in sys.babelfish_schema_permissions Jun 9, 2025

Empty-Commit

7c7b173

shreyeah38 changed the title ~~Adding Grantor Information in sys.babelfish_schema_permissions~~ Adding Grantor Information in sys.babelfish_schema_permissions catalog Jun 10, 2025

shalinilohia50 requested changes Jun 10, 2025

View reviewed changes

test/JDBC/input/db_accessadmin-vu-verify.mix Outdated Show resolved Hide resolved

Resolved permission error while granting permission on temp tables

30168b0

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

anju15bharti requested changes Jun 10, 2025

View reviewed changes

shreyaxr3 added 5 commits June 10, 2025 13:17

Fixed single_db testfiles and created before tesfiles for a few files.

29e5b2f

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Fixed Indentation and Formatting

06f5391

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Added before input files 1_GRANT_SCHEMA

1903cb6

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Fixed db_owner test file where grantor was babelfish internal user

46a5cbe

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Updated metadata inconsistency check to exclude public user role and …

cd233ec

…updated primary key Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

shalinilohia50 reviewed Jun 13, 2025

View reviewed changes

shreyaxr3 added 8 commits June 13, 2025 06:56

Fixed the dummy grantor in upgrade script

9271fea

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Fixed testfiles

6ccf89f

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Ignore before files in jdbc_schedule

e541ec6

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Handled role does not exist condition for internal BBF object

c7ca46e

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Fixed sequence of rows in test file

eacc339

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Replaced the dummy grantor with master_dbo to avoid inconsistency

5581341

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Created before files for db_owner test file

a022076

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Added heap tuple is valid check

feb9fd6

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

anju15bharti reviewed Jun 18, 2025

View reviewed changes

contrib/babelfishpg_tsql/src/catalog.c Outdated Show resolved Hide resolved

Added heap tuple is valid check

51516a2

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

anju15bharti requested changes Jun 18, 2025

View reviewed changes

Added condition to return from hook if object type is other than tabl…

a4fdd0b

…e, function or procedure Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

anju15bharti mentioned this pull request Jun 19, 2025

Adding Grantor Information in sys.babelfish_schema_permissions catalog amazon-aurora/postgresql_modified_for_babelfish#132

Open

Deepesh125 requested changes Jun 20, 2025

View reviewed changes

shalinilohia50 requested changes Jun 20, 2025

View reviewed changes

shreyaxr3 added 3 commits June 23, 2025 11:14

Fixed Indentation and other minor changes

31463d0

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Passed a common lock inside the functions to avoid race condition

12710c8

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

db_permissions

cb17e37

Signed-off-by: Shreya Rai <shreyaxr@amazon.com>

Imran-imtiaz48 reviewed Jul 11, 2025

View reviewed changes

Conversation

shreyeah38 commented Jun 9, 2025 • edited by anju15bharti Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shalinilohia50 Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

shreyeah38 Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anju15bharti Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15847758092

Details

💛 - Coveralls

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Deepesh125 Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

Deepesh125 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

shalinilohia50 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

shreyeah38 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shalinilohia50 Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shreyeah38 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

shreyeah38 commented Jun 9, 2025 •

edited by anju15bharti

Loading

anju15bharti Jun 18, 2025 •

edited

Loading

coveralls commented Jun 19, 2025 •

edited

Loading

shalinilohia50 Jun 20, 2025 •

edited

Loading