Stored the permissions in a single column using bits #2044

shalinilohia50 · 2023-11-21T09:30:46Z

Description

Store the permissions in a single column using bits

Issues Resolved

Task: BABEL-4344
Co-authored-by: Anju Bharti
Signed-off-by: Shalini Lohia

Test Scenarios Covered

Use case based -
Boundary conditions -
Arbitrary inputs -
Negative test cases -
Minor version upgrade tests -
Major version upgrade tests -
Performance tests -
Tooling impact -
Client tests -

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.

kuntalghosh · 2023-11-21T15:32:38Z

contrib/babelfishpg_tsql/sql/ownership.sql

@@ -19,10 +19,10 @@ CREATE TABLE sys.babelfish_schema_permissions (
  dbid smallint NOT NULL,
  schema_name NAME NOT NULL COLLATE sys.database_default,
  object_name NAME NOT NULL COLLATE sys.database_default,
-  permission NAME NOT NULL COLLATE sys.database_default,
+  permission SMALLINT,


Should be NOT NULL, right?

kuntalghosh · 2023-11-21T15:35:25Z

contrib/babelfishpg_tsql/src/catalog.c

+	table_endscan(scan);
+	table_close(bbf_schema_rel, RowExclusiveLock);
+
+	CommandCounterIncrement();


Why is this needed?

Not needed here.

kuntalghosh · 2023-11-21T15:37:41Z

contrib/babelfishpg_tsql/src/catalog.c

+		if((permission & priv) == permission)
+			return true;


Not ending the scan and not closing the table. Have you added a test for this? Otherwise, the test should have complained.

+1, but that would only come into consideration if you check the logs. There would have been explicit cache leak references

Addressed it.

kuntalghosh · 2023-11-21T15:39:57Z

contrib/babelfishpg_tsql/src/catalog.c

+			if((permission & PRIVILEGE_BIT_FOR_EXECUTE) == PRIVILEGE_BIT_FOR_EXECUTE)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "execute");
+			if((permission & PRIVILEGE_BIT_FOR_SELECT) == PRIVILEGE_BIT_FOR_SELECT)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "select");
+			if((permission & PRIVILEGE_BIT_FOR_INSERT) == PRIVILEGE_BIT_FOR_INSERT)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "insert");
+			if((permission & PRIVILEGE_BIT_FOR_UPDATE) == PRIVILEGE_BIT_FOR_UPDATE)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "update");
+			if((permission & PRIVILEGE_BIT_FOR_DELETE) == PRIVILEGE_BIT_FOR_DELETE)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "delete");
+			if((permission & PRIVILEGE_BIT_FOR_REFERENCES) == PRIVILEGE_BIT_FOR_REFERENCES)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "references");


Try to execute a single GRANT statement with all the permissions at once. Otherwise, it'll execute individual GRANT statement

We have separate sub commands for below privileges:

[select, insert, update, references] -> Applicable for Tables

[execute] -> Applicable for Functions and Procedures
So, we cannot execute a single statement if a grant statement specifies multiple privileges like insert, select, update

But, in a single iteration of the loop, the object type won't change, right?

kuntalghosh · 2023-11-21T15:40:40Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

 {
 	List	   *parsetree_list;
 	ListCell   *parsetree_item;
-	char	   *dbname = get_cur_db_name();
-	char	   *login = GetUserNameFromId(GetSessionUserId(), false);
+	parsetree_list = gen_grantschema_subcmds(schema_name, rolname, stmt->is_grant, stmt->with_grant_option, priv_name);


Shouldn't you free this list?

I don't think it's needed. References

kuntalghosh · 2023-11-21T15:59:14Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+					errmsg("Cannot find the schema \"%s\", because it does not exist or you do not have permission.", stmt->schema_name)));
+
+		if((privilege_maskInt & PRIVILEGE_BIT_FOR_EXECUTE) == PRIVILEGE_BIT_FOR_EXECUTE)
+			exec_gen_grantschema(schema_name, rolname, stmt, "execute");


Send privilege names as a list and execute a single statement

We have separate sub commands for below privileges:

[select, insert, update, references] -> Applicable for Tables

[execute] -> Applicable for Functions and Procedures
So, we cannot execute a single statement if a grant statement specifies multiple privileges like insert, select, update

I agree, we can write a single batch for all grant queries and get it executed as a batch, if thats possible. Is it? The code will be cleaner in that case

@shalinilohia50 But, for a single exec_stmt_grantschema execution, the object type won't change, right?

kuntalghosh · 2023-11-21T16:04:27Z

contrib/babelfishpg_tsql/src/pl_handler.c

@@ -3623,7 +3626,6 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
 						const char *logical_schema = NULL;


Why are you handling CREATE LOGICAL DATABASE in OBJECT_TABLE? What other subcommand can fall under this category?

One of the CREATE DATABASE subcommands was GRANT SELECT ON dummy.sysdatabases TO dummy; which we avoided here.
With this commit, looks like it's not needed anymore.

kuntalghosh · 2023-11-21T16:12:09Z

contrib/babelfishpg_tsql/src/pl_handler.c

+							if (prev_ProcessUtility)
+								prev_ProcessUtility(pstmt, queryString, readOnlyTree, context, params,
+									queryEnv, dest, qc);
+							else
+								standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params,
+									queryEnv, dest, qc);


Shouldn't this call exec_stmt_grantschema internally and update the catalog? If not, when exec_stmt_grantschema is called exactly?

We have defined a separate node PLtsql_stmt_grantschema for GRANT/REVOKE on SCHEMA statement which executes exec_stmt_grantschema

Okay. But, exec_stmt_grantschema also updates the catalog. So, do you need to update it here as well?

kuntalghosh · 2023-11-21T16:12:24Z

contrib/babelfishpg_tsql/src/pl_handler.c

+									AccessPriv *ap = (AccessPriv *) lfirst(lc1);
+									if(ap->cols != NULL)
+									{
+										flag = true;


What does this flag indicate?

This specifies if there is permission granted to a specific column in a table.
Renaming it to something meaningful.

kuntalghosh · 2023-11-21T16:28:52Z

contrib/babelfishpg_tsql/src/pltsql.h

+/* Privilege Bits for GRANT SCHEMA */
+#define PRIVILEGE_BIT_FOR_EXECUTE 32
+#define ALL_PRIVILEGE_EXCEPT_EXECUTE 31
+#define PRIVILEGE_BIT_FOR_SELECT 16
+#define PRIVILEGE_BIT_FOR_INSERT 8
+#define PRIVILEGE_BIT_FOR_UPDATE 4
+#define PRIVILEGE_BIT_FOR_DELETE 2
+#define PRIVILEGE_BIT_FOR_REFERENCES 1
+


Please use the same ACL defined in PG. This will help us reusing some internal functions/macros if required. For example you can use privilege_to_string() to retrieve the values or ACLITEM_SET_PRIVS to set the privileges.

KushaalShroff

I came across a flaw with the GRANT/REVOKE on schema code/design. In the following example you see Babelfish allows us to create objects with same names and also overload procedures and functions:

1> create table t (a int)
2> go

1> create procedure t as select 1
2> go

1>  create procedure t(@a int) as select @a
2> go

You are not scanning/looking up based on object_type at multiple places in the code.
a. You are going to return the first matching entry in table irrespective of the object_type.
b. You are inserting NULL for object_type at one place, why?
How are you gonna support Grants/Revokes on overloaded procedures and functions?
a. In the above example One grant/revoke statement will affect unintended objects, that seems like a critical bug to me
We need to add these test cases

KushaalShroff · 2023-11-21T19:04:55Z

contrib/babelfishpg_tsql/sql/ownership.sql

@@ -19,10 +19,10 @@ CREATE TABLE sys.babelfish_schema_permissions (
  dbid smallint NOT NULL,
  schema_name NAME NOT NULL COLLATE sys.database_default,
  object_name NAME NOT NULL COLLATE sys.database_default,
-  permission NAME NOT NULL COLLATE sys.database_default,
+  permission SMALLINT,


KushaalShroff · 2023-11-21T19:06:42Z

contrib/babelfishpg_tsql/src/catalog.c

+		if((permission & priv) == permission)
+			return true;


+1, but that would only come into consideration if you check the logs. There would have been explicit cache leak references

KushaalShroff · 2023-11-21T19:07:53Z

contrib/babelfishpg_tsql/src/catalog.c

+	char			*schema;
+	List			*res;
+	Node			*res_stmt;
+	PlannedStmt		*wrapper;


We need to free 3 of these, right?

KushaalShroff · 2023-11-21T19:11:57Z

contrib/babelfishpg_tsql/src/catalog.c

-
-			/* make sure later steps can see the object created here */
-			CommandCounterIncrement();
+			if((permission & PRIVILEGE_BIT_FOR_EXECUTE) == PRIVILEGE_BIT_FOR_EXECUTE)


Why do we need to equate???
Simple if(permission & PRIVILEGE_BIT_FOR_EXECUTE) should work!

KushaalShroff · 2023-11-21T19:16:05Z

contrib/babelfishpg_tsql/src/pltsql_utils.c

+int16
+create_privilege_bitmask(const List *l, bool grant_schema_stmt)
+{
+	int16 privilege_maskInt = 0;
+	ListCell   *lc;
+	foreach(lc, l)
+	{
+		char	*priv_name;
+		if (grant_schema_stmt)
+			priv_name = (char *) lfirst(lc);
+		else
+		{
+			AccessPriv *ap = (AccessPriv *) lfirst(lc);
+			priv_name = ap->priv_name;
+		}
+		if (!strcmp(priv_name,"execute"))
+			privilege_maskInt |= PRIVILEGE_BIT_FOR_EXECUTE;
+		else if (!strcmp(priv_name,"select"))
+			privilege_maskInt |= PRIVILEGE_BIT_FOR_SELECT;
+		else if (!strcmp(priv_name,"insert"))
+			privilege_maskInt |= PRIVILEGE_BIT_FOR_INSERT;
+		else if (!strcmp(priv_name,"update"))
+			privilege_maskInt |= PRIVILEGE_BIT_FOR_UPDATE;
+		else if (!strcmp(priv_name,"delete"))
+			privilege_maskInt |= PRIVILEGE_BIT_FOR_DELETE;
+		else if (!strcmp(priv_name,"references"))
+			privilege_maskInt |= PRIVILEGE_BIT_FOR_REFERENCES;
+	}
+	return privilege_maskInt;


WHY?
Why cant we simply store privilege_maskInt during parsing? You are parsing a string and storing a list of strings in antlr and then coming here to convert List of strings to int. Seems unnecessarily costly to me.

I tried storing bitmask during parsing but then to do this change was eventually leading change in structures' definition of postgres, hence we need to store strings only during parsing.

As I mentioned, use ACL defined in PG. It provides a lot of APIs for these.

KushaalShroff · 2023-11-21T20:01:34Z

contrib/babelfishpg_tsql/src/catalog.c

+						int16 new_priv,
+						int16 old_priv,
+						const char *grantee,
+						const char *object_type,


I cannot the use of object_type, why is it needed?
Was there no compiler warning at the time of build?

KushaalShroff · 2023-11-21T20:05:59Z

contrib/babelfishpg_tsql/sql/ownership.sql

  grantee NAME NOT NULL COLLATE sys.database_default,
  object_type NAME COLLATE sys.database_default,
-  PRIMARY KEY(dbid, schema_name, object_name, permission, grantee)
+  PRIMARY KEY(dbid, schema_name, object_name, grantee)


OBJECT_TYPE should be part of primary key too, right?

KushaalShroff · 2023-11-21T20:07:17Z

contrib/babelfishpg_tsql/src/catalog.c

+				tsql_get_server_collation_oid_internal(false),
+				F_NAMEEQ,
+				CStringGetDatum(schema_name));
+	ScanKeyEntryInitialize(&scanKey[2], 0,


We need to scan based on object type too!!!

How are you going to distinguish between a table and procedure?

1> create table t (a int) 2> go 1> create procedure t as select 1 2> go

KushaalShroff · 2023-11-21T20:20:48Z

contrib/babelfishpg_tsql/src/catalog.c

+	ScanKeyInit(&scanKey[0],
+				Anum_bbf_schema_perms_dbid,
+				BTEqualStrategyNumber, F_INT2EQ,
+				Int16GetDatum(dbid));
+	ScanKeyEntryInitialize(&scanKey[1], 0,
+				Anum_bbf_schema_perms_schema_name,
+				BTEqualStrategyNumber,
+				InvalidOid,
+				tsql_get_server_collation_oid_internal(false),
+				F_NAMEEQ,
+				CStringGetDatum(schema_name));
+	ScanKeyEntryInitialize(&scanKey[2], 0,
+				Anum_bbf_schema_perms_object_name,
+				BTEqualStrategyNumber,
+				InvalidOid,
+				tsql_get_server_collation_oid_internal(false),
+				F_NAMEEQ,
+				CStringGetDatum(object_name));
+	ScanKeyEntryInitialize(&scanKey[3], 0,
+				Anum_bbf_schema_perms_grantee,
+				BTEqualStrategyNumber,
+				InvalidOid,
+				tsql_get_server_collation_oid_internal(false),
+				F_NAMEEQ,
+				CStringGetDatum(grantee));


Based on this logic you are going to return the first object which matches irrespective of the object_type.
What if the first one was a table but user was granting for a procedure?

KushaalShroff · 2023-11-21T20:26:00Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+
+		/* Add entry for each grant statement. */
+		if (stmt->is_grant && !check_bbf_schema_for_entry(stmt->schema_name, PERMISSIONS_FOR_ALL_OBJECTS_IN_SCHEMA, rolname))
+			add_entry_to_bbf_schema(stmt->schema_name, PERMISSIONS_FOR_ALL_OBJECTS_IN_SCHEMA,  privilege_maskInt, rolname, NULL);


Why are you inserting NULL for object_type?

kuntalghosh · 2023-11-22T06:05:22Z

contrib/babelfishpg_tsql/src/catalog.c

+				CStringGetDatum(grantee));
+
+	scan = table_beginscan_catalog(bbf_schema_rel, 5, scanKey);
+	tuple_bbf_schema = heap_getnext(scan, ForwardScanDirection);


Yeah, in some place, it's a heap_getnext and in some place it's a sys scan.

kuntalghosh · 2023-11-22T06:06:04Z

contrib/babelfishpg_tsql/src/catalog.c

+				NULL);
+
+	/* make sure later steps can see the object created here */
+	CommandCounterIncrement();


Whats the next step?

kuntalghosh · 2023-11-22T06:07:58Z

contrib/babelfishpg_tsql/src/catalog.c

+			if((permission & PRIVILEGE_BIT_FOR_EXECUTE) == PRIVILEGE_BIT_FOR_EXECUTE)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "execute");
+			if((permission & PRIVILEGE_BIT_FOR_SELECT) == PRIVILEGE_BIT_FOR_SELECT)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "select");
+			if((permission & PRIVILEGE_BIT_FOR_INSERT) == PRIVILEGE_BIT_FOR_INSERT)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "insert");
+			if((permission & PRIVILEGE_BIT_FOR_UPDATE) == PRIVILEGE_BIT_FOR_UPDATE)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "update");
+			if((permission & PRIVILEGE_BIT_FOR_DELETE) == PRIVILEGE_BIT_FOR_DELETE)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "delete");
+			if((permission & PRIVILEGE_BIT_FOR_REFERENCES) == PRIVILEGE_BIT_FOR_REFERENCES)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "references");


But, in a single iteration of the loop, the object type won't change, right?

kuntalghosh · 2023-11-22T06:09:48Z

contrib/babelfishpg_tsql/src/catalog.c

+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "execute");
+			if((permission & PRIVILEGE_BIT_FOR_SELECT) == PRIVILEGE_BIT_FOR_SELECT)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "select");
+			if((permission & PRIVILEGE_BIT_FOR_INSERT) == PRIVILEGE_BIT_FOR_INSERT)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "insert");
+			if((permission & PRIVILEGE_BIT_FOR_UPDATE) == PRIVILEGE_BIT_FOR_UPDATE)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "update");
+			if((permission & PRIVILEGE_BIT_FOR_DELETE) == PRIVILEGE_BIT_FOR_DELETE)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "delete");
+			if((permission & PRIVILEGE_BIT_FOR_REFERENCES) == PRIVILEGE_BIT_FOR_REFERENCES)
+				grant_perms_to_each_obj(db_name, object_type, schema_name, object_name, grantee, "references");


In case of an error, the table will be closed automatically unless we catch the error explicitly.

kuntalghosh · 2023-11-22T06:11:28Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+					errmsg("Cannot find the schema \"%s\", because it does not exist or you do not have permission.", stmt->schema_name)));
+
+		if((privilege_maskInt & PRIVILEGE_BIT_FOR_EXECUTE) == PRIVILEGE_BIT_FOR_EXECUTE)
+			exec_gen_grantschema(schema_name, rolname, stmt, "execute");


@shalinilohia50 But, for a single exec_stmt_grantschema execution, the object type won't change, right?

kuntalghosh · 2023-11-22T06:14:57Z

contrib/babelfishpg_tsql/src/pl_handler.c

+							if (prev_ProcessUtility)
+								prev_ProcessUtility(pstmt, queryString, readOnlyTree, context, params,
+									queryEnv, dest, qc);
+							else
+								standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params,
+									queryEnv, dest, qc);


Okay. But, exec_stmt_grantschema also updates the catalog. So, do you need to update it here as well?

kuntalghosh · 2023-11-22T06:16:48Z

contrib/babelfishpg_tsql/src/pltsql_utils.c

+int16
+create_privilege_bitmask(const List *l, bool grant_schema_stmt)
+{
+	int16 privilege_maskInt = 0;
+	ListCell   *lc;
+	foreach(lc, l)
+	{
+		char	*priv_name;
+		if (grant_schema_stmt)
+			priv_name = (char *) lfirst(lc);
+		else
+		{
+			AccessPriv *ap = (AccessPriv *) lfirst(lc);
+			priv_name = ap->priv_name;
+		}
+		if (!strcmp(priv_name,"execute"))
+			privilege_maskInt |= PRIVILEGE_BIT_FOR_EXECUTE;
+		else if (!strcmp(priv_name,"select"))
+			privilege_maskInt |= PRIVILEGE_BIT_FOR_SELECT;
+		else if (!strcmp(priv_name,"insert"))
+			privilege_maskInt |= PRIVILEGE_BIT_FOR_INSERT;
+		else if (!strcmp(priv_name,"update"))
+			privilege_maskInt |= PRIVILEGE_BIT_FOR_UPDATE;
+		else if (!strcmp(priv_name,"delete"))
+			privilege_maskInt |= PRIVILEGE_BIT_FOR_DELETE;
+		else if (!strcmp(priv_name,"references"))
+			privilege_maskInt |= PRIVILEGE_BIT_FOR_REFERENCES;
+	}
+	return privilege_maskInt;


As I mentioned, use ACL defined in PG. It provides a lot of APIs for these.

Stored the permissions in a single column using bits

8301e76

shalinilohia50 requested a review from kuntalghosh November 21, 2023 09:30

lohia-shalini added 3 commits November 21, 2023 11:55

Merge branch 'BABEL_3_X_DEV' into fix-perms-col

202790c

Define macros

e05f06f

Fix tests

b40b2f2

shalinilohia50 requested a review from KushaalShroff November 21, 2023 12:59

Add a comment

a9bbf12

kuntalghosh requested changes Nov 21, 2023

View reviewed changes

Removed unnecessary checks

119a217

KushaalShroff requested changes Nov 21, 2023

View reviewed changes

shalinilohia50 closed this Nov 22, 2023

kuntalghosh reviewed Nov 22, 2023

View reviewed changes

KushaalShroff mentioned this pull request Jan 14, 2024

Support GRANT .. ON SCHEMA .. in Babelfish #2215

Merged

		@@ -3623,7 +3626,6 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
		const char *logical_schema = NULL;

Stored the permissions in a single column using bits #2044

Stored the permissions in a single column using bits #2044

Conversation

shalinilohia50 commented Nov 21, 2023 • edited Loading

Description

Issues Resolved

Test Scenarios Covered

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shalinilohia50 Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KushaalShroff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anju15bharti Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shalinilohia50 commented Nov 21, 2023 •

edited

Loading

shalinilohia50 Nov 21, 2023 •

edited

Loading

anju15bharti Nov 22, 2023 •

edited

Loading