tiberius icon indicating copy to clipboard operation
tiberius copied to clipboard

Statements

Open pimeys opened this issue 5 years ago • 5 comments

For better performance with lots of repetitive queries, a proper statement system would be nice. Whenever we execute a query, the server responds with a handle that can be reused with different parameters in the future. This means the query is parsed only once, saving the parsing time in the subsequent calls.

The work required:

  • query and execute should be able to take either a string or a statement as a parameter.
  • prepare function to create a new statement
  • A way of removing dropped statements from the server session

A good example how this could work can be found from steffengy's version.

Options for an API:

a) User keeps statements in memory and uses them when fit. b) We keeping statements in memory in Tiberius, hashing the incoming queries and using the already stored statement if possible.

If taking the approach b, we should be able to turn the feature off by configuration.

pimeys avatar May 09 '20 20:05 pimeys

Also what makes this a bit questionable is that the server already caches statements, so creating your own statement object to be cached in the user application is probably not that useful.

pimeys avatar Jun 08 '20 17:06 pimeys

I think it should be up to the library user to decide whether they want to prepare a statement or kick it off only once. This should make state management easier as the client can just return a Statement object which is initialized using sp_prepare und uses the Drop mechanism to call sp_unprepare once the caller doesn't need it anymore. The tricky part will be getting the proper lifetime done so that returned statement cannot outlast the client connection but I'm optimistic that this can be expressed in Rust, too.

milgner avatar Dec 04 '21 23:12 milgner

You cannot really do any IO on Drop, due to Rust not having an async Drop mechanism. Tiberius is also not bound to any runtime, meaning triggering a spawn (which doesn't need to be called from an async context) is not super clean and easy.

What we could do is to convert the crate to do more of an actor model of execution. The client talks to the connection through a channel, and the user must spawn the connection to the runtime while keeping the client for themselves. This is quite a large refactoring and I'm not 100% sure I like the tradeoff of the added complexity.

And, finally, just implementing the statement and noting in the docs how you must manually release the statement before dropping. Not the greatest API, but if you feel it could be useful, do you want to try your luck implementing it?

Edit: @esheppa has been working on this actor mechanism. Not saying no to merging that, but we must have a good reason to make the whole stack more complex. https://github.com/prisma/tiberius/pull/129

pimeys avatar Dec 06 '21 22:12 pimeys

@pimeys thank you for summing it up! I hadn't looked at the implementation-side of things yet and was thinking about it from the API user perspective. Not being able to run async code in the context of Drop is indeed an issue...

Personally I'd prefer to not put the responsibility for cleaning up on the user. From what I understand so far, the server will not dispose of the handle by itself and the library user can easily run into situations where the client is dropped for some reason, leaking the prepared statements.

Another idea would be to inject a runner into the client that can drive the future. Depending on the async library used this could then be async_std::task::spawn or the spawn method of a Tokio runtime. This would make client initialisation a bit more complex but could be worked around with a feature flag that provides a default implementation for both tokio and async-std.

Finally my colleague @phlay suggested that it might be possible to employ the must_use attribute to enforce the library user to call some kind of "cleanup" method. Upon closer inspection it did not look that straightforward, though and would probably require extensive redesign of the interface, too.

I probably won't find time to go into implementation time for this this week, so I'll let it simmer in my head for a bit and re-visit it this weekend.

milgner avatar Dec 07 '21 13:12 milgner

I'm really thinking of reviving the PR #129 for this purpose. It makes things a bit more complex, but has been working quite well for the tokio-postgres project. Channels are also runtime-independent, so we don't need to do any #[cfg] ugliness to get it working on all the different spawn implementations.

What this means architecture-wise:

  • Client would not hold the wire protocol, but would send a message to a channel and returns a Receiver to the user, who awaits it.
  • Connection must be spawned by the user on init to the runtime and . This connection gets messages from the channels and then responds when having data in the wire.
  • Connection is awaiting a channel, and awakes on a message that is a tuple of (Sender, Payload).
  • It will execute the Payload and when done, sends the response through Sender.
  • User who awaits the Receiver now wakes up and gets data.

How it helps with Transaction and Statement:

  • Drop has &mut self, and self holds a handle to the main channel
  • It can send a message to the channel, here again (Sender, Payload), where Payload handles the deletion of the tx/stmt.
  • It will not await and just exits at this point.
  • The connection running in another thread gets the message, handles it and responds to nobody due to Drop being already done.

Of course the Drop doesn't guarantee anything. If the connection is saturated or just in a weird state, we might not drop a transaction or statement in the server properly. This might be hard to debug and would be something against this idea of an API. It has been working quite well for tokio-postgres, so it might be not that big of an issue.

I think some kind of transaction/statement APIs are required before Tiberius can stabilize to 1.0. We can either refactor it now, or wait until Rust community comes with a working AsyncDrop trait and implement them then.

pimeys avatar Dec 07 '21 14:12 pimeys