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

[hashhar]

applied comments.