Skip to content
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

Add support for prepared statements #13

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

yerTools
Copy link

Hello :)

I added support for prepared query statements to improve overall performance when queries are reused.
Since both libraries (Erlang and JavaScript) use prepared statements under the hood when calling query, I split this function into prepare and query_prepared.

I also updated every test that uses sqlight.query by introducing a helper function that ensures both functions return the same errors and results.

I had to wrap the database connection in sqlight_ffi.js to keep track of the created prepared statements since the library requires that they be finalized before closing the connection. I think this isn't the most elegant solution, but I'm not aware of a better approach.

I also updated the README example, the CHANGELOG, and bumped the version to 0.10.0.

Thank you very much for the great work, and I'm hoping I didn't miss anything this time. :)

Copy link
Owner

@lpil lpil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you.

Could you write a test for what happens when the connection has been closed and then the statement is run. Thank you

src/sqlight.gleam Outdated Show resolved Hide resolved
src/sqlight_ffi.erl Outdated Show resolved Hide resolved
test/sqlight_test.gleam Outdated Show resolved Hide resolved
@yerTools
Copy link
Author

Thank you very much for the code review. I wrote some tests and got inconsistent results depending on the target platform.
Since you have more experience, I wanted to ask for your opinion before addressing these missing tests.


1. Running a normal query after the connection was closed:

  • Erlang panics with the failure badarg.
  • JavaScript returns an error: SqlightError(code: GenericError, message: "Database was closed.", offset: -1)

This inconsistency existed before this PR.
Can this behavior be tolerated, or is it a bug? If it's the latter, how should it be fixed?
I think it's better to return an error than to panic, but I currently have no idea how to prevent the panic.


2. Running a prepared statement after the connection was closed:

  • Erlang returns the expected result (perhaps because the query is very simple?).
  • JavaScript returns an error: SqlightError(code: GenericError, message: "Query is finalized.", offset: -1)

Since Gleam is immutable, I don't know how to keep track of whether the connection has been closed, other than wrapping the whole thing inside a process—which either breaks target support or introduces overhead.
I think returning an error would be the right way to handle it, but I'm not sure of the best approach.


I'm looking forward to your opinion on this topic. The best solution I currently have in mind is to document it as undefined behavior on the close function. You probably have a better solution. :)

Thank you

@yerTools
Copy link
Author

yerTools commented Nov 26, 2024

Hello,

I used this version in a private project of mine, and tests failed. After a few hours of debugging, I found the problem. If you don't reset the prepared statement after an error occurs, this error will reappear when the prepared statement is used again.

This is now solved, and I added the test case which triggered this bug by not resetting the prepared statement after each use. I also set a flag to indicate that these prepared statements are persistent.

Thank you very much! :)

@lpil
Copy link
Owner

lpil commented Dec 5, 2024

Great detective work there! Bravo!

That difference between the targets today is a bug. We'll have to make an issue for it.

I'm confused by the Erlang one where the prepared statement works even though it was closed. Do you understand why it is that works?

@yerTools
Copy link
Author

yerTools commented Dec 5, 2024

Thank you very much! <3

No, I have no clue why this behavior exists, but I can look into it when I have the motivation to do so. ;)


Regarding the closing issue:

It's probably not the best solution because I have no prior Erlang experience, and I'm probably somewhere on Mount Stupid (Dunning–Kruger effect), but my chain of thought is the following:

  1. To keep track of whether a connection was closed, we need mutable state.
  2. Creating mutable state in Erlang requires a process.
  3. If we don't want to modify the ESQLite library, we can do this at the level of this library.
  4. We can't use Gleam's actors, so we have to implement this in the FFIs for Erlang and JavaScript.
  5. This way we can guarantee the same behavior independently of the target platform and also fix future problems if and when they occur.

Since I also wrote connection pooling for my application, and I think this would be a generally useful feature, I would suggest the following:

  1. Writing native support for pooling on each platform.
  2. Using the pool to keep track of the closing state and handle use-after-close cases there.
  3. Wrapping the current API around the pool to create one with a single connection, so we do not introduce breaking changes.
  4. Exposing a new API for creating pooled connections.

I would be willing to implement this new feature in a new pull request, since I think this would be a great chance to get another code review from you. :3

Since I don't know if you even want this feature in your library, and it isn't that trivial IMHO, some input from you would be great!


Either way, thank you very much!
- Felix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants