-
Notifications
You must be signed in to change notification settings - Fork 48
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
Return out_of_memory error tuple if row alloc fails #304
base: main
Are you sure you want to change the base?
Conversation
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 prefer to have brackets around all if
statements. I've been bitten too many times by not having them.
Unsure on what to do when sqlite3_column_count
returns 0. The behavior I originally implemented just returned an error, but I don't know if that is appropriate. 🤔
@@ -628,30 +615,47 @@ exqlite_multi_step(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) | |||
return make_error_tuple(env, am_invalid_chunk_size); | |||
} | |||
|
|||
int column_count = sqlite3_column_count(statement->statement); |
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.
This can return 0
https://www.sqlite.org/c3ref/column_count.html
We should handle this. I don't think an error is appropriate here. I'm not 100% certain.
Co-authored-by: Matthew Johnston <[email protected]>
Co-authored-by: Matthew Johnston <[email protected]>
Co-authored-by: Matthew Johnston <[email protected]>
ERL_NIF_TERM* cells = NULL; | ||
cells = enif_alloc(sizeof(ERL_NIF_TERM) * column_count); | ||
|
||
if (!cells) { |
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.
if (!cells) { | |
if (!cells && column_count > 0) { |
Something like this?
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.
Yea I think that would work. It would be safe because the list of results returned would be an empty array.
ERL_NIF_TERM* cells = NULL; | ||
cells = enif_alloc(sizeof(ERL_NIF_TERM) * column_count); | ||
|
||
if (!cells) { |
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.
if (!cells) { | |
if (!cells && column_count > 0) { |
This needs to be rebased. |
Attempt at #301 (comment)
This PR makes multi_step allocate cells array once, and return an error tuple if that allocation fails.
Some benchmarks (the "10 rows" one might need to be re-run, it seems to be an outlier):
This approach
Results:
Master
Results: