Closed tenderlove closed 1 hour ago
I've read #558 a few times, and it's not clear to me why
sqlite3_close_v2
is necessary for fork safety, but I could definitely be missing something.
I get it now. When we fork, we need to make the database and any associated statements unusable. We need a way of marking the database as "unusable" whether that be via directly asking the db object for something, or indirectly asking a statement to do something (even if that statement is just calling step
).
I wonder if there's a way to at least make the check faster.
Yes. I will make this better.
For posterity: #558 did not introduce sqlite3_close_v2
, it was introduced by #557 and is orthogonal to fork-safety.
The problem being described in this ticket is a valid performance regression but is not related to which close
method is being used. #565 addresses the performance issue by avoiding instance variable access in a hot C method.
I didn't fully realize the consequences of switching to
sqlite3_close_v2
when I reviewed #558. Sincesqlite3_close_v2
doesn't return a status code, it means that we can haveSQLite3::Statement
objects that point at a database which has been closed by someone.Since the database could have been closed, we have to check on every call to
step
whether or not the db is "ok", and unfortunately thestep
method is extremely hot. It's called for every row of the statement.For example, this benchmark:
At commit 480f3e7d, the results are like this:
At commit 81ea485f680c54f604ec1d0f40a00d999a149dc0, the results are like this:
This simple benchmark is substantially slower after switching to
sqlite3_close_v2
.Before da908657099a20f2796e49e6ed42c102a9aa7a80, the following code would raise an exception:
This meant that statements couldn't have "dangling references" to a database connection. Since this doesn't raise an exception anymore, it is possible and we must therefore check the validity of the db connection before fetching the next row.
Would it be possible to switch back to using
sqlite3_close
and raising an exception?I know the SQLite documentation says we should call
sqlite3_close_v2
in GC languages, but I think it only makes sense to call that function from the free function called by the GC. If a user callsclose
, we should usesqlite3_close
and raise an exception.Statements keep a reference to the database connection, so we shouldn't have to fear the db connection being closed via GC until the user is done using the statement. I've read #558 a few times, and it's not clear to me why
sqlite3_close_v2
is necessary for fork safety, but I could definitely be missing something.Thanks.