-
-
Notifications
You must be signed in to change notification settings - Fork 1
Description
Hi Adriano!
I’m working on a full revamp of Firebird ODBC Driver and this library caught my eye. ❤️
May I ask you what's its current status? Are you planning to keep evolving it, or was it mainly a prototype?
I’d really like to use it in the ODBC driver; it could help us remove a lot of legacy code.
With a little help from Claude 😄 we jotted down a few topics we’d need to cover to move forward -- would you mind taking a look?
P.S. Of course, I'd be happy to contribute with PRs, as well.
📋 Suggestions for Improvement
1. Missing IResultSet Wrapper / Multi-Row Fetch
Issue: The Statement class has execute() and fetchNext(), but there's no separate ResultSet abstraction. This makes it awkward to:
- Differentiate between a statement that returns results vs. one that doesn't
- Pass a result set to another function without passing the statement
- Support multiple simultaneous result sets from the same statement (e.g., stored procedures returning multiple cursors)
Suggestion: Consider adding a ResultSet class that wraps IResultSet and is returned from Statement::execute() when the statement produces a result set. This matches the JDBC pattern (Statement.executeQuery() returns ResultSet).
Alternative: If keeping the single-class design, document that Statement::execute() returns true if there are results to fetch, and fetchNext() should only be called after execute() returns true.
2. Batch Execution (IBatch) Not Exposed
Issue: The Firebird 4.0+ IBatch interface enables high-performance bulk inserts with a single server roundtrip. This is critical for ETL workloads and ODBC array parameter binding. The current API requires calling execute() N times for N rows.
Suggestion: Add a Batch class or extend Statement with:
void Statement::addBatch(); // Queue current parameter values
BatchResult Statement::executeBatch(); // Execute all queued rows in one roundtripThe BatchResult could expose per-row status (IBatchCompletionState::getState()).
Priority: High — this is the single biggest performance differentiator between naive row-by-row and professional-grade bulk operations.
3. No Scrollable Cursor Support
Issue: Statement only exposes fetchNext(), fetchPrior(), fetchFirst(), fetchLast(), fetchAbsolute(), fetchRelative(). However, these require opening the result set with IStatement::openCursor() specifying CURSOR_TYPE_SCROLLABLE. The current implementation uses CURSOR_TYPE_SCROLLABLE (line 167 of Statement.cpp), but this is always enabled even for forward-only queries.
Suggestion: Add a CursorType enum to StatementOptions:
enum class CursorType { FORWARD_ONLY, SCROLLABLE };- Default to
FORWARD_ONLYfor performance (no server-side cursor buffering) - When
SCROLLABLE, callopenCursor()withCURSOR_TYPE_SCROLLABLE
Note: We observed that execute() always passes IStatement::CURSOR_TYPE_SCROLLABLE. For forward-only streaming of large result sets, this forces the server to buffer all rows. Consider defaulting to 0 (forward-only) and only using CURSOR_TYPE_SCROLLABLE when scrollable methods are needed.
5. Missing Descriptor Metadata Access
Issue: The Descriptor struct in Descriptor.h contains rich metadata (type, scale, length, nullability, field/relation names). However:
Statement::getInputDescriptors()andgetOutputDescriptors()returnconst std::vector<Descriptor>&- The
Descriptorstruct is only defined for read access — applications can't modify it for deferred binding
Suggestion: For ODBC drivers and similar adapters, we need:
- Access to
IMessageMetadata::getField(),getRelation(),getAlias()etc. — currently these are processed internally but not fully exposed - Ability to override input metadata (e.g., bind a string value as VARCHAR when the server expects INTEGER) — this requires
IMetadataBuilder
Consider exposing:
Statement::getInputMetadata()→FbRef<IMessageMetadata>(already exists!)Statement::getOutputMetadata()→FbRef<IMessageMetadata>(already exists!)- A helper to build custom
IMessageMetadatafor input override scenarios
6. Statement Copy/Move Semantics
Issue: Statement is move-only (copy deleted, move-only). After a move, isValid() returns false. This is correct, but:
Statement::operator=(Statement&&)is deleted — why? This preventsstd::swapand vector/map usage- The moved-from statement's
statementHandleis moved butattachmentreference becomes dangling
Suggestion: Either:
- Make
Statementfully movable (implementoperator=(Statement&&)) - Or document explicitly that
Statementshould be heap-allocated and managed viastd::unique_ptr
7. Thread Safety Documentation
Issue: The library doesn't document thread safety guarantees. Questions:
- Is it safe to use one
Clientfrom multiple threads? - Is it safe to use one
Attachmentfrom multiple threads with differentTransactions? - Is
StatusWrapperthread-safe (it contains adirtyflag)?
Suggestion: Add a "Thread Safety" section to the documentation. Based on our reading of the code:
Clientappears thread-safe (only reads frommaster)Attachmentis NOT thread-safe (sharedTransactionstate)Statementis NOT thread-safe (mutableinMessage/outMessage)
8. Error Message Localization
Issue: DatabaseException::what() returns the Firebird error message, which is localized based on the server's LC_MESSAGES setting. For ODBC drivers, we need access to:
- The raw ISC error vector (
intptr_t*) for SQLSTATE mapping - The SQLCODE (via
IUtil::formatStatus()or equivalent)
Suggestion: Expose the raw error data in DatabaseException:
const std::vector<intptr_t>& getErrorVector() const;
int getSqlCode() const; // Computed via IStatus::getErrors()9. Missing IUtil Wrapper
Issue: Firebird's IUtil provides valuable utilities:
formatStatus()— format error messagesdecodeDate()/encodeDate()— manual date conversiongetClientVersion()— client library versiongetInt128()/getDecFloat16()/getDecFloat34()— type converters
Some of these are used internally (NumericConverter, CalendarConverter) but not exposed.
Suggestion: Consider exposing Client::getUtil() → fb::IUtil* or wrapping the most useful methods.