Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for DROP SCHEMA CASCADE in JDBC connectors #18305

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Jul 14, 2023

Description

Relates to #17649

Release notes

(x) Release notes are required, with the following suggested text:

# ClickHouse, MariaDB, MySQL, Oracle, SingleStore
* Add support for `CASCADE` option in `DROP SCHEMA` statement. ({issue}`18305`)

@cla-bot cla-bot bot added the cla-signed label Jul 14, 2023
@ebyhr ebyhr mentioned this pull request Jul 14, 2023
9 tasks
@ebyhr
Copy link
Member Author

ebyhr commented Jul 18, 2023

TestPostgreSqlConnectorTest.testSelectInformationSchemaColumns failed though the test succeeded on the 2nd attempt. Going to remove support for PostgreSQL and Redshift from this PR.

Error:  Failures: 
Error:    TestPostgreSqlConnectorTest>BaseConnectorTest.testSelectInformationSchemaColumns:2244->AbstractTestQueryFramework.assertQuery:339 Execution of 'actual' query 20230715_051130_02304_yh37n failed: SELECT table_name, column_name FROM information_schema.columns WHERE table_catalog = 'postgresql' AND table_schema = 'tpch' AND table_name LIKE '_rders'

The cause might be concurrent set column type tests.

2023-07-14T23:11:31.115-0600	INFO	pool-5-thread-2	io.trino.testng.services.ProgressLoggingListener	[TEST SUCCESS] io.trino.plugin.postgresql.TestPostgreSqlConnectorTest.testSetColumnTypeWithDefaultColumn; (took: 0.2 seconds)
2023-07-14T23:11:31.115-0600	INFO	pool-5-thread-2	io.trino.testng.services.ProgressLoggingListener	[TEST START] io.trino.plugin.postgresql.TestPostgreSqlConnectorTest.testSetColumnTypeWithNotNull
2023-07-14T23:11:31.116-0600	ERROR	SplitRunner-9-172	io.trino.execution.executor.TaskExecutor	Error processing Split 20230715_051130_02304_yh37n.0.0.0-0 io.trino.connector.informationschema.InformationSchemaSplit@7259262 (start = 526942.025557, wall = 42 ms, cpu = 0 ms, wait = 0 ms, calls = 1): JDBC_ERROR: Error listing table columns for catalog postgresql: ERROR: could not open relation with OID 18382

https://github.com/trinodb/trino/actions/runs/5559234810/jobs/10156058413?pr=18305

@ebyhr ebyhr marked this pull request as draft July 19, 2023 01:45
@ebyhr ebyhr force-pushed the ebi/jdbc-drop-schema-cascade branch from 7ec83fb to b2d1610 Compare August 3, 2023 08:54
@ebyhr ebyhr marked this pull request as ready for review August 3, 2023 08:54
@ebyhr ebyhr requested review from wendigo and hashhar August 3, 2023 08:55
@ebyhr ebyhr force-pushed the ebi/jdbc-drop-schema-cascade branch from b2d1610 to ff2400f Compare August 3, 2023 08:56
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good.

throws SQLException
{
// ClickHouse always deletes all tables inside the database https://clickhouse.com/docs/en/sql-reference/statements/drop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to prevent this if cascade was not specified? (and in MySQL, MariaDB and Singlestore too).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean throwing an exception when !cascade? It means users always need to specify cascade option even when the target database is empty, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was thinking of would require a list operation - and even then there can be races.
I don't know a good solution. My main concern is that if someone does a DROP SCHEMA (without CASCADE) they may not expect the operation to actually CASCADE to all objects in the schema.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the test and manually tested without the same check in DropSchemaTask.

@hashhar hashhar requested a review from Praveen2112 August 3, 2023 12:20
@ebyhr ebyhr force-pushed the ebi/jdbc-drop-schema-cascade branch from 6803a6d to 64c57c2 Compare August 8, 2023 09:30
@ebyhr ebyhr requested a review from hashhar August 10, 2023 05:51
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Squash fixups.

@ebyhr ebyhr force-pushed the ebi/jdbc-drop-schema-cascade branch from 64c57c2 to dfaced4 Compare August 10, 2023 08:59
@ebyhr ebyhr merged commit 5fba447 into trinodb:master Aug 10, 2023
@ebyhr ebyhr deleted the ebi/jdbc-drop-schema-cascade branch August 10, 2023 09:00
@github-actions github-actions bot added this to the 423 milestone Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants