Reset cross-db flag for each statement #3237

HarshLunagariya · 2024-12-11T14:11:18Z

Description

Earlier, while parsing the batch, We were marking the all the subsequent statements of the batch as cross-database statements once we encounter any cross-database statement in batch which was incorrect.

This commit fixes above behaviour by resetting 'is_cross_db' maintained at mutator level once we mark some statement as cross_db in the parse tree.

Issues Resolved

BABEL-5447

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.

Signed-off-by: Harsh Lunagariya <[email protected]>

coveralls · 2024-12-11T14:49:10Z

Pull Request Test Coverage Report for Build 12408305896

Details

2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 74.823%

Totals
Change from base Build 12395755914:	0.0%
Covered Lines:	46478
Relevant Lines:	62117

💛 - Coveralls

Signed-off-by: Harsh Lunagariya <[email protected]>

shalinilohia50

LGTM!

rishabhtanwar29 · 2024-12-13T04:49:58Z

test/JDBC/expected/BABEL-CROSS-DB-vu-verify.out

+GO
+
+-- Batches having system objects
+BEGIN; SELECT DISTINCT 1 FROM my_babel_cross_db_vu_prepare_db1.dbo.sysdatabases; BEGIN; SELECT DISTINCT 1 FROM babel_cross_db_vu_prepare_master_t1; END; END;


Let's also add test where we call sys objects as well like db1.sys.databases etc.

Create temp tables after executing cross-db stmt as DDL permissions work differently.

Also let's include below cases:

tests for statements in batch inside a transaction.

some cases where login/user has some default db and schemas with it and it's executing one cross-db query with other queries in a batch.

Let's also add test where we call sys objects as well like db1.sys.databases etc.

Create temp tables after executing cross-db stmt as DDL permissions work differently.

Done.

Also let's include below cases:

tests for statements in batch inside a transaction.

some cases where login/user has some default db and schemas with it and it's executing one cross-db query with other queries in a batch.

Regarding, 2nd point - not adding anything for default db setting since it doesn't affect anything while query execution. It is only utilised while setting up a new db session.

regarding default_schema, It is being tracked by BABEL-5050, needs separate fix.

anju15bharti · 2024-12-13T05:44:01Z

test/JDBC/expected/BABEL-CROSS-DB-vu-verify.out

+GO
+
+-- Batches having system objects
+BEGIN; SELECT DISTINCT 1 FROM my_babel_cross_db_vu_prepare_db1.dbo.sysdatabases; BEGIN; SELECT DISTINCT 1 FROM babel_cross_db_vu_prepare_master_t1; END; END;


Also let's include below cases:

tests for statements in batch inside a transaction.

some cases where login/user has some default db and schemas with it and it's executing one cross-db query with other queries in a batch.

Signed-off-by: Harsh Lunagariya <[email protected]>

tanscorpio7 · 2024-12-16T06:54:17Z

contrib/babelfishpg_tsql/src/tsqlIface.cpp

 			stmt->is_cross_db = true;
+			is_cross_db = false;


We are also setting this flag to true in makeExecuteProcedure but not resetting it back. What would happen if we exec a batch like

EXEC db.dbo.p SELECT * FROM t

can we reset is_cross_db in enterDMLStatement ?

Also seems like we can just remove use of this flag from makeExecuteProcedure

Also seems like we can just remove use of this flag from makeExecuteProcedure

Yes, I noticed it too. On top of that we can cleanup some logic from pl_exec as well. We should track it separately because I assume there we will need more test coverage, can't just rely on existing tests to conclude that given logic is redundant.

can we reset is_cross_db in enterDMLStatement ?

Done.

Signed-off-by: Harsh Lunagariya <[email protected]>

rishabhtanwar29 · 2024-12-19T06:49:04Z

test/JDBC/expected/BABEL-CROSS-DB-vu-verify.out

+~~ROW COUNT: 4~~
+
+
+SELECT * FROM my_babel_cross_db_vu_prepare_db1.dbo.babel_cross_db_vu_prepare_db1_t1;


This one needs an ORDER BY clause or else it will be flaky.

rishabhtanwar29 · 2024-12-19T06:49:42Z

test/JDBC/expected/BABEL-CROSS-DB-vu-verify.out

+~~ROW COUNT: 1~~
+
+
+SELECT * FROM my_babel_cross_db_vu_prepare_db1.dbo.babel_cross_db_vu_prepare_db1_t1;


This one also needs an ORDER BY clause.

rishabhtanwar29 · 2024-12-19T06:49:58Z

test/JDBC/expected/BABEL-CROSS-DB-vu-verify.out

+~~ROW COUNT: 1~~
+
+
+SELECT * FROM dbo.my_babel_cross_db_vu_prepare_db1_t2;


Same here, needs ORDER BY.

Signed-off-by: Harsh Lunagariya <[email protected]>

Earlier, while parsing the batch, We were marking the all the subsequent statements of the batch as cross-database statements once we encounter any cross-database statement in batch which was incorrect. This commit fixes above behaviour by resetting 'is_cross_db' maintained at mutator level once we mark some statement as cross_db in the parse tree. Task: BABEL-5447 Signed-off-by: Harsh Lunagariya <[email protected]>

Earlier, while parsing the batch, We were marking the all the subsequent statements of the batch as cross-database statements once we encounter any cross-database statement in batch which was incorrect. This commit fixes above behaviour by resetting 'is_cross_db' maintained at mutator level once we mark some statement as cross_db in the parse tree. Cherry-pick of d2e7653 Task: BABEL-5447 Signed-off-by: Harsh Lunagariya <[email protected]>

Reset cross-db flag for each statement

4e00c8c

Signed-off-by: Harsh Lunagariya <[email protected]>

Add tests

168aeb0

Signed-off-by: Harsh Lunagariya <[email protected]>

HarshLunagariya force-pushed the babel_cbd_imp branch from 84bc298 to 168aeb0 Compare December 13, 2024 04:12

HarshLunagariya changed the title ~~[DO NOT MERGE] Reset cross-db flag for each statement~~ Reset cross-db flag for each statement Dec 13, 2024

shalinilohia50 approved these changes Dec 13, 2024

View reviewed changes

rishabhtanwar29 reviewed Dec 13, 2024

View reviewed changes

anju15bharti suggested changes Dec 13, 2024

View reviewed changes

more tests

b20fa79

Signed-off-by: Harsh Lunagariya <[email protected]>

tanscorpio7 suggested changes Dec 16, 2024

View reviewed changes

address comments

7a6c570

Signed-off-by: Harsh Lunagariya <[email protected]>

HarshLunagariya force-pushed the babel_cbd_imp branch from d931262 to 7a6c570 Compare December 17, 2024 10:33

rishabhtanwar29 previously approved these changes Dec 17, 2024

View reviewed changes

tanscorpio7 approved these changes Dec 17, 2024

View reviewed changes

HarshLunagariya dismissed rishabhtanwar29’s stale review via decfd2d December 18, 2024 14:15

More changes

8ca2971

Signed-off-by: Harsh Lunagariya <[email protected]>

HarshLunagariya force-pushed the babel_cbd_imp branch from decfd2d to 8ca2971 Compare December 18, 2024 16:20

KushaalShroff previously approved these changes Dec 18, 2024

View reviewed changes

rishabhtanwar29 reviewed Dec 19, 2024

View reviewed changes

improve tests

c72b9ca

Signed-off-by: Harsh Lunagariya <[email protected]>

HarshLunagariya dismissed KushaalShroff’s stale review via c72b9ca December 19, 2024 07:37

rishabhtanwar29 approved these changes Dec 19, 2024

View reviewed changes

rishabhtanwar29 merged commit d2e7653 into babelfish-for-postgresql:BABEL_5_X_DEV Dec 19, 2024
44 checks passed

rishabhtanwar29 deleted the babel_cbd_imp branch December 19, 2024 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset cross-db flag for each statement #3237

Reset cross-db flag for each statement #3237

HarshLunagariya commented Dec 11, 2024 •

edited

Loading

coveralls commented Dec 11, 2024 •

edited

Loading

shalinilohia50 left a comment

rishabhtanwar29 Dec 13, 2024

anju15bharti Dec 13, 2024 •

edited

Loading

HarshLunagariya Dec 16, 2024

HarshLunagariya Dec 17, 2024

anju15bharti Dec 13, 2024 •

edited

Loading

tanscorpio7 Dec 16, 2024

tanscorpio7 Dec 16, 2024 •

edited

Loading

tanscorpio7 Dec 16, 2024

HarshLunagariya Dec 16, 2024 •

edited

Loading

rishabhtanwar29 Dec 19, 2024

rishabhtanwar29 Dec 19, 2024

rishabhtanwar29 Dec 19, 2024

		~~ROW COUNT: 4~~


		SELECT * FROM my_babel_cross_db_vu_prepare_db1.dbo.babel_cross_db_vu_prepare_db1_t1;

		~~ROW COUNT: 1~~


		SELECT * FROM my_babel_cross_db_vu_prepare_db1.dbo.babel_cross_db_vu_prepare_db1_t1;

		~~ROW COUNT: 1~~


		SELECT * FROM dbo.my_babel_cross_db_vu_prepare_db1_t2;

Reset cross-db flag for each statement #3237

Reset cross-db flag for each statement #3237

Conversation

HarshLunagariya commented Dec 11, 2024 • edited Loading

Description

Issues Resolved

Test Scenarios Covered

Check List

coveralls commented Dec 11, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12408305896

Details

💛 - Coveralls

shalinilohia50 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anju15bharti Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anju15bharti Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanscorpio7 Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshLunagariya Dec 16, 2024 • 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

HarshLunagariya commented Dec 11, 2024 •

edited

Loading

coveralls commented Dec 11, 2024 •

edited

Loading

anju15bharti Dec 13, 2024 •

edited

Loading

anju15bharti Dec 13, 2024 •

edited

Loading

tanscorpio7 Dec 16, 2024 •

edited

Loading

HarshLunagariya Dec 16, 2024 •

edited

Loading