Cache server_version_info and guard against re-entrant reads

[hashhar]

Description

The SQLAlchemy dialect exposed server_version_info as an un-cached property whose getter runs "SELECT version()" through the connection. Because the query executes via the same SQLAlchemy machinery any code that reads server_version_info while a query is in flight re-enters the getter and issues another query, recursing forever.

aws-xray-sdk is an example of this pattern.

Cache the resolved value on the dialect instance and seed the cache with None before issuing the query. A re-entrant read that arrives while the version query is in flight now returns immediately from the seeded cache instead of recursing and subsequent reads are served from the cache without additional HTTP calls.

Non-technical explanation

Cache server_version_info and guard against re-entrant reads. Fixes #559.

Release notes

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

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

[hashhar]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

tests/integration/test_sqlalchemy_integration.py (1)

744-760: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Test will fail if eager evaluation is adopted.

If you refactor dialect.py to use eager evaluation as recommended, this test will fail because SELECT version() will be executed during dialect initialization (meaning num_queries will be 1 immediately after connection, before server_version_info is explicitly accessed).

If eager evaluation is adopted, this test should either be removed or repurposed to simply verify that server_version_info is correctly populated and cached once per engine.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/integration/test_sqlalchemy_integration.py` around lines 744 - 760, The
test_version_is_lazy test assumes lazy server-version evaluation and will fail
if dialect initialization becomes eager. Repurpose or remove its pre-access
query-count assertions, and instead verify that dialect.server_version_info is
correctly populated and that repeated access does not issue more than one SELECT
version() query per engine.

tests/unit/sqlalchemy/test_dialect.py (1)

319-359: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Test may be obsolete if eager evaluation is adopted.

This test correctly validates the new caching logic designed to prevent recursive execution. However, as noted in dialect.py, the current lazy property implementation introduces significant concurrency and connection-leak risks.

If you adopt the recommended eager evaluation approach, the re-entrant hazard natively disappears. In that case, this test should be simplified to just verify that _get_server_version_info returns the expected tuple and handles exceptions gracefully, without needing to mimic re-entrant reads or use custom property getters.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/sqlalchemy/test_dialect.py` around lines 319 - 359, Update
test_server_version_info_does_not_recurse_and_is_cached to match the eager
server-version evaluation approach: remove the re-entrant
FakeConnection/property-read simulation and cache-specific execute-count
assertions, and instead verify that _get_server_version_info returns the
expected version tuple and handles exceptions gracefully.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@trino/sqlalchemy/dialect.py`:
- Around line 423-443: Remove the lazy property assignment and nested
get_server_version_info closure from _get_server_version_info. Restore eager
evaluation using the provided connection: execute SELECT version(), return the
version as a one-element tuple, and return None on exc.ProgrammingError while
logging the exception without relying on e.orig.message. Do not mutate
cls.server_version_info or cache results on the dialect instance.
- Around line 436-439: Update the ProgrammingError handler in the server-version
lookup to log the caught exception via its string representation, such as e or
str(e), instead of accessing e.orig.message. Preserve the existing debug logging
context and value = None fallback.

---

Nitpick comments:
In `@tests/integration/test_sqlalchemy_integration.py`:
- Around line 744-760: The test_version_is_lazy test assumes lazy server-version
evaluation and will fail if dialect initialization becomes eager. Repurpose or
remove its pre-access query-count assertions, and instead verify that
dialect.server_version_info is correctly populated and that repeated access does
not issue more than one SELECT version() query per engine.

In `@tests/unit/sqlalchemy/test_dialect.py`:
- Around line 319-359: Update
test_server_version_info_does_not_recurse_and_is_cached to match the eager
server-version evaluation approach: remove the re-entrant
FakeConnection/property-read simulation and cache-specific execute-count
assertions, and instead verify that _get_server_version_info returns the
expected version tuple and handles exceptions gracefully.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d7f0c464-5106-4a8c-9877-ba7c247c20e0

📥 Commits

Reviewing files that changed from the base of the PR and between 7b74702 and 2452316.

📒 Files selected for processing (3)

• tests/integration/test_sqlalchemy_integration.py
• tests/unit/sqlalchemy/test_dialect.py
• trino/sqlalchemy/dialect.py

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Avoid mutating the class and capturing transient initialization connections.

This lazy evaluation approach introduces three severe architectural hazards across SQLAlchemy engines and threads:

1. Class Mutation: Assigning cls.server_version_info = property(...) mutates the TrinoDialect class globally. All engine instances using this dialect will share the exact same property.
2. Connection Leak/Hazard: The get_server_version_info closure captures the specific connection passed during the very first engine's initialization. When the property is evaluated later by an application (or an interception tool like aws-xray-sdk), it will attempt to execute SELECT version() on that originally captured connection. If that connection has been returned to the pool or closed, it can lead to "connection closed" errors or interfere with other threads currently using that pooled connection.
3. Thread Race Condition: Since a single TrinoDialect instance is shared across all connections in an engine pool, setting self.__dict__["_trino_server_version_info"] = None creates a race condition. If Thread A triggers the lookup, it blocks on I/O. If Thread B accesses server_version_info concurrently, it will immediately receive None from the cache instead of the real version, causing Thread B to operate with missing version data.

If the overhead of a single SELECT version() during Engine initialization is acceptable, the safest and most robust fix is to remove the lazy property entirely and perform the eager evaluation that SQLAlchemy fundamentally expects:

    `@classmethod`
    def _get_server_version_info(cls, connection: Connection) -> Any:
        query = "SELECT version()"
        try:
            res = connection.execute(sql.text(query))
            version = res.scalar()
            return (version,)
        except exc.ProgrammingError as e:
            logger.debug("Failed to get server version: %s", e)
            return None

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@trino/sqlalchemy/dialect.py` around lines 423 - 443, Remove the lazy property
assignment and nested get_server_version_info closure from
_get_server_version_info. Restore eager evaluation using the provided
connection: execute SELECT version(), return the version as a one-element tuple,
and return None on exc.ProgrammingError while logging the exception without
relying on e.orig.message. Do not mutate cls.server_version_info or cache
results on the dialect instance.

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Crash risk and incorrect logging when accessing e.orig.message.

Accessing e.orig.message introduces two significant failure modes:

1. Crash risk: If the underlying DBAPI exception (e.orig) does not have a message attribute (which is common for standard DBAPI exceptions), this will raise an AttributeError. Since AttributeError is not caught here, it will bubble up and crash the application during the version lookup.
2. Incorrect logging: For TrinoQueryError where message is defined as a method (def message(self) -> str:), omitting the parentheses causes the f-string to evaluate and log the bound method object (e.g., <bound method ...>) instead of the actual error message.

To fix both issues and robustly handle any underlying DBAPI exception type, use str(e) or e directly.

🛠️ Proposed fix to safely log the exception

-            except exc.ProgrammingError as e:
-                logger.debug(f"Failed to get server version: {e.orig.message}")
-                value = None
+            except exc.ProgrammingError as e:
+                logger.debug("Failed to get server version: %s", e)
+                value = None

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	except exc.ProgrammingError as e:
	logger.debug(f"Failed to get server version: {e.orig.message}")
	return None
	value = None
	self.__dict__["_trino_server_version_info"] = value
	except exc.ProgrammingError as e:
	logger.debug("Failed to get server version: %s", e)
	value = None
	self.__dict__["_trino_server_version_info"] = value

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@trino/sqlalchemy/dialect.py` around lines 436 - 439, Update the
ProgrammingError handler in the server-version lookup to log the caught
exception via its string representation, such as e or str(e), instead of
accessing e.orig.message. Preserve the existing debug logging context and value
= None fallback.