Add rollback action for CTAS destination table by findinpath · Pull Request #27702 · trinodb/trino

findinpath · 2025-12-19T13:28:42Z

Description

While CREATE TABLE is an atomic operation, CREATE TABLE AS SELECT statement happens in two phases
for JDBC connectors from Trino:

creating the table
inserting content into the table

If the insertion of the data fails, the CTAS may leave residual empty tables behind.

The JDBC connectors, in dealing with CREATE TABLE AS SELECT scenarios,
remove the destination table in case of dealing with exceptions
while inserting the content.

NOTE that introducing the rollbackConsumer in the beginCreateTable method may come with potential changes in the functionality of the connectors that extend from trino-base-jdbc module.

Additional context and related issues

Here is a relevant stacktrace gathered from CI that showcases the issue of leaving behind empty tables after a CTAS

Error:  ....plugin.redshift.TestRedshiftConnectorSmokeTest.testRowLevelDelete -- Time elapsed: 13.15 s <<< ERROR!
io.trino.testing.QueryFailedException: line 1:1: Destination table 'redshift.test_schema.test_row_level_deletepbx2vn9g2n' already exists
	at io.trino.testing.AbstractTestingTrinoClient.execute(AbstractTestingTrinoClient.java:138)
	at io.trino.testing.DistributedQueryRunner.executeInternal(DistributedQueryRunner.java:645)
	at io.trino.testing.DistributedQueryRunner.execute(DistributedQueryRunner.java:621)
	at io.trino.plugin.redshift.TrinoSqlExecutorWithRetries.lambda$execute$1(TrinoSqlExecutorWithRetries.java:52)
	at dev.failsafe.Functions.lambda$toCtxSupplier$9(Functions.java:228)
	at dev.failsafe.Functions.lambda$get$0(Functions.java:46)
	at dev.failsafe.internal.RetryPolicyExecutor.lambda$apply$0(RetryPolicyExecutor.java:74)
	at dev.failsafe.SyncExecutionImpl.executeSync(SyncExecutionImpl.java:187)
	at dev.failsafe.FailsafeExecutor.call(FailsafeExecutor.java:376)
	at dev.failsafe.FailsafeExecutor.run(FailsafeExecutor.java:220)
	at io.trino.plugin.redshift.TrinoSqlExecutorWithRetries.execute(TrinoSqlExecutorWithRetries.java:52)
	at io.trino.testing.sql.TestTable.createAndInsert(TestTable.java:52)
	at io.trino.testing.sql.TestTable.<init>(TestTable.java:47)
	at io.trino.plugin.redshift.TestRedshiftConnectorSmokeTest.newTrinoTable(TestRedshiftConnectorSmokeTest.java:47)
	at io.trino.testing.AbstractTestQueryFramework.newTrinoTable(AbstractTestQueryFramework.java:700)
	at io.trino.testing.BaseConnectorSmokeTest.testRowLevelDelete(BaseConnectorSmokeTest.java:291)
	at java.base/java.lang.reflect.Method.invoke(Method.java:565)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:511)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.tryRemoveAndExec(ForkJoinPool.java:1490)
	at java.base/java.util.concurrent.ForkJoinPool.helpJoin(ForkJoinPool.java:2248)
	at java.base/java.util.concurrent.ForkJoinTask.awaitDone(ForkJoinTask.java:499)
	at java.base/java.util.concurrent.ForkJoinTask.join(ForkJoinTask.java:666)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:511)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1450)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:2019)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:187)
	Suppressed: java.lang.Exception: SQL: CREATE TABLE test_row_level_deletepbx2vn9g2n AS SELECT * FROM region
		at io.trino.testing.DistributedQueryRunner.executeInternal(DistributedQueryRunner.java:652)
		... 24 more
Caused by: io.trino.spi.TrinoException: line 1:1: Destination table 'redshift.test_schema.test_row_level_deletepbx2vn9g2n' already exists
	at io.trino.sql.analyzer.SemanticExceptions.semanticException(SemanticExceptions.java:58)
	at io.trino.sql.analyzer.SemanticExceptions.semanticException(SemanticExceptions.java:52)
	at io.trino.sql.analyzer.StatementAnalyzer$Visitor.visitCreateTableAsSelect(StatementAnalyzer.java:960)
	at io.trino.sql.analyzer.StatementAnalyzer$Visitor.visitCreateTableAsSelect(StatementAnalyzer.java:533)
	at io.trino.sql.tree.CreateTableAsSelect.accept(CreateTableAsSelect.java:86)
	at io.trino.sql.tree.AstVisitor.process(AstVisitor.java:27)
	at io.trino.sql.analyzer.StatementAnalyzer$Visitor.process(StatementAnalyzer.java:552)
	at io.trino.sql.analyzer.StatementAnalyzer.analyze(StatementAnalyzer.java:512)
	at io.trino.sql.analyzer.StatementAnalyzer.analyze(StatementAnalyzer.java:501)
	at io.trino.sql.analyzer.Analyzer.analyze(Analyzer.java:98)
	at io.trino.sql.analyzer.Analyzer.analyze(Analyzer.java:87)
	at io.trino.execution.SqlQueryExecution.analyze(SqlQueryExecution.java:332)
	at io.trino.execution.SqlQueryExecution.<init>(SqlQueryExecution.java:241)
	at io.trino.execution.SqlQueryExecution$SqlQueryExecutionFactory.createQueryExecution(SqlQueryExecution.java:984)
	at io.trino.dispatcher.LocalDispatchQueryFactory.lambda$createDispatchQuery$0(LocalDispatchQueryFactory.java:160)
	at io.trino.$gen.Trino_testversion____20251208_175533_1.call(Unknown Source)
	at com.google.common.util.concurrent.TrustedListenableFutureTask$TrustedFutureInterruptibleTask.runInterruptibly(TrustedListenableFutureTask.java:128)
	at com.google.common.util.concurrent.InterruptibleTask.run(InterruptibleTask.java:74)
	at com.google.common.util.concurrent.TrustedListenableFutureTask.run(TrustedListenableFutureTask.java:80)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1090)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:614)
	at java.base/java.lang.Thread.run(Thread.java:1474)

Relevant code from the method testRowLevelDelete() in BaseConnectorSmokeTest.java:

trino/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java

Line 286 in d8c8057

    
           try (TestTable table = newTrinoTable("test_row_level_delete", "AS SELECT * FROM region")) {

Relevant issues and PRs

Inspired from the discussions done during #27677

Release notes

() This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

## JDBC
* Drop the table in CTAS scenario in case of an exception during data insertion phase. ({issue}`issuenumber`)

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

chenjian2664

LGTM % comments

plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteMetadata.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

While CREATE TABLE is an atomic operation, CREATE TABLE AS SELECT statement happens in two phases for JDBC connectors from Trino: - creating the table - inserting content into the table If the insertion of the data fails, the CTAS may leave residual empty tables behind. The JDBC connectors, in dealing with CREATE TABLE AS SELECT scenarios, remove the destination table in case of dealing with exceptions while inserting the content.

chenjian2664 · 2025-12-27T12:09:23Z

Adding the release note into ClickHouse, DuckDB, Ignite, MariaDB, MySQL, Oracle, PostgreSQL, Redshift, SingleStore, Snowflake, SQL Server, Vertica. let me know if you think there are connectors I should add the RN as well

findepi · 2025-12-30T17:01:47Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

        }
    }

+    protected void rollbackCreateDestinationTable(ConnectorSession session, RemoteTableName remoteTableName)


rollbackTableCreation / rollbackCreateTable

findepi · 2025-12-30T17:03:07Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

        }
-        JdbcOutputTableHandle handle = jdbcClient.beginCreateTable(session, tableMetadata);
+        JdbcOutputTableHandle handle = jdbcClient.beginCreateTable(session, tableMetadata, rollbackActions::add);
        rollbackActions.add(() -> jdbcClient.rollbackCreateTable(session, handle));


we had rollback here. do we rollback twice now?

the rollbackCreateTable is mainly for the clean ups of the temporary tables

the responsibilities are now blurred, aren't they?
maybe we can clarify them with better naming or something?

the responsibilities are now blurred, aren't they?

now, yes. But I think the name is ok since we are inside the "createTable" method, that's is fair to have a rollback method like "rollbackCreateTable", it just our implementation only do cleanups of temporary table. Ideally if we could do the cleanup of target table inside this method as well

@findinpath #27848 the method is going to split such cleanups into two methods(we already do), but finally, I think that's more elegant that we should only has one rollback methods for "createTable", caller don't care or needs to know the cleanups table is "temporary" or not

While rolling back the creation of the destination table is characteristic only for CTAS statements, rolling back the creation of a temporary table can occur on any DML statement.
I see a good rationale on having two separate methods for doing rollback for table creation.

findepi · 2025-12-30T17:04:35Z

plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteMetadata.java

            throw new TrinoException(NOT_SUPPORTED, "This connector does not support replacing tables");
        }
-        igniteClient.beginCreateTable(session, tableMetadata);
+        igniteClient.beginCreateTable(session, tableMetadata, _ -> {});


document why it's ok to ignore a rollback action
(it should be actually safer not to ignore -- this is CREATE TABLE flow. if nothing follows CREATE TABLE, nothing fails later and no rollback will take place)

findepi · 2025-12-30T17:07:25Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcClient.java

    void createTable(ConnectorSession session, ConnectorTableMetadata tableMetadata);

-    JdbcOutputTableHandle beginCreateTable(ConnectorSession session, ConnectorTableMetadata tableMetadata);
+    JdbcOutputTableHandle beginCreateTable(ConnectorSession session, ConnectorTableMetadata tableMetadata, Consumer<Runnable> rollbackActionConsumer);


rollbackActionCollector would be a better name

more importantly, this is a rather odd abstraction and doesn't seem necessary.
so far it was JdbcMetadata who was respomnsible for instantiating and remembering rollback actions. let's not change that, if we don't have to

rollbackActionCollector would be a better name

+1

so far it was JdbcMetadata who was respomnsible for instantiating and remembering rollback actions

We are entering a situation where it is unclear when it is safe to roll back the target table during CTAS. For example, in BaseJdbcClient#beginCreateTable, when FTE is enabled, both the target table and a temporary table are created in the method, during rollback in DefaultJdbcMetadata, we cannot reliably determine which tables should be cleaned up

let's not change that, if we don't have to

For now, this works as a solution, but I believe we could evolve the API to shift the responsibility back to DefaultJdbcMetadata. For example, we could change the method's return value so that DefaultJdbcMetadata knows exactly which tables need to be cleaned up

rollbackActionCollector would be a better name

+1

Included in

Test JDBC connector DDL/DML rollback with PostgreSQL #27794

however, the name may still be imperfect -- #27702 (comment)

in BaseJdbcClient#beginCreateTable, when FTE is enabled, both the target table and a temporary table are created in the method

I made the same mistake. It's not FTE-related despite the code making it look so.

during rollback in DefaultJdbcMetadata, we cannot reliably determine which tables should be cleaned up

It's unclear why destination table is created there. I think it might be possible to defer that statement until later.

let's not change that, if we don't have to

For now, this works as a solution, but I believe we could evolve the API to shift the responsibility back to DefaultJdbcMetadata. For example, we could change the method's return value so that DefaultJdbcMetadata knows exactly which tables need to be cleaned up

That was also my intention.
I stopped short from execution on it in

Test JDBC connector DDL/DML rollback with PostgreSQL #27794

when i saw the beginMerge flow. We create high number of tables there, and someone needs to clean them up if we fail mid-way.

findinpath · 2026-01-05T10:34:38Z

Following up the open discussions in #27848

cla-bot bot added the cla-signed label Dec 19, 2025

findinpath mentioned this pull request Dec 19, 2025

Testing: Ensure in Redshift before retrying a CTAS statement that the table gets dropped #27677

Closed

findepi reviewed Dec 19, 2025

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

findinpath force-pushed the findinpath/retry-on-drop-table branch 2 times, most recently from 022746f to d099899 Compare December 19, 2025 15:57

findinpath changed the title ~~Retry dropping a table~~ Add rollback action for CTAS destination table Dec 19, 2025

findinpath requested review from Praveen2112, ebyhr and findepi December 19, 2025 15:58

chenjian2664 reviewed Dec 20, 2025

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java Outdated Show resolved Hide resolved

chenjian2664 reviewed Dec 21, 2025

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

findinpath force-pushed the findinpath/retry-on-drop-table branch from d099899 to fb25fcf Compare December 23, 2025 07:24

github-actions bot added druid Druid connector exasol Exasol connector ignite Ignite connector sqlserver SQLServer connector labels Dec 23, 2025

findinpath requested a review from chenjian2664 December 23, 2025 07:26

chenjian2664 reviewed Dec 23, 2025

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

findinpath force-pushed the findinpath/retry-on-drop-table branch from fb25fcf to b3142de Compare December 23, 2025 08:33

chenjian2664 reviewed Dec 23, 2025

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

findinpath force-pushed the findinpath/retry-on-drop-table branch from b3142de to 540828e Compare December 23, 2025 08:46

github-actions bot added mysql MySQL connector postgresql PostgreSQL connector labels Dec 23, 2025

findinpath requested a review from chenjian2664 December 23, 2025 08:47

chenjian2664 reviewed Dec 23, 2025

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

findinpath force-pushed the findinpath/retry-on-drop-table branch from 540828e to a1e5a0c Compare December 23, 2025 11:06

findinpath requested a review from chenjian2664 December 23, 2025 11:07

chenjian2664 approved these changes Dec 23, 2025

View reviewed changes

plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteMetadata.java Show resolved Hide resolved

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java Show resolved Hide resolved

chenjian2664 reviewed Dec 23, 2025

View reviewed changes

plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteClient.java Outdated Show resolved Hide resolved

findinpath force-pushed the findinpath/retry-on-drop-table branch from a1e5a0c to 88664e3 Compare December 23, 2025 15:13

findinpath requested a review from chenjian2664 December 23, 2025 15:14

chenjian2664 approved these changes Dec 24, 2025

View reviewed changes

plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteClient.java Outdated Show resolved Hide resolved

findinpath force-pushed the findinpath/retry-on-drop-table branch from 88664e3 to 5ae171b Compare December 24, 2025 07:07

ebyhr approved these changes Dec 24, 2025

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Outdated Show resolved Hide resolved

findinpath force-pushed the findinpath/retry-on-drop-table branch from 5ae171b to 007ac15 Compare December 24, 2025 07:33

ebyhr force-pushed the findinpath/retry-on-drop-table branch from 007ac15 to 112deed Compare December 24, 2025 07:58

ebyhr merged commit ac78011 into trinodb:master Dec 24, 2025
66 of 67 checks passed

github-actions bot added this to the 480 milestone Dec 24, 2025

findinpath mentioned this pull request Dec 24, 2025

Remove unnecessary DROP TABLE statement in TestIgniteTypeMapping #27755

Merged

This was referenced Dec 26, 2025

Handle concurrent DROP TABLE IF EXISTS correctly #27760

Closed

Add 480 release notes #27719

Open

findepi reviewed Dec 30, 2025

View reviewed changes

findepi mentioned this pull request Dec 30, 2025

Test JDBC connector DDL/DML rollback with PostgreSQL #27794

Merged

findinpath mentioned this pull request Jan 5, 2026

Refactorings on the JdbcClient methods #27848

Merged

Conversation

findinpath commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context and related issues

Relevant issues and PRs

Release notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chenjian2664 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chenjian2664 commented Dec 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findinpath commented Jan 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

findinpath commented Dec 19, 2025 •

edited

Loading

chenjian2664 commented Dec 27, 2025 •

edited

Loading