Don't drop destination table when non-transactional insert fails

[lhofhansl]

Description

See #12225

Is this change a fix, improvement, new feature, refactoring, or other?

Fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Most JDBC connectors will have this issue. Verified with Postgres.

How would you describe this change to a non-technical end user or system administrator?

Trino will happily drop your entire table if it encounters an error during a non-transactional insert.
This is a pretty SERIOUS issue.

Related issues, pull requests, and links

Fixes #12225

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

[lhofhansl]

Simple fix. I'll think on a quick test too (very limited time, though.)

[lhofhansl]

Haven't run tests in a while. Always fails with docker-machine executable was not found on PATH ... (This is a Fedora 34 box)

[lhofhansl]

Something like this. But I cannot run the tests locally ATM.

[findepi]

This should be a logic of jdbcClient.rollbackCreateTable
since isNonTransactionalInsert is handled by jdbcClient.beginInsertTable

(eg imagine a connector which overrides jdbcClient.beginInsertTable and ignores isNonTransactionalInsert session property; it would have no way to behave correctly)

[lhofhansl]

What if someone calls jdbcClient.rollbackCreateTable in some other context? In that case we'd want to remove that table regardless of whether inserts are transactional or not.

Edit: I guess they'd have to get it right in jdbcClient.

[lhofhansl]

Seem it's wrong either way:
DefaultJdbcMetadata.beginCreateTable also sets up jdbcClient.rollbackCreateTable as rollback action. In that case we always want the rollback to happen.
For DefaultJdbcMetadata.beginInsert we want the rollback handled conditionally.

[findepi]

i think JdbcOutputTableHandle needs to track information whether this is a temp table or not.

[lhofhansl]

Yep. That's would work.
I think one can also argue if there's code in a JdbcClient that creates a table it wants deleted then it should setup up the rollback action (not DefaultJdbcMetadata).

[lhofhansl]

Actually... Even that would not work (nicely). It would need to be a specific flag for isNonTransactionalInsert.
All other table creation (that we want to have removed) have to be marked as temporary. Now you have remember explicitly not do that, seems more frail to me. IMHO this has to be connected to an action, not the OutputTable.

I think my initial fix is still the safest. BaseJdbcClient.beginInsertTable is not overriden (except for disabling via throwing an Exception). And when one overrides that, one would have to aware of the implications. One could put a comment on that method.

As is, this will happily drop your TB size table if you fail to insert into it on non-transactional mode. :(

IMHO, the damage is done in the original implementation that separated table creation from the responsibility to clean the table up into two different classes. It's not immediately clear how to undo that.

[findepi]

I think my initial fix is still the safest.

it spreads responsibility between the two classes: DefaultJdbcMetadata (which generally should not be overridden) and JdbcClient which is the extension point for all the jdbc-based connectors

All other table creation (that we want to have removed) have to be marked as temporary. Now you have remember explicitly not do that, seems more frail to me

I thought about adding new parameter to JdbcOutputTableHandle. You cannot forget about it, because it's a required parameter.

However, we don't need a new parameter, we should just compare JdbcOutputTableHandle#tableName and JdbcOutputTableHandle#temporaryTableName. Or, better, make temporaryTableName Optional.

[lhofhansl]

it spreads responsibility between the two classes

That already happened because BaseJdbcClient creates the tables, but DefaultJdbcMetadata is responsible for the cleanup.

I thought about adding new parameter to JdbcOutputTableHandle. You cannot forget about it, because it's a required parameter.

I started with that, easy enough to do. But I ran into the issue I mentioned above.

compare JdbcOutputTableHandle#tableName and JdbcOutputTableHandle#temporaryTableName

So from looking BaseJdbcClient.beginInsertTable you are suggesting that DefaultJdbcMetadata.beginInsert to set up the rollback only handler when tableName and temporaryTableName are identical? (real tables seem to have these two set differently)
Or that the rollback handler not do anything when the tableName and temporaryTableName are identical? (that would seem weird)
Won't any derived beginInsertTable now equally have to be aware of that or have the table removed?

Happy to do that as long as we can fix the problem.

[lhofhansl]

How about this?

[lhofhansl]

We should get this into 380. This is a serious bug that will delete data unexpectedly.

@findepi @martint

[findepi]

Thanks for improving the logic, but this doesn't change the fact the logic belongs to jdbcClient.rollbackCreateTable, as it is coupled with jdbcClient.beginInsertTable.

As suggested in a1f6ada#r866674664, let's make temporaryTableName Optional.

[lhofhansl]

Edit: Sorry, I get what you meant now.

I pushed a new update. (Will rename squash/rename the commits later).

Not sure I like it.
Perhaps I could abstract the repeated handle.getTemporaryTableName().orElse(handle.getTableName()) into JdbcOutputTableHanlde itself, and add a separate method to check whether a temporary table has been provided.

[lhofhansl]

Test failures look unrelated.

[findepi]

(changed commit title and applied small style changes)

[lhofhansl]

As usually, thank for the thorough review.