Fix IDENTITY_INSERT to work cross-db #3402

thephantomthief · 2025-01-14T06:44:52Z

Description

Previously, passing three part object names to SET IDENTITY_INSERT was
throwing an error. In this commit, if the relation name is a three part
name, we will parse the catalog name and set the IDENTITY_INSERT
property for the relation in that catalog.

Also with this commit, we will check if current user has permission to SET
IDENTITY_INSERT for the relation.

Task: BABEL-3212
Signed-off-by: Sharu Goel goelshar@amazon.com

Test Scenarios Covered

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

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.

Previously, passing three part object names to SET IDENTITY_INSERT was throwing an error. In this commit, if the relation name is a three part name, we will parse the catalog name and set the IDENTITY_INSERT property for the relation in that catalog. Also with this commit, we will check if current user has permission to SET IDENTITY_INSERT for the relation. Task: BABEL-3212 Signed-off-by: Sharu Goel <goelshar@amazon.com>

coveralls · 2025-01-14T08:53:49Z

Pull Request Test Coverage Report for Build 12763602829

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

32 of 40 (80.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.002%) to 74.959%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
contrib/babelfishpg_tsql/src/pltsql_utils.c	6	7	85.71%
contrib/babelfishpg_tsql/src/pl_handler.c	26	33	78.79%

Totals
Change from base Build 12742860948:	0.002%
Covered Lines:	47003
Relevant Lines:	62705

💛 - Coveralls

shalinilohia50 · 2025-01-15T17:49:28Z

test/JDBC/expected/BABEL-IDENTITY.out

@@ -1208,7 +1208,7 @@ Index Only Scan using babel_3384_test_pkey on babel_3384_test

 ~~START~~
 text
-Babelfish T-SQL Batch Parsing Time: 0.159 ms
+Babelfish T-SQL Batch Parsing Time: 0.154 ms


Can't these values change?

Yes they can but the test framework ignores the Batch Parsing time when taking the diff. See: https://github.com/babelfish-for-postgresql/babelfish_extensions/blob/BABEL_5_X_DEV/test/JDBC/src/test/java/com/sqlsamples/TestQueryFile.java#L380

HarshLunagariya · 2025-01-16T09:28:30Z

contrib/babelfishpg_tsql/src/pl_handler.c

+				schema ? schema : "",
+				schema ? "." : "",


It will produce db1.obj1 instead of db1..obj1 when there is no schema name supplied.

Actually SET IDENTITY_INSERT db1..obj1 ON/OFF will throw syntax error at or near ".."

HarshLunagariya · 2025-01-16T09:34:46Z

contrib/babelfishpg_tsql/src/pl_handler.c

 				schema_name = get_physical_schema_name(cur_db_name,
-													   schema_name);
+													   logical_schema_name);
+
+			/* If no schema name is provided, we should use default schema */
+			if (!schema_name)


By this logic, We will be using default schema when user supplies wrong schema.

Do we have any testcase covering it?

get_physical_schema_name() doesn't perform the catalog look up. The actual error will get thrown at https://github.com/babelfish-for-postgresql/babelfish_extensions/pull/3402/files#diff-ce1101bab57d0e7f8b36f0df103ed3ae49f4b488e473ac87bf2320c32f2697e1R430. For test case see https://github.com/babelfish-for-postgresql/babelfish_extensions/pull/3402/files#diff-c16577e6f7e7da2222539138bf10997966a06ded7b507a823fe31328fbfe27d4R146.

HarshLunagariya · 2025-01-16T09:41:23Z

test/JDBC/expected/IDENTITY-INSERT-CROSS-DB.out

+-- tsql
+CREATE DATABASE identity_insert_db
+GO
+


Can we avoid making this new file IDENTITY-INSERT-CROSS-DB? , instead I suggest moving this to BABEL-IDENTITY.

tanscorpio7 · 2025-01-16T19:39:21Z

contrib/babelfishpg_tsql/src/pltsql_utils.c

+Oid
+get_pg_class_obj_owner_id(Oid relation_oid)
+{
+	HeapTuple		tuple;
+	Oid      		ownerId;
+
+	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relation_oid));
+	if (!HeapTupleIsValid(tuple))
+		ereport(ERROR,
+				(errcode(ERRCODE_UNDEFINED_TABLE),
+				 errmsg("relation with OID %u does not exist", relation_oid)));
+	ownerId = ((Form_pg_class) GETSTRUCT(tuple))->relowner;
+	ReleaseSysCache(tuple);
+
+	return ownerId;
+}
+


We can use existing engine function object_ownercheck for checking if user is owner of table.

tanscorpio7 · 2025-01-16T19:40:02Z

contrib/babelfishpg_tsql/src/pl_handler.c

@@ -335,6 +348,10 @@ assign_identity_insert(const char *newval, void *extra)
 		char	   *id_insert_rel_name = NULL;
 		char	   *id_insert_schema_name = NULL;
 		char	   *cur_db_name;
+		char	   *catalog_name = NULL;


Why are we naming this as catalog_name ?

database name is also commonly referred as catalog name

tanscorpio7 · 2025-01-16T19:50:38Z

contrib/babelfishpg_tsql/src/pl_handler.c

+		if (!(has_privs_of_role(curr_user_id, get_pg_class_obj_owner_id(rel_oid)) ||
+			has_privs_of_role(curr_user_id, get_db_ddladmin_oid(cur_db_name, false)) ||
+			has_privs_of_role(curr_user_id, get_db_owner_oid(cur_db_name, false))))
+			throw_error_for_identity_insert(catalog_name, logical_schema_name, rel_name);


I think the ideal way to check permission is to figure out the logical db_id from the relation schema OID.
Relation schema OID --> belongs to which logical database --> now check permissions using this db_id.

It is possible to force scenarios like

Current db is some_db_1 but search path contains schema from some_other_db_2

If we create database db1 & db1_db1, I can refer db1_db1 database object using db1.db1_schema_name.object.

Not sure how I can force (1). For (2) if there exists database db1 and db1_db1, you cannot create schema db1_dbo in db1 database. It will throw error schema "db1_db1_dbo" already exists

CREATE DATABASE db1 CREATE DATABASE db1_db1 USE db1_db1 Changed database context to 'db1_db1'. CREATE SCHEMA s1 CREATE VIEW s1.v1 AS SELECT 'I AM IN DB1_DB1' USE db1 Changed database context to 'db1'. SELECT * FROM db1.db1_s1.v1 ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- I AM IN DB1_DB1 (1 rows affected)

For 1, easiest way is to set the search path yourself using set_config() and we internally do some set_config("Search_path", ..) that will have the same effect.

For 2

1> use master 2> go Changed database context to 'master'. 1> create procedure sp_proc_in_master_dbo as select * from v1 2> go 1> create view v1 as select 'I am in master_dbo' 2> go 1> use tempdb 2> go Changed database context to 'tempdb'. 1> sp_proc_in_master_dbo 2> go ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- I am in master_dbo (1 rows affected)

This is just one case. I can force many other such cases where we pick an object which does not exists in current logical database when object is not schema qualified.

I was actually thinking on lines of incorrectly resolving from object A to object B where both A and B exist. But you are talking about the case where A does not exist and we resolve to B. Will fix this.

tanscorpio7 · 2025-01-16T20:05:55Z

contrib/babelfishpg_tsql/src/pl_handler.c

 static void
 assign_identity_insert(const char *newval, void *extra)
 {


We need to move this to check hook. A GUC assign hook must never fail.
see src/backend/utils/misc/README

Note that there is no provision for a failure result code. assign_hooks should never fail except under the most dire circumstances, since a failure may for example result in GUC settings not being rolled back properly during transaction abort. In general, try to do anything that could conceivably fail in a check_hook instead, and pass along the results in an "extra" struct, so that the assign hook has little to do beyond copying the data to someplace. This applies particularly to catalog lookups: any required lookups must be done in the check_hook, since the assign_hook may be executed during transaction rollback when lookups will be unsafe.

Actually this one might not be a problem in this case since we are not actually using the GUC value anywhere and instead maintaining are own static variables.

tanscorpio7 · 2025-01-16T20:27:25Z

contrib/babelfishpg_tsql/src/pl_handler.c

@@ -362,40 +379,61 @@ assign_identity_insert(const char *newval, void *extra)
 		option_flag = (char *) linitial(elemlist);
 		rel_name = (char *) lsecond(elemlist);

+		/* Use catalog name if provided */
+		if (list_length(elemlist) == 4)


We need to add test cases where schema name/table name has a full stop. Using !SplitGUCList(input_string, '.', &elemlist) does not make me feel good.

Yeah this is wrong. We need to remove the grammar from bison and set the GUC from ANTLR.

This is problematic because customer can do [schema.name].table for set identity insert and we will incorrectly change identity insert value of some other logical databases table.

tanscorpio7 · 2025-01-16T20:32:51Z

contrib/babelfishpg_tsql/src/pl_handler.c

 		/* Check the user provided schema value */
 		if (list_length(elemlist) >= 3)
 		{
-			schema_name = (char *) lthird(elemlist);
+			logical_schema_name = (char *) lthird(elemlist);


Let use add a test case with schema name missing set identity_insert db1..t5 on

I had added them before but everything threw syntax error. I'll add them back.

Let keep the failing tests even if we are not fixing it right now. This will ensure any future changed does not crash here because of NULL logical_schema_name

Signed-off-by: Sharu Goel <goelshar@amazon.com>

…cross-db-identity

Sharu Goel added 2 commits January 14, 2025 06:42

Remove duplicate test file

b2284a1

thephantomthief requested a review from HarshLunagariya January 14, 2025 08:26

thephantomthief requested review from HarshLunagariya and shalinilohia50 and removed request for HarshLunagariya January 14, 2025 14:15

shalinilohia50 reviewed Jan 15, 2025

View reviewed changes

thephantomthief requested review from shalinilohia50 and tanscorpio7 January 16, 2025 05:01

thephantomthief assigned thephantomthief and unassigned thephantomthief Jan 16, 2025

HarshLunagariya suggested changes Jan 16, 2025

View reviewed changes

shalinilohia50 approved these changes Jan 16, 2025

View reviewed changes

tanscorpio7 suggested changes Jan 16, 2025

View reviewed changes

thephantomthief requested review from tanscorpio7 and HarshLunagariya January 17, 2025 05:27

Sharu Goel added 2 commits January 17, 2025 05:55

Add tests. Combine tests in single file

2dd8343

Signed-off-by: Sharu Goel <goelshar@amazon.com>

Merge remote-tracking branch 'upstream/BABEL_5_X_DEV' into sharu-dev-…

a75f419

…cross-db-identity

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix IDENTITY_INSERT to work cross-db #3402

Fix IDENTITY_INSERT to work cross-db #3402

thephantomthief commented Jan 14, 2025 •

edited

Loading

coveralls commented Jan 14, 2025

shalinilohia50 Jan 15, 2025

thephantomthief Jan 16, 2025

HarshLunagariya Jan 16, 2025

thephantomthief Jan 16, 2025

HarshLunagariya Jan 16, 2025

HarshLunagariya Jan 16, 2025

thephantomthief Jan 16, 2025

HarshLunagariya Jan 16, 2025

tanscorpio7 Jan 16, 2025

tanscorpio7 Jan 16, 2025

thephantomthief Jan 17, 2025

tanscorpio7 Jan 16, 2025 •

edited

Loading

tanscorpio7 Jan 16, 2025

thephantomthief Jan 17, 2025

tanscorpio7 Jan 17, 2025

tanscorpio7 Jan 17, 2025 •

edited

Loading

tanscorpio7 Jan 17, 2025

thephantomthief Jan 17, 2025

tanscorpio7 Jan 16, 2025

tanscorpio7 Jan 16, 2025

tanscorpio7 Jan 16, 2025

thephantomthief Jan 17, 2025 •

edited

Loading

tanscorpio7 Jan 17, 2025

tanscorpio7 Jan 16, 2025

thephantomthief Jan 17, 2025

tanscorpio7 Jan 17, 2025

Fix IDENTITY_INSERT to work cross-db #3402

Are you sure you want to change the base?

Fix IDENTITY_INSERT to work cross-db #3402

Conversation

thephantomthief commented Jan 14, 2025 • edited Loading

Description

Test Scenarios Covered

Check List

coveralls commented Jan 14, 2025

Pull Request Test Coverage Report for Build 12763602829

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

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

tanscorpio7 Jan 16, 2025 • 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

tanscorpio7 Jan 17, 2025 • 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

thephantomthief Jan 17, 2025 • 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

thephantomthief commented Jan 14, 2025 •

edited

Loading

tanscorpio7 Jan 16, 2025 •

edited

Loading

tanscorpio7 Jan 17, 2025 •

edited

Loading

thephantomthief Jan 17, 2025 •

edited

Loading