Support for DBCC CHECKIDENT #1646

aadityavermaa · 2023-07-25T15:47:06Z

Description

This changes adds support for DBCC CHECKIDENT command in Babelfish that allows user to reset the current value of the identity column in a table. Earlier user had to do the same with Setval() command from pg end-point, with this change they can do it directly from T-SQL end-point. As this is the first time a DBCC command is implemented in Babelfish, this change also provides support to syntax for DBCC commands, and the design is kept scalable so that new DBCC commands can be better introduced in future.

Issues Resolved

BABEL-3201

Test Scenarios Covered

Use case based -
Yes
Boundary conditions -
Yes
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.

Signed-off-by: Aditya Verma <[email protected]>

kuntalghosh · 2023-08-11T05:57:36Z

contrib/babelfishpg_tsql/src/iterative_exec.c

+			if (pltsql_explain_only)
+				{
+				ereport(ERROR,
+						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						 errmsg("Showing Estimated Execution Plan for DBCC statment is not yet supported")));
+				}


DBCC is utility command. It doesn't make sense to show an execution plan for the same. What's the SQL Server behavior?

Executing DBCC CHECKIDENT after SET SHOWPLAN_ALL ON in SQL Server does not throw and error but most of the fields come out to be NULL, I have removed the condition for now.

Sorry, which condition exactly have you removed? What's the Babelfish behavior with your changes?

Apologies, I just saw that I hadn't included this file in the commit, I meant I have removed the pltsql_explain_only condition for now.

contrib/babelfishpg_tsql/src/pl_funcs-2.c

kuntalghosh · 2023-08-11T06:10:49Z

contrib/babelfishpg_tsql/src/tsqlUnsupportedFeatureHandler.cpp

+antlrcpp::Any TsqlUnsupportedFeatureHandlerImpl::visitDbcc_statement(TSqlParser::Dbcc_statementContext *ctx)
+{
+
+	if (ctx->dbcc_command())


We should add some telemetry for DBCC statements. Can be addressed separately.

contrib/babelfishpg_tsql/src/pl_exec-2.c

kuntalghosh · 2023-08-11T06:38:48Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+	/*
+	 * We need to find out the last value of the identity column in the table
+	 * and last value of the identity sequence to compare and decide on the
+	 * action that needs to be taken.
+	 */


It's not clear what CHECKIDENT behavior we're trying to implement here. Also, we shouldn't do the relation scan if customer has already provided the new seed value.

Updated the comments and finding the max value of the column where it's required ( not when new_reseed_value ) is already provided.

kuntalghosh · 2023-08-11T06:41:14Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+	 * and last value of the identity sequence to compare and decide on the
+	 * action that needs to be taken.
+	 */
+	scan = table_beginscan_catalog(rel, 0, NULL);


Why are we using catalog scan API to scan an user relation?

Since we have to use MAX() to find out the maximum value, I have removed the catalog scan API.

kuntalghosh · 2023-08-11T06:42:49Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+
+	if (HeapTupleIsValid(tuple))
+	{
+		max_identity_value = DatumGetInt64(heap_getattr(tuple, attnum+1,


The last value need not be the max value. We should execute the select max(id) from table query and fetch the result.

Made the change in the latest commit. Thanks.

kuntalghosh · 2023-08-11T06:47:35Z

test/JDBC/input/BABEL-3201-vu-verify.sql

@@ -0,0 +1,35 @@
+DBCC CHECKIDENT(babel_3201_t1);


Please try to add at least one test case for all the errors these changes are introducing.

I have added test cases for all the functional scenarios, corner cases ( no rows have been inserted in table, rows are removed using TRUNCATE TABLE, rows are deleted using DELETE TABLE), and negative tests for syntax and datatype bounds.

surendravishnoi · 2023-08-14T09:24:26Z

test/JDBC/expected/BABEL-3201-vu-verify.out

@@ -0,0 +1,59 @@
+DBCC CHECKIDENT(babel_3201_t1);


Please also add some tests to validate transactional behaviour.
Running DBCC inside a transaction
DBCC running into error inside transaction
Calling DBCC from procedure/functions (with/without errors)

Added tests for transactions and procedures. Please take a look. Thanks.

- addreseed review comments - negative input parameter showed error, fixed - updated permission checks - use max(id) to find current identity value - updated testcases - fix crash if reseed is used on empty table - Signed-off-by: Aditya Verma <[email protected]>

Signed-off-by: Aditya Verma <[email protected]>

- added permission related tests - updated BABEL-UNSUPPORTED tests - added Relation lock on table Signed-off-by: Aditya Verma <[email protected]>

contrib/babelfishpg_tsql/src/pl_exec-2.c

test/JDBC/input/BABEL-3201-vu-verify.mix

- fix for locking behaviour - tests in isolation test framework for concurrency - Add some missing jdbc tests - Address review comments Signed-off-by: Sandeep Kumawat <[email protected]>

Signed-off-by: Sandeep Kumawat <[email protected]>

contrib/babelfishpg_tsql/src/pl_exec-2.c

HarshLunagariya · 2023-10-05T09:21:01Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+	else
+	{
+		/* 
+		 * If schema_name is not provided, find default schema for current user
+		 * and get physical schema name
+		 */
+		if (!user)
+		{
+			ereport(ERROR,
+			(errcode(ERRCODE_UNDEFINED_OBJECT),
+				 errmsg("user does not exist")));
+		}
+		else if ((guest_role_name && strcmp(user, guest_role_name) == 0))
+		{
+			nsp_name = pstrdup(get_guest_schema_name(db_name));
+		}
+		else
+		{
+			schema_name = get_authid_user_ext_schema_name((const char *) db_name, user);
+			nsp_name = get_physical_schema_name(db_name, schema_name);
+		}


Missing dbo user handling. Please refer set_search_path_for_user_schema() for more details.

made changes. Please have a look.

contrib/babelfishpg_tsql/src/tsqlIface.cpp

surendravishnoi · 2023-10-10T07:20:36Z

contrib/babelfishpg_tsql/src/tsqlIface.cpp

+		if (ctx->table_name()->schema)
+			schema_name = stripQuoteFromId(ctx->table_name()->schema);
+		if (ctx->table_name()->table)
+		table_name = stripQuoteFromId(ctx->table_name()->table);


surendravishnoi · 2023-10-10T08:37:18Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+	int64	max_identity_value = 0;
+	int64	cur_identity_value = 0;
+	char	*max_identity_value_str = NULL;
+    char	*login = GetUserNameFromId(GetSessionUserId(), false);


indentation

surendravishnoi · 2023-10-10T08:37:35Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+	char		message[200];
+
+
+    bool    is_cross_db = false;


indentation

surendravishnoi · 2023-10-10T10:56:23Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+	{
+		PG_RE_THROW();
+	    if (is_cross_db)
+            SetCurrentRoleId(current_user_id, false);


control will never come here, we should first do set and then rethrow

made changes. please have a look.

surendravishnoi · 2023-10-10T10:57:51Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+	}
+	PG_CATCH();
+	{
+		PG_RE_THROW();


who will release lock in case of errors?

made changes to release locks in case of errors.

surendravishnoi · 2023-10-10T11:01:52Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+
+	if(!dbcc_stmt.new_reseed_value)
+	{
+		UnlockRelationOid(table_oid, ShareLock);


what about exclusive lock?

So, we are trying to hold this shareLock on table only for the execution of this statement. We try to hold Exclusive Lock on a transaction level. We want to release this share lock as soon as we are done with the execution of DBCC CHECKIDENT() stmt. But Exclusive Lock will get release when transection gets commited or rollbacked.

surendravishnoi · 2023-10-10T11:03:47Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+	}
+	PG_END_TRY();
+
+	if (max_identity_value_str)


we want to free memory for error cases as well

done. made changes in the next commit. please take a look.

surendravishnoi · 2023-10-10T11:04:28Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+		if (is_cross_db)
+            SetCurrentRoleId(current_user_id, false);
+	}
+	PG_CATCH();


do we have test cases with different error and lock types?

So there are test-cases in isolation test framwork, when one transection made update to table t1 (by reseeding identity value via dbcc checkident) and other transection is not able to use t1 inside a dbcc checkident stmt.

- Correct Parser Support for DBCC CHECKIDENT,<table_name> in quotes is the correct way. - Update test files accordingly - Adds more test-cases for cross-db errors, db_name longer then 63, Mixed-cases names, etc. - Made updates to free memory in case of errors Signed-off-by: Sandeep Kumawat <[email protected]>

HarshLunagariya · 2023-10-17T08:41:53Z

contrib/babelfishpg_tsql/antlr/TSqlParser.g4

+    | CHECKDB
+    | CHECKFILEGROUP
+    | CHECKIDENT


Do we still need these changes ? since in the dbcc_command rule we have already explicitly specified them.

Correct. Removing.

HarshLunagariya · 2023-10-17T08:48:16Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

@@ -19,6 +20,8 @@
 #include "pl_explain.h"
 #include "session.h"

+#include <math.h>


Is it needed?
I see we are using pg_strtoint64 which is there 'utils/builtins.h' which is indirectly included by some other header.

Yes. Removing it .

HarshLunagariya · 2023-10-17T08:57:27Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+		else
+		{
+			nsp_name = get_physical_schema_name(db_name, dbcc_stmt.schema_name);
+		}


This is incorrect. Here dbcc_stmt will be NULL We are missing coverage for this case otherwise it would have failed.
It should be schema_name instead. Move schema_name evaluation inside this condition.

Thanks for pointing this out. Fixing it and also adding a test-case for the coverage. However as we do need schema_ name later ahead in the code, keeping schema_name evaluation out of this else condition

Signed-off-by: Sandeep Kumawat <[email protected]>

surendravishnoi · 2023-10-18T09:09:06Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+
+		if (is_float_value)
+		{
+			if (dbcc_stmt.new_reseed_value[0] == '.' || 


have we tried input like ".", "-" etc for testing?

tested and added cases.

surendravishnoi · 2023-10-18T09:11:29Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+			(errcode(ERRCODE_UNDEFINED_DATABASE),
+				errmsg("database \"%s\" does not exist", dbcc_stmt.db_name)));
+		}
+       		if (pg_strncasecmp(db_name, dbcc_stmt.db_name, NAMEDATALEN) != 0)


make sure we have test cases for mixed case db name and DB name more than 64 chars (if allowed)

There are test cases with db name longer then 64 char, mixed case db name.

surendravishnoi · 2023-10-18T09:11:58Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+    	}
+
+	user = get_user_for_database(db_name);
+	login_is_db_owner = 0 == strncmp(GetUserNameFromId(GetSessionUserId(), false),


make sure we have test cases for mixed case user name and user name more than 64 chars (if allowed)

added a test case where user name is longer then 64 chars.

surendravishnoi · 2023-10-18T09:30:59Z

contrib/babelfishpg_tsql/src/pl_exec-2.c

+		 */
+		if (!user)
+		{
+			ereport(ERROR,


not clear. can you test without cross db? also, if we cannot write a test case means that code path is not possible. If that is the case, add an assert (at-least for debug)

- Address PR comments - Fixes locking behaviour Signed-off-by: Sandeep Kumawat <[email protected]>

HarshLunagariya

Verified user and permissions related changes. Looks good!

On leave and goes beyond code freeze. Will request Kuntal to review this again once he is back. Meanwhile all the comments are addressed and resolved.

Aditya Verma added 6 commits July 25, 2023 15:41

Parser support for BABEL-3201

9309443

Signed-off-by: Aditya Verma <[email protected]>

change dbcc_command to CHECKIDENT in newly added grammar

c713130

Signed-off-by: Aditya Verma <[email protected]>

remove commented code

b96e1b7

Signed-off-by: Aditya Verma <[email protected]>

implementation for NORESEED and RESEED option

afc6dbc

Signed-off-by: Aditya Verma <[email protected]>

remove not required comments

1836d40

Signed-off-by: Aditya Verma <[email protected]>

handle cases where table has no values, added comments

a71eab1

Signed-off-by: Aditya Verma <[email protected]>

aadityavermaa changed the title ~~Parser support for DBCC CHECKIDENT~~ Support for DBCC CHECKIDENT Aug 11, 2023

kuntalghosh requested changes Aug 11, 2023

View reviewed changes

surendravishnoi reviewed Aug 14, 2023

View reviewed changes

Aditya Verma and others added 7 commits August 22, 2023 06:05

updated test cases and moved position of permission check

eef6981

Signed-off-by: Aditya Verma <[email protected]>

made updates

b640db6

- added permission related tests - updated BABEL-UNSUPPORTED tests - added Relation lock on table Signed-off-by: Aditya Verma <[email protected]>

Merge branch 'BABEL_3_X_DEV' into babel-3201

fa7862b

implement output messages for dbcc checkident

2d740ac

Re-implemented lock

a434ef7

Merge branch 'BABEL_3_X_DEV' into babel-3201

ef07676

kuntalghosh previously requested changes Sep 27, 2023

View reviewed changes

skumawat2025 requested a review from HarshLunagariya September 27, 2023 09:55

Sandeep Kumawat added 3 commits October 3, 2023 05:28

This commit includes-

10a35bc

- fix for locking behaviour - tests in isolation test framework for concurrency - Add some missing jdbc tests - Address review comments Signed-off-by: Sandeep Kumawat <[email protected]>

Merge branch 'BABEL_3_X_DEV' into babel-3201

63e0997

Fix: test failures

748739c

Signed-off-by: Sandeep Kumawat <[email protected]>

HarshLunagariya suggested changes Oct 5, 2023

View reviewed changes

surendravishnoi reviewed Oct 10, 2023

View reviewed changes

Sandeep Kumawat added 2 commits October 15, 2023 23:24

Merge branch 'BABEL_3_X_DEV' into babel-3201

9c08ae0

HarshLunagariya reviewed Oct 17, 2023

View reviewed changes

Sandeep Kumawat added 2 commits October 17, 2023 20:55

Address review comments

a1a27c8

Signed-off-by: Sandeep Kumawat <[email protected]>

address test failures

307681a

Signed-off-by: Sandeep Kumawat <[email protected]>

surendravishnoi reviewed Oct 18, 2023

View reviewed changes

Merge branch 'BABEL_3_X_DEV' into babel-3201

ee3eba8

This commit -

b589b73

- Address PR comments - Fixes locking behaviour Signed-off-by: Sandeep Kumawat <[email protected]>

skumawat2025 requested review from surendravishnoi and HarshLunagariya October 19, 2023 15:52

surendravishnoi approved these changes Oct 20, 2023

View reviewed changes

HarshLunagariya reviewed Oct 20, 2023

View reviewed changes

jsudrik merged commit 2ad7351 into babelfish-for-postgresql:BABEL_3_X_DEV Oct 23, 2023
27 checks passed

Support for DBCC CHECKIDENT #1646

Support for DBCC CHECKIDENT #1646

Conversation

aadityavermaa commented Jul 25, 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

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

HarshLunagariya left a comment

Choose a reason for hiding this comment

aadityavermaa commented Jul 25, 2023 •

edited

Loading