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 SqlStr #3723

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

Add SqlStr #3723

wants to merge 7 commits into from

Conversation

joeydewaal
Copy link
Contributor

Not sure if it's appreciated to take over a PR but I thought I'd try and work #3364 out.

This PR changes the following things:

  • Add the SqlStr, AssertSqlStr types and SqlSafeStr trait.
  • Removes the lifetime of Statement since the underlying sql string is now owned.
  • Updates the Executor trait to take a SqlStr instead of a &'q str.
  • Changes the AnyStatement::try_from_statement method to take a Statement so the prepare_with method on AnyConnectionBackend doesn't have to clone the SqlStr.

There are still lifetimes that should be changed/relaxed, but this works.

@joeydewaal joeydewaal changed the title Add Sqlstr Add SqlStr Feb 1, 2025
@abonander
Copy link
Collaborator

abonander commented Feb 2, 2025

I appreciate the effort on this, but in #3364 kind of got stuck on some design questions. I was thinking that I would:

  • Delete the Execute trait because it adds unnecessary monomorphization.
  • Replace the methods on Executor with fetch_prepared() and fetch_unprepared()
    • Maybe execute_prepared() and execute_unprepared() as well, to tell the driver to ignore returned rows.
    • Use structs to pass arguments to these so additional parameters can be added without it being a breaking change.
    • Add a batch_size parameter to make queries cancellable (default to 0/infinite to fix Parallel workers not used on Postgres #3673)
  • Try to figure out all the lifetime issues with Executor
  • Lay some of the groundwork for the execution model refactor I've been meaning to do (run the connection on a background task for parallelism and cancellation safety)
    • I was experimenting with a new batched SPSC channel design that could achieve insane throughput in 100% safe code (10x faster than flume or tokio::sync::mpsc): e5c2380
    • This was going to use batch_size by default to set the capacity of the channel but because of Parallel workers not used on Postgres #3673 we might need a separate parameter (or just choose a decent default).

It's extremely hard to resist scope creep with major refactors like this. At minimum I would still delete the Execute trait though.

(Also, I don't assume you intended to do this, but you erased my authorship in these commits.)

@joeydewaal
Copy link
Contributor Author

Lay some of the groundwork for the execution model refactor I've been meaning to do (run the connection on a background task for parallelism and cancellation safety)

This would be great, also dropping transactions would probably be easier.

It's extremely hard to resist scope creep with major refactors like this. At minimum I would still delete the Execute trait though.

Ah that's why the Execute trait was missing. I wouldn't mind updating this PR. But you'd have to explain a bit how it should look.
Also if you'd rather make these changes (SqlStr, Execute, ...) yourself I wouldn't mind. I don't want to waste your/my time with this PR if you'd rather want to go in a different direction. That said I have the time so I wouldn't mind increasing the scope a bit.

(Also, I don't assume you intended to do this, but you erased my authorship in these commits.)

This wasn't my first time trying to make this PR work, in my first attempt I tried working on top of your PR but there were too things changed in the query{_as, _scalar}.rs files and wanted to work on a clean branch. I eventually just copied over the sql_str.rs file but didn't think about authorship. I'll rebase when I work on this some more.

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