Allow custom OID serialization #154
Conversation
dullbananas
changed the title
|
Thanks for working on this. Unfortunately I won't have the capacity to review this soon due to the need to handle some cleanup after the last diesel release and because I'm away for holiday soon. It's on my list to look at this after I'm back in July. |
There was a problem hiding this comment.
I finally found some time to go over the changes and note some thoughts.
First of all I would like to thank you for working on this and finding this solution. That is really good work.
I have notes some ideas how to side step the more expensive parts of this approach that I think might be worth to try out, even if the introduce more complexity.
src/pg/mod.rs
Outdated
| /// Allows unambiguously determining: | ||
| /// * where OIDs are written in `bind_collector.binds` after being returned by `lookup_type` | ||
| /// * determining the maximum hardcoded OID in `bind_collector.metadata` | ||
| struct SameOidEveryTime { |
There was a problem hiding this comment.
I wonder whether we can add a flag to this type that tracks whether or not we hit a custom type (called the lookup function).
This would allow us to use this knowledge above to skip the second run of the bind collector and the comparison for the common case that we don't have any custom type.
There was a problem hiding this comment.
Another alternative would be to just collect the relevant type names here so that we already have them for later lookups. And we can check if that list is empty before doing the expensive stuff.
The other quick idea that I have is to use something uniquely identifiable per type instead a constant OID. Maybe a truncated hash value of the type name? (For the second variant we can just add a constant, because only the difference in the output is relevant, right?)
src/pg/mod.rs
Outdated
| /// * where OIDs are written in `bind_collector.binds` after being returned by `lookup_type` | ||
| /// * determining the maximum hardcoded OID in `bind_collector.metadata` | ||
| struct SameOidEveryTime { | ||
| first_byte: u8, |
There was a problem hiding this comment.
I likely would just use a 'u32' directly here instead of constructing it everytime in the lookup function.
src/pg/mod.rs
Outdated
| let collect_bind_result = | ||
| query.collect_binds(&mut bind_collector, &mut metadata_lookup, &Pg); | ||
|
|
||
| let fake_oid_locations = std::iter::zip(bind_collector_0.binds, bind_collector_1.binds) |
There was a problem hiding this comment.
I'm not sure if it might be worth to do that in the future anyway just do allow us to perform the comparison and the replacement in one pass.
src/pg/mod.rs
Outdated
| @@ -367,10 +366,35 @@ impl AsyncPgConnection { | |||
| // | |||
| // We apply this workaround to prevent requiring all the diesel | |||
| // serialization code to beeing async | |||
| let mut metadata_lookup = PgAsyncMetadataLookup::new(); | |||
| let mut bind_collector_0 = RawBytesBindCollector::<diesel::pg::Pg>::new(); | |||
| let collect_bind_result_0 = query.collect_binds( | |||
There was a problem hiding this comment.
We want to have some comment here why these steps are necessary and how the approach works from a top level view. Just to make sure that future refactoring won't break it again.
* Do not generate a second bind collector if we don't encounter a custom oid at all * Do not generate a third bind collector at all, we don't need that * Skip comparing buffers for types without custom oids as they won't contain any difference * Minor cleanup + documentation of the approach
|
I've pushed a bunch of cleanups + optimizations. This should be good to go now. |
Fixes #103