Skip to content

Deny DML commands within explicit transactions in Hive #10798

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.

Sign up for GitHub

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

Merged
findepi merged 1 commit into from
Jan 28, 2022
Merged

Deny DML commands within explicit transactions in Hive #10798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1624,6 +1624,9 @@ public ConnectorTableHandle beginUpdate(ConnectorSession session, ConnectorTable
if (!isFullAcidTable(table.getParameters())) {
throw new TrinoException(NOT_SUPPORTED, "Hive update is only supported for ACID transactional tables");
}
if (!autoCommit) {
Comment thread
findinpath marked this conversation as resolved.
Outdated
throw new TrinoException(NOT_SUPPORTED, "Updating transactional tables is not supported in explicit transactions (use autocommit mode)");
}

// Verify that none of the updated columns are partition columns or bucket columns

Expand Down Expand Up @@ -1723,7 +1726,9 @@ public HiveInsertTableHandle beginInsert(ConnectorSession session, ConnectorTabl
if (isTransactional && retryMode != NO_RETRIES) {
Copy link
Copy Markdown

@boson234 boson234 Aug 29, 2023

Choose a reason for hiding this comment

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

"create table as select" can work as "insert into", so the table create in transaction will still be there after rollback...

throw new TrinoException(NOT_SUPPORTED, "Inserting into Hive transactional tables is not supported with query retries enabled");
}

if (isTransactional && !autoCommit) {
Comment thread
findinpath marked this conversation as resolved.
Outdated
throw new TrinoException(NOT_SUPPORTED, "Inserting into Hive transactional tables is not supported in explicit transactions (use autocommit mode)");
}
List<HiveColumnHandle> handles = hiveColumnHandles(table, typeManager, getTimestampPrecision(session)).stream()
.filter(columnHandle -> !columnHandle.isHidden())
.collect(toImmutableList());
Expand Down Expand Up @@ -2425,6 +2430,9 @@ public ConnectorTableHandle beginDelete(ConnectorSession session, ConnectorTable
if (retryMode != NO_RETRIES) {
throw new TrinoException(NOT_SUPPORTED, "Deleting from Hive tables is not supported with query retries enabled");
}
if (!autoCommit) {
throw new TrinoException(NOT_SUPPORTED, "Deleting from Hive transactional tables is not supported in explicit transactions (use autocommit mode)");
Copy link
Copy Markdown
Member

@findepi findepi Jan 27, 2022

Choose a reason for hiding this comment

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

remove isTransactionalTable -- beginDelete won't be called for a non-acid table.

Copy link
Copy Markdown
Contributor Author

@findinpath findinpath Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I just tried the scenario of deleting from a non-transactional table that is being partitioned and this worked.

CREATE TABLE test2 (a_string varchar, a_integer int)
WITH (format = 'ORC', transactional = false, partitioned_by = ARRAY['a_integer']);

insert into test2 values ('hello', 1);
insert into test2 values ('world', 2);
insert into test2 values ('Trino', 1);

delete from test2 where a_integer =1 ;

I tend to think that it makes sense to keep the transactional table check.

Copy link
Copy Markdown
Member

@findepi findepi Jan 28, 2022

Choose a reason for hiding this comment

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

Are you saying that removing isTransactionalTable check here would break this scneario?

Copy link
Copy Markdown
Member

@losipiuk losipiuk Jan 28, 2022

Choose a reason for hiding this comment

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

The example above would call applyDelete, righ?

Copy link
Copy Markdown
Member

@findepi findepi Jan 28, 2022

Choose a reason for hiding this comment

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

@losipiuk yes

Copy link
Copy Markdown
Contributor Author

@findinpath findinpath Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

trino:default> start transaction;
START TRANSACTION
trino:default> delete from test2 where a_integer =1 ;
DELETE

Query 20220128_112521_00030_bfch6, FINISHED, 1 node
http://localhost:8080/ui/query.html?20220128_112521_00030_bfch6
Splits: 1 total, 1 done (100.00%)
CPU Time: 0.0s total,     0 rows/s,     0B/s, 100% active
Per Node: 0.0 parallelism,     0 rows/s,     0B/s
Parallelism: 0.0
Peak Memory: 116B
0.43 [0 rows, 0B] [0 rows/s, 0B/s]

trino:default> select * from test2;
 a_string | a_integer 
----------+-----------
 world    |         2 
(1 row)

Query 20220128_112531_00031_bfch6, FINISHED, 1 node
http://localhost:8080/ui/query.html?20220128_112531_00031_bfch6
Splits: 9 total, 9 done (100.00%)
CPU Time: 0.0s total,   166 rows/s,   49KB/s, 46% active
Per Node: 0.0 parallelism,     3 rows/s,   965B/s
Parallelism: 0.0
Peak Memory: 150B
0.31 [1 rows, 301B] [3 rows/s, 965B/s]

trino:default> rollback;
ROLLBACK
trino:default> select * from test2;
 a_string | a_integer 
----------+-----------
 hello    |         1 
 Trino    |         1 
 world    |         2 
(3 rows)

Query 20220128_112538_00033_bfch6, FINISHED, 1 node
http://localhost:8080/ui/query.html?20220128_112538_00033_bfch6
Splits: 11 total, 11 done (100.00%)
CPU Time: 0.0s total,   187 rows/s, 54.9KB/s, 42% active
Per Node: 0.0 parallelism,     6 rows/s, 1.92KB/s
Parallelism: 0.0
Peak Memory: 150B
0.46 [3 rows, 900B] [6 rows/s, 1.92KB/s]

I've tried this functionality on master on config-default & config-hdp3 where the check is not present.

Copy link
Copy Markdown
Contributor Author

@findinpath findinpath Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

trino:default> CREATE TABLE txtest (a_string varchar) WITH (format = 'ORC', transactional = true);
CREATE TABLE
trino:default> insert into txtest values ('a'), ('b'), ('c');
INSERT: 3 rows

Query 20220128_113930_00016_cj4we, FINISHED, 1 node
http://localhost:8080/ui/query.html?20220128_113930_00016_cj4we
Splits: 19 total, 19 done (100.00%)
CPU Time: 0.0s total,     0 rows/s,     0B/s, 7% active
Per Node: 0.0 parallelism,     0 rows/s,     0B/s
Parallelism: 0.0
Peak Memory: 416B
1.08 [0 rows, 0B] [0 rows/s, 0B/s]

trino:default> start transaction;
START TRANSACTION
trino:default> delete from txtest where a_string = 'a';
DELETE: 1 row

Query 20220128_113950_00018_cj4we, FINISHED, 1 node
http://localhost:8080/ui/query.html?20220128_113950_00018_cj4we
Splits: 10 total, 10 done (100.00%)
CPU Time: 0.1s total,    57 rows/s,   13KB/s, 50% active
Per Node: 0.1 parallelism,     6 rows/s, 1.36KB/s
Parallelism: 0.1
Peak Memory: 108B
0.50 [3 rows, 693B] [6 rows/s, 1.36KB/s]

trino:default> delete from txtest where a_string = 'b';
Query 20220128_113954_00019_cj4we failed: Tried to access metastore after transaction has been committed/aborted
java.lang.IllegalStateException: Tried to access metastore after transaction has been committed/aborted
	at io.trino.plugin.hive.metastore.SemiTransactionalHiveMetastore.checkReadable(SemiTransactionalHiveMetastore.java:2441)
	at io.trino.plugin.hive.metastore.SemiTransactionalHiveMetastore.getTable(SemiTransactionalHiveMetastore.java:217)
	at io.trino.plugin.hive.HiveMetadata.getView(HiveMetadata.java:2391)

Tested on config-hdp3 on master

Copy link
Copy Markdown
Member

@findepi findepi Jan 28, 2022

Choose a reason for hiding this comment

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

i fail to understand the response.
it would help me if you did remove the check and the CI showed the failure for me.

Copy link
Copy Markdown
Member

@losipiuk losipiuk Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@findinpath just dro the isTransactionalTable() part of check.
It is not needed. It is already checked above via ensureTableSupportsDelete()

Copy link
Copy Markdown
Member

@losipiuk losipiuk Jan 28, 2022

Choose a reason for hiding this comment

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

I would not be strongly against verify(isTransactionalTable(table.getParameters()) but it feels overchatty.

}

LocationHandle locationHandle = locationService.forExistingTable(metastore, session, table);

Expand Down
33 changes: 33 additions & 0 deletions ...o-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,6 +950,39 @@ public void testInsertOnlyMultipleWriters(boolean bucketed, Engine inserter1, En
});
}

@Test(groups = HIVE_TRANSACTIONAL)
public void testInsertFailsInExplicitTrinoTransaction()
{
withTemporaryTable("insert_fail_explicit_transaction", true, false, NONE, tableName -> {
onTrino().executeQuery(format("CREATE TABLE %s (a_string varchar) WITH (format = 'ORC', transactional = true)", tableName));
onTrino().executeQuery("START TRANSACTION");
assertQueryFailure(() -> onTrino().executeQuery(format("INSERT INTO %s (a_string) VALUES ('Commander Bun Bun')", tableName)))
.hasMessageContaining("Inserting into Hive transactional tables is not supported in explicit transactions (use autocommit mode)");
});
}

@Test(groups = HIVE_TRANSACTIONAL)
public void testUpdateFailsInExplicitTrinoTransaction()
{
withTemporaryTable("update_fail_explicit_transaction", true, false, NONE, tableName -> {
onTrino().executeQuery(format("CREATE TABLE %s (a_string varchar) WITH (format = 'ORC', transactional = true)", tableName));
onTrino().executeQuery("START TRANSACTION");
assertQueryFailure(() -> onTrino().executeQuery(format("UPDATE %s SET a_string = 'Commander Bun Bun'", tableName)))
.hasMessageContaining("Updating transactional tables is not supported in explicit transactions (use autocommit mode)");
});
}

@Test(groups = HIVE_TRANSACTIONAL)
public void testDeleteFailsInExplicitTrinoTransaction()
{
withTemporaryTable("delete_fail_explicit_transaction", true, false, NONE, tableName -> {
onTrino().executeQuery(format("CREATE TABLE %s (a_string varchar) WITH (format = 'ORC', transactional = true)", tableName));
onTrino().executeQuery("START TRANSACTION");
assertQueryFailure(() -> onTrino().executeQuery(format("DELETE FROM %s WHERE a_string = 'Commander Bun Bun'", tableName)))
.hasMessageContaining("Deleting from Hive transactional tables is not supported in explicit transactions (use autocommit mode)");
});
}

@Test(groups = HIVE_TRANSACTIONAL, dataProvider = "transactionModeProvider")
public void testColumnRenamesOrcPartitioned(boolean transactional)
{
Expand Down