Make it possible to use copy_from/copy_to with dynamic tables #4549
Conversation
Previously, copy_from and copy_to would only be possible with tables known at compile time. It is now possible to also use them with tables that are only known at runtime. That is achieved by changing the signature of `CopyTarget::walk_target` to take a `self` argument.
There was a problem hiding this comment.
Thanks for opening this PR ❤️
Using an Rc there is unfortunately not possible, but should be easy to avoid.
Since this only changes some internal method signatures, does it need additional tests?
Should the docs for CopyTarget mention anything about tables only known at runtime, as that's a very niche use case?
I would prefer having an explicit example here to ensure that we don't break this again. That would serve as test + documentation together.
| @@ -372,7 +377,7 @@ where | |||
| #[must_use = "`COPY FROM` statements are only executed when calling `.execute()`."] | |||
| #[cfg(feature = "postgres_backend")] | |||
| pub struct CopyFromQuery<T, Action> { | |||
| table: T, | |||
| table: Rc<T>, | |||
There was a problem hiding this comment.
Using an Rc here is unfortunately a breaking change as this turns this struct from something that implements Send into something that doesn't implement Send.
Fortunately we don't need to explicitly store table inside of InsertableWrapper. Instead we can just pass a reference in through the method call chain.
There was a problem hiding this comment.
Thanks for the feedback! I'll incorporate it, but it might take me a little bit to get around to it
Previously, copy_from and copy_to would only be possible with tables known at compile time. It is now possible to also use them with tables that are only known at runtime. That is achieved by changing the signature of
CopyTarget::walk_targetto take aselfargument.A couple things I am not sure about with this PR, but more than happy to address:
CopyTargetmention anything about tables only known at runtime, as that's a very niche use case?Rc- I hope that's not a big issue