Add individual binds #298
Add individual binds #298
Conversation
7159f99 to
e1f7d87
Compare
Absolutely, this would mean we can toss my old implementation. |
| try do | ||
| Sqlite3.bind(ref, params) | ||
| rescue | ||
| e -> {:error, %Error{message: Exception.message(e), statement: statement}, state} |
There was a problem hiding this comment.
We lose useful stacktrace here (which bind_* was called? helps identify which type was expected), but this can probably be handled later.
There was a problem hiding this comment.
Can we reraise with a reformed error and retain the stack trace?
There was a problem hiding this comment.
Maybe. I don't have much context on this function or how DBConnection handles exceptions "from within" .... I think it's safer for now to keep the previous behavior of not raising and returning an error triplet. I tried to think of various scenarios in #298 (comment) and right now I don't see anything useful being lost.
|
|
||
| @deprecated "Use `bind/2` instead" | ||
| @spec bind(db, statement, [bind_value]) :: :ok | ||
| def bind(_conn, stmt, args), do: bind(stmt, args) |
There was a problem hiding this comment.
I think all functions that receive both db and stmt can be updated to only require stmt since it has all the necessary info and we can use sqlite3_db_handle(stmt) to get the DB if needed.
| a when is_atom(a) -> bind_text(stmt, idx, Atom.to_string(a)) | ||
| {:blob, b} when is_binary(b) -> bind_blob(stmt, idx, b) | ||
| {:blob, b} when is_list(b) -> bind_blob(stmt, idx, IO.iodata_to_binary(b)) | ||
| _other -> raise ArgumentError, "unsupported type: #{inspect(param)}" |
There was a problem hiding this comment.
I removed BindError to standardize on ArgumentError, since it was already used in convert/1, and because it's also raised from IO.iodata_to_binary/1 and the new bind_* functions. bind_* might raise Exqlite.Error but I can't think of a scenario where it could realistically happen since in bind/2 we call reset/1 and ensure correct idx by checking for bind_params_count(stmt) == length(args). Maybe it could happen if blob or text are too large? Or if the statement has been release/1-ed or step/2-ed from another thread? But that's not possible under the current db_connection implementation and would mean a user error of using statements and Exqlite.Sqlite3 directly from different processes, so I think that's fine to assume it only raises ArgumentErrors.
There was a problem hiding this comment.
I added BindError because I could not figure out how to raise it directly from the NIF and I wanted to pass the term that had failed. I'm not attached to it.
Maybe it could happen if blob or text are too large?
AFAIK this probably isn't possible under normal circumstances. You are allowed to redefine what the max limit of blobs can be when compiling, so you could in theory test this yourself locally by defining -DSQLITE_MAX_LENGTH=4096 and doing a quick test to see the behavior.
There was a problem hiding this comment.
raise it directly from the NIF
AFAIK there is no way to do this right now, but there were some plans to add error_info to erl_nif APIs. error_info is already being used in bifs and gets picked up by Elixir as well: https://github.com/elixir-lang/elixir/blob/be84111523cc18498b68949e4c2058eacb44fa00/lib/elixir/lib/exception.ex#L2483 I think that's how ets started returning not just "badarg" but
iex> :ets.new "asdf", []
** (ArgumentError) errors were found at the given arguments:
* 1st argument: not an atom
(stdlib 6.0) :ets.new("asdf", [])
iex:1: (file)| @@ -1282,7 +1244,13 @@ static ErlNifFunc nif_funcs[] = { | |||
| {"execute", 2, exqlite_execute, ERL_NIF_DIRTY_JOB_IO_BOUND}, | |||
| {"changes", 1, exqlite_changes, ERL_NIF_DIRTY_JOB_IO_BOUND}, | |||
| {"prepare", 2, exqlite_prepare, ERL_NIF_DIRTY_JOB_IO_BOUND}, | |||
| {"bind", 3, exqlite_bind, ERL_NIF_DIRTY_JOB_IO_BOUND}, | |||
| {"reset", 1, exqlite_reset, ERL_NIF_DIRTY_JOB_CPU_BOUND}, | |||
There was a problem hiding this comment.
I'm using a dirty CPU scheduler here because of this mutex: https://github.com/sqlite/sqlite/blob/7f5a10e4ba20826b03ea898945f3ae9138b5568e/src/vdbeapi.c#L136
It can probably be scheduled on the main scheduler in most db_connection-related scenarios though. Or we can use https://www.sqlite.org/c3ref/stmt_status.html to check if the statement needs reset (i.e. if run > 1), and only run it then. sqlite3_stmt_status can use a main scheduler since it's just a struct field read.
There was a problem hiding this comment.
Overall @ruslandoga this looks great. I'll merge this once we resolve any lingering questions. I want to iterate on this more and drive towards removing DBConnection if we can.
|
Yea this looks good. Let's get this merged and then iterate one this. |
Predefines atoms like Rustler does. I think this improves readability and kind of makes it safer against typos. Once or if #298 is merged, many of the currently used atoms could be replaced with `raise_badarg()` function. I've also left some notes on atoms that I think shouldn't be atoms and rather SQLite errors.
This PR adds ability to set individual binds and re-implements
bind/3using the newbind_*functions. And also deprecatesbind/3in favor ofbind/2since we don't need conn reference anymore.