Skip to content

Clear the connection error after SQLError reports it (fixes #102) - #104

Open
singhpratech wants to merge 1 commit into
opensearch-project:mainfrom
singhpratech:fix-connecterror-never-clears
Open

singhpratech wants to merge 1 commit into
opensearch-project:mainfrom
singhpratech:fix-connecterror-never-clears

Conversation

@singhpratech

Copy link
Copy Markdown

Fixes #102.

SQLError takes no record number: by the ODBC 2 contract each call hands back the next diagnostic and the driver forgets it, so that a caller looping until SQL_NO_DATA_FOUND terminates. OPENSEARCHAPI_ConnectError read the error with CC_get_error() and returned SQL_SUCCESS without clearing it, so every call returned the same record.

unixODBC's SQLDriverConnect(W) prefers SQLError(W) over SQLGetDiagRec(W) when the driver exports it, and collects diagnostics in a do { } while (SQL_SUCCEEDED(ret)) loop. With the error never cleared that loop does not end, so any connect that cannot succeed — a closed port, useSSL=1 against a plaintext cluster, an unknown auth= value — leaves the process spinning at 100% CPU instead of returning SQL_ERROR. The issue has the strace evidence: 31,948 writes to the log in 20 seconds, all of them the same ConnectError entry.

The change

Four lines in src/sqlodbc/environ.c. There is already a precedent in the same file: EN_get_error() clears the environment's error after handing it out, which is why OPENSEARCHAPI_EnvError terminates. This does the same for the connection.

Three details worth stating, since they are the reasons it is written this way rather than more simply:

  • In ConnectError, not in CC_get_error(). SQLGetDiagRec must stay repeatable by record number, and it reaches the same data; clearing inside the getter would break it.
  • After the last use of msg. CC_clear_error() frees __error_message, which is exactly what CC_get_error() handed back in msg, so clearing earlier would leave a dangling pointer.
  • The cbErrorMsgMax == 0 path is left alone. That is the buffer-sizing retry, which deliberately re-sets the error number so the caller can fetch the record properly on the next call; clearing there would lose it.

What I could and could not verify

gcc -fsyntax-only on the edited translation unit is clean and produces no new diagnostics against the unpatched baseline. I could not build or run the suite: src/sqlodbc/CMakeLists.txt adds /usr/src/linux-headers-5.0.0-27/include to the include path, which does not exist on a current distribution, and the unit tests want vcpkg, the AWS SDK and a live cluster. So this is a reasoned fix rather than a locally tested one, and I would rather say that than imply otherwise — please let CI judge it.

The reproduction in #102 needs no index and no fixture: point SQLDriverConnectW at a closed port and it currently never returns. That is the behaviour to check before and after.

On the DCO

The commit is signed off as singhpratech, which is the identity all my other upstream contributions use. I see CONTRIBUTING.md asks for a real name; if that is a blocker I am happy to amend the commit to sign off as Prateek Singh with the same address — just say so and I will force-push the change.

This came up while running OpenSearch through adbcBridge (https://github.com/singhpratech/adbcbridge), a driver for ADBC — Apache Arrow's database connectivity API — that works over any ODBC driver: it opens connections through the driver manager, so a failed connect that never returns is a hang rather than an error.

SQLError takes no record number: by the ODBC 2 contract each call hands back
the next diagnostic and the driver forgets it, so a caller looping until
SQL_NO_DATA_FOUND terminates. OPENSEARCHAPI_ConnectError read the error with
CC_get_error() and returned SQL_SUCCESS without clearing it, so every call
returned the same record.

unixODBC's SQLDriverConnect(W) prefers SQLError(W) over SQLGetDiagRec(W) when
the driver exports it, and collects diagnostics in a
do { } while (SQL_SUCCEEDED(ret)) loop. With the error never cleared that loop
does not end: any connect that cannot succeed - a closed port, useSSL=1
against a plaintext cluster, an unknown auth= value - leaves the process at
100% CPU rather than returning SQL_ERROR.

EN_get_error() already clears the environment's error for this reason, which
is why OPENSEARCHAPI_EnvError terminates. This does the same for the
connection, in ConnectError rather than in CC_get_error(), because
SQLGetDiagRec must stay repeatable. The clear happens after the last use of
msg, which points at __error_message, and the cbErrorMsgMax == 0 retry path is
left alone so that a caller sizing its buffer can still re-fetch the record.

Signed-off-by: singhpratech <42719720+singhpratech@users.noreply.github.com>
singhpratech added a commit to singhpratech/adbcbridge that referenced this pull request Sep 26, 2026
…s parameter-array half (#150)

opensearch-project/sql-odbc#104 (ConnectError never clears its error, so a
failed connect spins forever) and #105 (a character value asked for as a
datetime came back as today's date) were opened 2026-09-24. ClickHouse/
clickhouse-odbc#588 covers the second half of #582; the maintainer's review
asked for the ODBC error-path contract, which was added the same day and
verified on a driver built from the branch.

None of the three has landed. The rows say what each PR does and how far it
was verified, which for the OpenSearch pair is a syntax check only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant