#4116 fix(postgres): SHOW server_version returns proper value for SQLAlchemy#4117
Conversation
…Alchemy SQLAlchemy 2.x calls SHOW server_version during connection initialization and feeds the scalar result into a regex. Two defects in PostgresNetworkExecutor caused scalar() to return None (TypeError on re.match): - queryCommand() (simple-query 'Q') had no SHOW handler, so the query fell through to ArcadeDB SQL which has no SHOW statement and returned an empty result set. - parseCommand() generic SHOW branch returned column "CURRENT_SCHEMA" with the database name as value, ignoring the requested variable. Both paths now extract the variable name and resolve it via the new getShowConfigValue() helper, which maps known PG runtime parameters (server_version, standard_conforming_strings, integer_datetimes, client_encoding, server_encoding, timezone, transaction isolation level) to their correct values. Tests: - PostgresProtocolIT#showServerVersion / showStandardConformingStrings - e2e-python/tests/test_sqlalchemy.py covering engine.connect(), SELECT, CREATE/INSERT via SQLAlchemy + psycopg3
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 3 minor |
| Security | 4 high |
🟢 Metrics 12 complexity
Metric Results Complexity 12
🟢 Coverage 69.23% diff coverage · -7.78% coverage variation
Metric Results Coverage variation ✅ -7.78% coverage variation Diff coverage ✅ 69.23% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (7ce2cd0) 123678 90871 73.47% Head commit (9c0f1c1) 155101 (+31423) 101893 (+11022) 65.69% (-7.78%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#4117) 39 27 69.23% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Code Review
This pull request implements support for several PostgreSQL SHOW commands and adds SQLAlchemy integration tests. The review feedback highlights several areas for improvement: using specific Docker image tags for deterministic testing, making command parsing more robust against leading whitespace, unifying version strings, and ensuring consistent naming and behavior for configuration parameters like transaction isolation levels and unknown variables.
I am having trouble creating individual review comments. Click here to see my feedback.
e2e-python/tests/test_sqlalchemy.py (25)
Using the latest tag for the Docker image in tests can lead to non-deterministic results and makes it difficult to reproduce failures. It is recommended to use a specific version tag or a variable that identifies the image built from the current branch/PR to ensure the tests are running against the intended code changes.
postgresw/src/main/java/com/arcadedb/postgres/PostgresNetworkExecutor.java (481)
If the query includes a language prefix followed by a space (e.g., {sql} SHOW server_version), query.query will contain a leading space, causing this startsWith check to fail. It is recommended to trim the query string before performing these checks. Additionally, there is an inconsistency between the hardcoded 11.0.0 version in SELECT VERSION() (line 478) and the new PG_SERVER_VERSION (12.0). Both should ideally be unified.
postgresw/src/main/java/com/arcadedb/postgres/PostgresNetworkExecutor.java (1148)
Similar to the issue in queryCommand, if the query has leading whitespace (e.g., after a language prefix), the startsWith("SHOW ") check at line 1147 will fail. Trimming the query string before checking the prefix would make the command recognition more robust.
postgresw/src/main/java/com/arcadedb/postgres/PostgresNetworkExecutor.java (1359)
The isolation level is returned here in lowercase. However, the existing special case in parseCommand (line 1145) returns it in uppercase and uses the column name LEVEL. In queryCommand, the column name will be transaction isolation level. This inconsistency should be addressed. Consider using a consistent column name like transaction_isolation and a consistent value format to align with standard PostgreSQL behavior.
postgresw/src/main/java/com/arcadedb/postgres/PostgresNetworkExecutor.java (1360)
Returning database.getName() as a default for any unknown SHOW variable is misleading, as it doesn't represent the actual configuration parameter requested. PostgreSQL typically returns an error for unknown configuration parameters. Consider returning an empty string or throwing an exception if the variable is not recognized, or at least documenting why the database name is used as a fallback.
Code Review - PR #4117: fix(postgres): SHOW server_version returns proper value for SQLAlchemyOverall: Good, targeted fix for a real SQLAlchemy compatibility issue. The approach is clean and the new tests validate the core behaviour. A few things worth addressing before merge. CorrectnessVersion inconsistency between
resultSet = new IteratorResultSet(createResultSet("VERSION", PG_SERVER_VERSION).iterator());
// and
createResultSet(portal, "VERSION", PG_SERVER_VERSION);Column name inconsistency for After this PR,
The Default case in
Python E2E Test IssuesMissing container teardown - the @pytest.fixture(scope="module", autouse=True)
def setup(request):
arcadedb.start()
wait_for_http_endpoint(arcadedb, "/api/v1/ready", 2480, 204, 60)
# Missing: yield / cleanupShould use @pytest.fixture(scope="module", autouse=True)
def setup():
arcadedb.start()
wait_for_http_endpoint(arcadedb, "/api/v1/ready", 2480, 204, 60)
yield
arcadedb.stop()Redundant import: both Test isolation: Wait timeout for OpenBeer import: the Minor NitThe What's Good
|
- SELECT VERSION() now returns PG_SERVER_VERSION ("12.0") consistent with
SHOW server_version and the startup ParameterStatus, instead of the
hardcoded "11.0.0" that conflicted with the rest of the protocol surface.
- queryCommand() gains a SHOW TRANSACTION ISOLATION LEVEL special case
matching parseCommand(), so both wire-protocol paths return column "LEVEL"
with the same uppercase value. The dead "transaction isolation level"
branch in getShowConfigValue() is removed.
- getShowConfigValue() default now returns "" instead of database.getName()
for unknown PostgreSQL runtime parameters - returning the database name
for arbitrary SHOW <var> was misleading.
- getLanguageAndQuery() trims after stripping the {language} prefix so
"{sql} SHOW server_version" no longer fails the prefix checks.
- selectVersion test now asserts the unified PG_SERVER_VERSION value.
|
Review feedback addressed in 067abd9: Fixed:
Deferred / not changed:
|
Code Review - PR #4117: fix(postgres): SHOW server_version returns proper value for SQLAlchemyOverviewThis PR fixes a real SQLAlchemy 2.x connectivity issue by making Positives
Issues1. Version format mismatch (breaking change risk)
2. The specific case in both 3. e2e test: missing container teardown @pytest.fixture(scope="module", autouse=True)
def setup(request):
arcadedb.start()
wait_for_http_endpoint(arcadedb, "/api/v1/ready", 2480, 204, 60)
# Missing: yield + arcadedb.stop()The container is started but never explicitly stopped. It will linger for the duration of the test session rather than cleaning up when the module fixture exits. Add a @pytest.fixture(scope="module", autouse=True)
def setup(request):
arcadedb.start()
wait_for_http_endpoint(arcadedb, "/api/v1/ready", 2480, 204, 60)
yield
arcadedb.stop()4. e2e test: arcadedb = DockerContainer("arcadedata/arcadedb:latest")Using 5. e2e test: redundant imports and minor style import time
from time import sleep
6. e2e test:
7. e2e test: ArcadeDB-specific SQL may not round-trip through SQLAlchemy result = conn.execute(text("SELECT FROM Beer LIMIT 5"))
rows = result.fetchall()
assert len(rows) == 5
Minor Suggestions
SummaryThe core fix is correct and well-targeted. The main items worth addressing before merge are the container teardown in the e2e test, the |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4117 +/- ##
==========================================
- Coverage 64.49% 64.49% -0.01%
==========================================
Files 1624 1627 +3
Lines 123678 123752 +74
Branches 26343 26362 +19
==========================================
+ Hits 79762 79809 +47
- Misses 32837 32859 +22
- Partials 11079 11084 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ries The first round of the fix addressed SHOW server_version but missed the actual queries SQLAlchemy 2.x sends. End-to-end testing with the locally built image revealed three additional gaps: - SQLAlchemy issues `select pg_catalog.version()` (not `select version()`). This was caught by handlePgTypeQuery() (matches "PG_CATALOG") and returned an empty list, so scalar() was None and SQLAlchemy's PostgreSQL/EnterpriseDB regex hit re.match(pattern, None) -> TypeError. Both queryCommand() and parseCommand() now match SELECT [PG_CATALOG.]VERSION() and return a proper "PostgreSQL X.Y (ArcadeDB Z)" string so SQLAlchemy's regex matches. Same treatment for SELECT [PG_CATALOG.]CURRENT_SCHEMA(). - SELECT VERSION() and SELECT CURRENT_SCHEMA() comparisons in queryCommand() were case-sensitive (.equals); psycopg3 sends lowercase. Switched to equalsIgnoreCase / cached upperCaseText to match parseCommand() behaviour. - After fetching the version, psycopg3 wraps subsequent type-info lookups in SAVEPOINT/RELEASE/ROLLBACK TO. Only SAVEPOINT was treated as a no-op, and only in parseCommand(). queryCommand() now ignores all three forms, and parseCommand() also accepts the bare `RELEASE name` / `ROLLBACK TO name` variants psycopg3 emits. Column names returned for VERSION/CURRENT_SCHEMA changed to lowercase to match real PostgreSQL.
|
CI surfaced the actual end-to-end picture I'd missed. Pushed The earlier
Verified end-to-end with the locally built image:
|
Code ReviewOverviewThis PR fixes issue #4116 where The changes are well-scoped and the approach is sound. A few issues and observations are noted below. Bugs / Correctness1. Silent empty-string fallback for unknown SHOW variables
default -> "";Real PostgreSQL responds with an error for unknown 2. PostgreSQL supports two forms for isolation level:
The single-word form would fall into case "transaction_isolation" -> "read committed";3. Both 4. E2E test: no container teardown In @pytest.fixture(scope="module", autouse=True)
def setup(request):
arcadedb.start()
wait_for_http_endpoint(arcadedb, "/api/v1/ready", 2480, 204, 60)There is no request.addfinalizer(arcadedb.stop)5. E2E test: potential race condition on dataset availability
Minor / Style6. Column name for This is correct - PostgreSQL returns lowercase column names here - but it is a behavior change that could break clients that were accessing the column by name using the old uppercase key. Worth noting in the CHANGELOG. 7. Missing tests for new The PR adds 8. Missing test for The 9.
Good Improvements
SummaryThe core fix is correct and well-implemented. The main actionable items are:
|
…ue for SQLAlchemy (ArcadeData#4117)
Summary
SHOW server_versionover the PostgreSQL wire protocol now returns columnserver_versionwith value12.0so that SQLAlchemy 2.x (postgresql+psycopg://) can completeengine.connect()instead of failing withTypeError: expected string or bytes-like object, got 'NoneType'.queryCommand()(simple-query 'Q') gains aSHOWbranch;parseCommand()genericSHOWbranch now uses the requested variable name as the column instead of the misleadingCURRENT_SCHEMA.getShowConfigValue()maps known PostgreSQL runtime parameters (server_version,standard_conforming_strings,integer_datetimes,client_encoding,server_encoding,timezone,transaction isolation level).Test plan
mvn test -f postgresw/pom.xml -Dtest=PostgresProtocolIT -DskipITs=false(65/65 pass, including newshowServerVersionandshowStandardConformingStrings)mvn test -f postgresw/pom.xml -DskipITs=false(247/247 pass, no regressions)e2e-python/tests/test_sqlalchemy.pyagainst the freshly built image to verify SQLAlchemy + psycopg3 connectivity end-to-end.