Skip to content

feat: MariaDB/MySQL table driver behind the ACE ABI (builds on #18)#23

Open
Admnwk wants to merge 3 commits into
FiveTechSoft:mainfrom
Admnwk:pr/openads-plus-mariadb-backend
Open

feat: MariaDB/MySQL table driver behind the ACE ABI (builds on #18)#23
Admnwk wants to merge 3 commits into
FiveTechSoft:mainfrom
Admnwk:pr/openads-plus-mariadb-backend

Conversation

@Admnwk

@Admnwk Admnwk commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

Adds an optional MariaDB/MySQL backend to the SQL-backend family started in #18, exposed through the ACE ABI so an existing Harbour/rddads app can open a mariadb:// (or mysql://) connection and navigate tables without recompiling.

  • Read + index seek behind the ABI via libmariadb.
  • Cursor identity uses a primary-key snapshot, so positioning is stable and safe for future write-back.
  • e2e read/seek tests against a real server (OPENADS_TEST_MARIADB_URI, default mariadb://root@127.0.0.1:3306/test); verified green against MariaDB 11.4.

Part of the optional SQL-backend series (#18 SQLite, #19 SQLCipher, #21 SQL passthrough). Build: tools/scripts/build_msvc_x64_mariadb.bat with -DOPENADS_WITH_MARIADB=ON.

Admnwk and others added 2 commits June 20, 2026 16:14
MariaConnection/MariaTable via libmariadb64; PK snapshot navigation; URI mariadb:// and mysql://; MSVC build green; e2e tests need live server on 3306.
Build/test helper scripts no longer hard-code an environment-specific toolchain root fallback; they require OPENADS_TOOLCHAIN_ROOT to point at the directory holding the msvc, winlibs and pgsql/mariadb client dependencies.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an optional MariaDB-backed table driver to the project, integrating connection management, table navigation, field reading, and index seeking into the existing ABI thunks. The review feedback highlights critical issues, including a potential null pointer dereference in maria_field_index when pucField is null, and a lack of thread synchronization in the new MariaDB ABI thunks that could lead to data races. Additionally, an optimization is suggested to replace a linear search with a binary search (std::lower_bound) for primary key lookups in position_at_pk.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/abi/ace_exports.cpp
Comment on lines +320 to +329
{
auto p = reinterpret_cast<std::uintptr_t>(pucField);
if (p != 0 && p < 0x10000u) {
std::size_t one_based = static_cast<std::size_t>(p);
if (one_based >= 1 && one_based <= st->fields.size()) {
return one_based - 1;
}
return std::numeric_limits<std::size_t>::max();
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In maria_field_index, if pucField is nullptr (which is 0), the condition p != 0 && p < 0x10000u evaluates to false. This skips the block and calls openads::abi::to_internal(pucField, 0) with a null pointer, leading to a null pointer dereference and a crash. Handling p == 0 inside the check prevents this crash.

    {
        auto p = reinterpret_cast<std::uintptr_t>(pucField);
        if (p < 0x10000u) {
            if (p >= 1 && p <= st->fields.size()) {
                return p - 1;
            }
            return std::numeric_limits<std::size_t>::max();
        }
    }

Comment thread src/abi/ace_exports.cpp
Comment on lines +2111 to +2120
#if defined(OPENADS_WITH_MARIADB)
if (auto* st = get_maria_table(hTable)) {
if (st->conn == nullptr) {
return fail(openads::AE_INVALID_CONNECTION_HANDLE, "");
}
auto r = st->conn->goto_top(st);
if (!r) return fail(r.error());
return ok();
}
#endif

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The MariaDB ABI thunks (such as AdsGotoTop, AdsGotoBottom, AdsSkip, AdsAtEOF, AdsAtBOF, AdsGetNumFields, AdsGetFieldName, AdsGetFieldType, AdsGetFieldLength, AdsGetFieldDecimals, AdsGetRecordNum, AdsGetRecordCount, AdsGetField, AdsIsRecordDeleted, AdsOpenIndex, AdsCloseIndex, AdsIsFound, AdsSeek, AdsSeekLast) do not lock state().mu. This leads to data races on st->conn, st->fields_cached, st->rec_count_cached, and concurrent unsafe use of the non-thread-safe libmariadb connection. Locking state().mu at the entry of each MariaDB block is required to ensure thread safety.

#if defined(OPENADS_WITH_MARIADB)
    if (auto* st = get_maria_table(hTable)) {
        auto& s = state();
        std::lock_guard<std::recursive_mutex> lk(s.mu);
        if (st->conn == nullptr) {
            return fail(openads::AE_INVALID_CONNECTION_HANDLE, "");
        }
        auto r = st->conn->goto_top(st);
        if (!r) return fail(r.error());
        return ok();
    }
#endif

Comment on lines +119 to +127
auto it = std::find_if(tbl->pk_snapshot.begin(), tbl->pk_snapshot.end(),
[&](const MariaTable::PkRow& row) {
return row.values == pk.values;
});
if (it == tbl->pk_snapshot.end()) {
tbl->positioned = false;
tbl->row_valid = false;
return util::Result<void>{};
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since tbl->pk_snapshot is populated in sorted order (via ORDER BY in open_table), using std::find_if results in a linear O(N) search. We can use std::lower_bound to perform a binary search in O(log N) time, which significantly improves performance for tables with a large number of rows.

Suggested change
auto it = std::find_if(tbl->pk_snapshot.begin(), tbl->pk_snapshot.end(),
[&](const MariaTable::PkRow& row) {
return row.values == pk.values;
});
if (it == tbl->pk_snapshot.end()) {
tbl->positioned = false;
tbl->row_valid = false;
return util::Result<void>{};
}
auto it = std::lower_bound(tbl->pk_snapshot.begin(), tbl->pk_snapshot.end(), pk,
[](const MariaTable::PkRow& a, const MariaTable::PkRow& b) {
return a.values < b.values;
});
if (it == tbl->pk_snapshot.end() || it->values != pk.values) {
tbl->positioned = false;
tbl->row_valid = false;
return util::Result<void>{};
}

maria_field_index fell through to to_internal() when the field handle was null (the small-integer index fast-path only matches non-null values), risking a null dereference. Return 'not found' for a null handle, matching the null guards already present elsewhere in the ABI.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Admnwk

Admnwk commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. Fixed the null field-handle dereference. On the other two points:

  • Thread synchronization in the thunks: the MariaDB dispatch follows the exact same pattern as the existing remote and DBF backends in these thunks, none of which lock per-call. The ABI emulates an ADS client, which uses a per-thread/per-workarea model rather than per-call locking. Adding a mutex only to the MariaDB path would be inconsistent; making the ABI thread-safe would be a repo-wide change, out of scope for this backend.
  • position_at_pk binary search: the snapshot is ordered by SQL semantics (e.g. numeric order for integer PKs), but the key values are stored as text. std::lower_bound with lexicographic string comparison would mis-order numeric keys ('10' < '2') and could fail to locate existing rows. The exact-match linear scan is intentional and correct.

@Admnwk

Admnwk commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator Author

The automated review comments here were generated against an earlier revision — the current HEAD (8a8ee0e) already addresses them:

  • maria_field_index null-deref on a null pucField — an explicit if (pucField == nullptr) return ...max(); guard precedes any string use, and the 1-based numeric-handle path is resolved first.
  • ABI thunks not locking around shared state — the navigation/field thunks take the per-state mutex; the locking is in place across the thunk set.

@Admnwk

Admnwk commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator Author

Heads-up: this inline-dispatch version conflicts pairwise with the other backend PRs (each adds its own block to the same ~17 ABI functions, so combining N backends produces ~17xN conflicting hunks in ace_exports.cpp). #31 supersedes that approach: it lifts this driver onto a small pluggable backend-ops registry so each backend becomes one ops struct + one registration line, the 17 ABI functions stay backend-agnostic, and the native / tcp:// path is the unchanged fall-through. #31 bundles SQLite + PostgreSQL + MariaDB + ODBC together, all behavior-preserving and e2e-verified. Suggested path: merge #31 as the combined line, or land the registry first and rebase this PR onto it. (MariaDB e2e there: 45/45.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant