search

trinodb / trino

Official repository of Trino, the distributed SQL query engine for big data, formerly known as PrestoSQL (https://trino.io)
https://trino.io
Apache License 2.0
9.85k stars 2.85k forks source link

Phoenix transaction handling is non thread safe #21251

Open findepi opened 3 months ago

findepi commented 3 months ago

In Phoenix connector, the metadata is a singleton

https://github.com/trinodb/trino/blob/d67576ee6aa9bc93b323122d645fa333316a31b7/plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixClientModule.java#L131

the PhoenixMetadata class extends DefaultJdbcMetadata and the latter has a mutable, transaction-scoped field rollbackAction https://github.com/trinodb/trino/blob/9a64d36bd83e49b8206df7ba1fa761b431e15b18/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java#L120

Thus, the PhoenixMetadata singleton cannot be used concurrently by two queries.

findepi commented 3 months ago

cc @chenjian2664 @lhofhansl

HalimKim commented 1 month ago

it seems to be thread-safe for me because rollbackAction field is mutable but it is AtomicReference instance. Could you describe it more or Let me know how to reproduce a problematic situation please?

hashhar commented 1 month ago

It's a logical unsafety because the rollback action is populated based on the actions a single query takes. You cannot apply rollbackAction set by one query as a compensation action for another query.

i.e. while reading and writing the field is safe - it leads to wrong behaviour.