Add support for sys.sp_reset_connection stored procedure #2758

sharathbp · 2024-07-19T13:14:06Z

Description

Support sys.sp_reset_connection store procedure to reset tds connection

At the moment babelfish doesn't support sys.sp_reset_connection to reset the connection state. With this change it supports reset connection stored procedure.
This change is required for supporting RDS Proxy for Babelfish. RDS Proxy calls sys.sp_reset_connection before connection is reused by other clients.

Issues Resolved

BABEL-429

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.

coveralls · 2024-07-19T15:00:18Z

Pull Request Test Coverage Report for Build 10990507023

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

16 of 16 (100.0%) changed or added relevant lines in 3 files are covered.
1654 unchanged lines in 9 files lost coverage.
Overall coverage increased (+0.03%) to 74.465%

Files with Coverage Reduction	New Missed Lines	%
contrib/babelfishpg_tsql/src/pltsql_bulkcopy.c	4	91.57%
contrib/babelfishpg_tsql/src/pltsql_utils.c	37	91.93%
contrib/babelfishpg_tsql/src/multidb.c	91	80.03%
contrib/babelfishpg_tsql/src/dbcmds.c	104	75.15%
contrib/babelfishpg_tsql/src/rolecmds.c	115	83.52%
contrib/babelfishpg_tsql/src/pl_exec-2.c	122	81.64%
contrib/babelfishpg_tsql/src/catalog.c	228	87.18%
contrib/babelfishpg_tsql/src/hooks.c	285	84.2%
contrib/babelfishpg_tsql/src/pl_exec.c	668	44.2%

Totals
Change from base Build 10937107580:	0.03%
Covered Lines:	44939
Relevant Lines:	60349

💛 - Coveralls

test/JDBC/input/storedProcedures/Test-sp_reset_connection.sql

contrib/babelfishpg_tds/src/backend/tds/tdsprotocol.c

thephantomthief · 2024-07-25T06:31:24Z

contrib/babelfishpg_tds/src/backend/tds/tdsprotocol.c

@@ -276,6 +277,7 @@ GetTDSRequest(bool *resetProtocol)
 			ResetTDSConnection();
 			TdsErrorContext->err_text = "Fetching TDS Request";
 			*resetProtocol = true;
+			reset_tds_connection_flag = false;


Does it make sense to also put this in the catch block?

On error connection is not reset so we should not set the flag to false. right ?
otherwise TDS request after error will reuse the old connection resources ?

contrib/babelfishpg_tsql/src/procedures.c

tanscorpio7 · 2024-08-02T09:06:43Z

contrib/babelfishpg_tds/src/backend/tds/tdsprotocol.c

@@ -71,6 +71,8 @@ typedef ResetConnectionData *ResetConnection;
 * Local structures
 */
 TdsRequestCtrlData *TdsRequestCtrl = NULL;
+bool resetTdsConnectionFlag;


Should we initialise resetTdsConnectionFlag as false, that is on backend startup ?

Lets initialize both vars

KushaalShroff · 2024-08-05T06:42:09Z

test/dotnet/input/Storedproc/TestStoredProcedure.txt

+SELECT * from #babel_temp_table
+storedproc#!#prep#!#sys.sp_reset_connection#!#
+SELECT * from #babel_temp_table


This is not actually testing the RPC in this connection, can we also have an RPC after storedproc?

KushaalShroff · 2024-08-05T06:43:58Z

contrib/babelfishpg_tds/src/backend/tds/tdsprotocol.c

@@ -71,6 +71,8 @@ typedef ResetConnectionData *ResetConnection;
 * Local structures
 */
 TdsRequestCtrlData *TdsRequestCtrl = NULL;
+bool resetTdsConnectionFlag;


Lets initialize both vars

contrib/babelfishpg_tds/src/include/tds_protocol.h

… procedure

test/JDBC/expected/Test-sp_reset_connection-vu-verify.out

KushaalShroff · 2024-09-09T08:36:15Z

test/dotnet/input/Storedproc/TestStoredProcedure.txt

+# Test (24): End
+
+# Test (25): Test sys.sp_reset_connection stored procedure


create a new test file for sp_reset_connection

KushaalShroff · 2024-09-09T08:36:54Z

test/dotnet/input/Storedproc/TestStoredProcedure.txt

+INSERT INTO #babel_temp_table (Data) VALUES (100), (200), (300)
+SELECT * from #babel_temp_table
+storedproc#!#prep#!#sys.sp_reset_connection#!#


We must add tests for sp_prepexec, sp_execute as well. This test only uses sp_customtype

KushaalShroff · 2024-09-09T10:01:29Z

contrib/babelfishpg_tds/src/backend/tds/tdsprotocol.c

+/*
+ * SetResetTDSConnectionFlag - Sets reset tds connection flag
+ */
+void SetResetTDSConnectionFlag()


On diving deep, Found 2 behaviour differences:

sys.sp_reset_connection should not send Env change token and TDSResetConnection should send

Isolation levels are reset for sp_reset_connection but retained for TDSResetConnection

We have not fixed this for sp_reset_connection and this is paramount for feature complete. Lets focus on testing and aligning TSQL Behaviour and using wireshark

contrib/babelfishpg_tds/src/backend/tds/tdsprotocol.c

KushaalShroff · 2024-09-23T04:55:39Z

test/JDBC/expected/Test-sp_reset_connection.out

+
+EXEC sys.sp_reset_connection
+-- TODO: GUC is not resetting
+SELECT @@lock_timeout;


I'm assuming the default value was 0. How are we really testing the reset in this case?

KushaalShroff · 2024-09-23T04:57:14Z

test/JDBC/expected/Test-sp_reset_connection.out

+~~END~~
+
+DECLARE @handle int;
+EXEC SP_PREPARE @handle output, NULL, N'exec sys.sp_reset_connection'


From my previous comments, we have still not added RPC tests

KushaalShroff · 2024-09-23T04:57:38Z

test/JDBC/expected/Test-sp_reset_connection.out

+~~END~~
+
+DECLARE @handle int;
+EXEC SP_PREPARE @handle output, NULL, N'exec sys.sp_reset_connection'


From my previous comments, we have still not added RPC tests

Okay I see the these are added in .net

Approved

…or-postgresql#2758) Support sys.sp_reset_connection store procedure to reset tds connection 1. At the moment babelfish doesn't support sys.sp_reset_connection to reset the connection state. With this change it supports reset connection stored procedure. 2. This change is required for supporting RDS Proxy for Babelfish. RDS Proxy calls sys.sp_reset_connection before connection is reused by other clients. Tasks: BABEL-429 Signed off by: Sharath BP <[email protected]> (cherry picked from commit 3997e2e)

sharathbp changed the title ~~Adding support for sp_reset_connection stored procedure~~ Adding support for sys.sp_reset_connection stored procedure Jul 19, 2024

sharathbp force-pushed the babel-429-rds-proxy-support branch from dffe0b3 to d1df741 Compare July 19, 2024 14:15

sharathbp changed the title ~~Adding support for sys.sp_reset_connection stored procedure~~ Add support for sys.sp_reset_connection stored procedure Jul 19, 2024

sharathbp force-pushed the babel-429-rds-proxy-support branch from d1df741 to e048c94 Compare July 24, 2024 06:30

thephantomthief reviewed Jul 25, 2024

View reviewed changes

sharathbp requested a review from kuntalghosh July 25, 2024 06:36

sharathbp force-pushed the babel-429-rds-proxy-support branch 6 times, most recently from 7d70d95 to 933cbae Compare August 2, 2024 09:06

tanscorpio7 suggested changes Aug 2, 2024

View reviewed changes

sharathbp force-pushed the babel-429-rds-proxy-support branch 3 times, most recently from 6eea9b8 to 60589ef Compare August 4, 2024 16:49

sharathbp requested review from tanscorpio7 and thephantomthief August 5, 2024 05:33

KushaalShroff requested changes Aug 5, 2024

View reviewed changes

sharathbp force-pushed the babel-429-rds-proxy-support branch from ec430c1 to 344acb5 Compare August 20, 2024 11:29

sharathbp closed this Aug 20, 2024

sharathbp force-pushed the babel-429-rds-proxy-support branch from 344acb5 to a66de39 Compare August 20, 2024 11:33

Add support for sys.sp_reset_connection stored procedure

c561179

sharathbp reopened this Aug 20, 2024

Update upgrade path sql to incorporate sys.sp_reset_connection stored…

7b7a8ea

… procedure

sharathbp force-pushed the babel-429-rds-proxy-support branch 2 times, most recently from 8107191 to fa5c6dc Compare August 20, 2024 16:26

Add Test-sp_reset_connection to upgrade tests

4ea70d8

sharathbp force-pushed the babel-429-rds-proxy-support branch from fa5c6dc to 4ea70d8 Compare August 20, 2024 17:00

Sharath BP added 3 commits August 21, 2024 07:32

Add upgrade test for sp_reset_connection

b68575c

Added upgrade test for sp_reset_connection

1d4a9fb

Add upgrade test for sp_reset_connection

d92fcd8

tanscorpio7 approved these changes Sep 3, 2024

View reviewed changes

thephantomthief approved these changes Sep 5, 2024

View reviewed changes

Deepesh125 previously requested changes Sep 6, 2024

View reviewed changes

test/JDBC/expected/Test-sp_reset_connection-vu-verify.out Outdated Show resolved Hide resolved

KushaalShroff requested changes Sep 9, 2024

View reviewed changes

sharathbp requested a review from shardgupta September 11, 2024 05:34

Sharath BP added 9 commits September 19, 2024 04:24

Add sp_reset_connection tests

3acb08d

Fix tds protocol.c

f8a0f79

move sp_reset_connection test to separate file

0690b94

backmerge with babel_4x_dev

fa16724

Add test cases

0f3c8ea

Ignore upgrade tests for sp_reset_connection

dec89b0

Ignore upgrade tests for sp_reset_connection

f41dec1

Remove sp_reset_connection from upgrade test

711a197

Remove sp_reset_connection from upgrade test

5bd9f96

KushaalShroff requested changes Sep 23, 2024

View reviewed changes

Merge with origin

19fe321

KushaalShroff approved these changes Sep 25, 2024

View reviewed changes

KushaalShroff merged commit 3997e2e into babelfish-for-postgresql:BABEL_4_X_DEV Sep 25, 2024
46 checks passed

shardgupta deleted the babel-429-rds-proxy-support branch September 25, 2024 06:21

tanscorpio7 mentioned this pull request Oct 12, 2024

Implement sp_reset_connection system procedure for connection pooling #2136

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sys.sp_reset_connection stored procedure #2758

Add support for sys.sp_reset_connection stored procedure #2758

sharathbp commented Jul 19, 2024 •

edited by Deepesh125

Loading

coveralls commented Jul 19, 2024 •

edited

Loading

thephantomthief Jul 25, 2024

tanscorpio7 Aug 2, 2024

tanscorpio7 Aug 2, 2024

KushaalShroff Aug 5, 2024

KushaalShroff Aug 5, 2024

KushaalShroff Aug 5, 2024

KushaalShroff Sep 9, 2024

KushaalShroff Sep 9, 2024

KushaalShroff Sep 9, 2024

KushaalShroff Sep 23, 2024

KushaalShroff Sep 23, 2024

KushaalShroff Sep 23, 2024

KushaalShroff Sep 23, 2024

		# Test (24): End

		# Test (25): Test sys.sp_reset_connection stored procedure

Add support for sys.sp_reset_connection stored procedure #2758

Add support for sys.sp_reset_connection stored procedure #2758

Conversation

sharathbp commented Jul 19, 2024 • edited by Deepesh125 Loading

Description

Issues Resolved

Test Scenarios Covered

Check List

coveralls commented Jul 19, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10990507023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharathbp commented Jul 19, 2024 •

edited by Deepesh125

Loading

coveralls commented Jul 19, 2024 •

edited

Loading