-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(archive): patch the sqlite driver to solve results inconsistencies #1415
Conversation
@@ -127,10 +127,15 @@ template prepare*(env: Sqlite, q: string, cleanup: untyped): ptr sqlite3_stmt = | |||
|
|||
proc bindParam*(s: RawStmtPtr, n: int, val: auto): cint = | |||
when val is openarray[byte]|seq[byte]: | |||
# The constant, SQLITE_TRANSIENT, may be passed to indicate that the object is to be copied | |||
# prior to the return from sqlite3_bind_*(). The object and pointer to it must remain valid | |||
# until then. SQLite will then manage the lifetime of its private copy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this means it handles the malloc
but always free
it again - more memory but (hopefully) not leaky memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We call the sqlite3_finalize
function later in the code to release the prepared statement memory.
This is what the documentation says about the SQLite statement destructor:
The application must finalize every prepared statement in order to avoid resource leaks.
I interpret this as deallocating any memory associated with the prepared statement. Bindings included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great work. Quite something to find this. Well done!
macOS test2 job failing due to the same offending test case. Test failure already tracked in #1357. Merging without re-running the checks and Ubuntu test2 job is passing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is a temporary patch until the SQLite library is improved. Making the SQLite manage a copy is far from ideal. Binding the parameters with this "destructor configuration" option, implies that SQLite is making a copy of the buffer. This is inefficient. A more performant solution should be investigated.
From SQLite C documentation (link):
The tests I have performed so far seem to confirm that these changes solve issue #1400. But to be 100% sure, more thorough testing needs to be done.