Add SqlStr

[joeydewaal]

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.

[abonander]

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]

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.

[joeydewaal]

CI is passing and this PR is using your commit, so this should be ready for the next step. What should the replacement for the Execute trait look like?

Also while going through some of the lifetimes I noticed that the Arguments and ArgumentBuffer currently have a lifetime that doesn't feel useful. Only the Sqlite driver is using them but calls SqliteArguments::into_static before sending them through a channel here.
If I'd have to guess I'd say that the lifetimes would've probably been used before #1551 but were not touched because of backwards compatibility.
Since you're trying to move towards a background task and communicating through channels, should these be removed? The 'static lifetime on channels would make this lifetime unusable in the future. This would also remove the lifetime on the Encode and Execute trait which would clean the lifetimes on the Executor trait up quite a bit.

[abonander]

Yeah, deleting the lifetimes on Arguments was an idea, though AnyArguments also uses it when encoding &str and &[u8]. It saves a copy when later encoding into the database-specific format.

[joeydewaal]

Ah I forgot to check the Any driver, that's unfortunate because it would solve #1430 and #1151.

[abonander]

CI should be fixed if you rebase.

[abonander]

@joeydewaal do you have time to address the merge conflicts?

[joeydewaal]

Yes, I'll work on this today

[abonander]

Another bikeshedding question: should we change the name of QueryBuilder::push(), since it allows adding unsanitized data to the query? It's already got a warning on it, but I'm wondering if the name is missing some nuance. I guess use of QueryBuilder is already something that's going to draw a bit of scrutiny since the onus is already on the user to ensure it generates syntactically valid SQL.

[joeydewaal]

I don't really have a strong opinion on that, but since we trying to make it really safe we might as well. What name did you have in mind, I'm thinking of push_unsanitized?

[abonander]

Yeah, let's deprecate push in favor of push_unsanitized and see what feedback we get on it.

[abonander]

On second thought, I'd rather just merge this. I think QueryBuilder already draws enough attention to itself.

[tisonkun]

The fact that Execute::sql(self) takes ownership prevents downstream projects from wrapping the connection to attach AOP logics:

    fn fetch_many<'e, 'q, E>(
        self,
        query: E,
    ) -> Pin<Box<dyn Stream<Item = Result<Either<PgQueryResult, PgRow>, Error>> + Send + 'e>>
    where
        'q: 'e,
        'c: 'e,
        E: 'q + Execute<'q, Self::Database>,
    {
        let query_sql = query.sql();
        let timeout_at = Instant::now() + self.timeout;

        Box::pin(try_stream(async move |tx| {
            let mut steam = self.conn.fetch_many(query);

            while let Some(item) = runtime::timer()
                .timeout_at(timeout_at, steam.next())
                .await
                .map_err(|_| {
                    Error::Protocol(format!(
                        "meta query timeout after {:.3}s: {}",
                        self.timeout.as_secs_f64(),
                        query_sql.as_str()
                    ))
                })?
            {
                tx.send(item?).await;
            }

            Ok(())
        }))
    }

cannot compile because:

use of moved value: query [E0382]

[abonander]

I believe we did this so that .sql() didn't unnecessarily touch the refcount of the string, since most of the time the object is being consumed anyway.

We would need to add a separate .get_sql() method (and ideally rename .sql() to .into_sql() for clarity), but that would be a breaking change since the trait is not sealed and there'd be no reasonable way to add it as a default method besides having it panic.

0.9.0 has had an alpha release out since October. Did you not try it?

[tisonkun]

We would need to add a separate .get_sql() method (and ideally rename .sql() to .into_sql() for clarity), but that would be a breaking change since the trait is not sealed and there'd be no reasonable way to add it as a default method besides having it panic.

Sounds reasonable. Most SqlStr in my case are short enough to clone.

0.9.0 has had an alpha release out since October. Did you not try it?

Nope. I believe trying to upgrade the dependency as soon as it's out is already a tight connection; testing alpha is one level up :P

I don't know there was a released alpha version. Perhaps Cargo can notify that or the project can notify early testers in some way.