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

Support in FromDatum for binary coercible types (including domain types) #1028

Merged

Conversation

EdMcBane
Copy link
Contributor

@EdMcBane EdMcBane commented Feb 2, 2023

Add support for deserializing domain types into rust types compatible with the base type.
If the initial oid compatibility check fails, we iteratively check the type cache for the domain base type and test against that.

@eeeebbbbrrrr
Copy link
Contributor

Interesting. This is pretty simple. What might any downsides be?

Would we also want to get Postgres' IsBinaryCoercible() function involved before checking the typecache?

@EdMcBane
Copy link
Contributor Author

EdMcBane commented Feb 2, 2023

As my patch currently stands, it partially duplicates logic in isBinaryCoercible, so there might be maintenance overhead.

I (mistakenly?) assumed from our previous exchange that it was not the correct solution, but looking at it now it basically does, at least for domain types.
I'll explore that further and update this, integrating it might allow to handle more than just domain types.

From a performance standpoint:

  • the happy path is unaffected (as long as we first rely on the existing direct "supported oids" check in rust);
  • binary-coercible types would pay a slight penalty (FFI + typecache / typecast lookup);
  • try_from_datum with incompatible types would also pay a penalty, as above, but these are basically programming errors and thus the impact is negligible.

@eeeebbbbrrrr
Copy link
Contributor

Just coming back to this. My question about isBinaryCoercible is simply "should we also ask it", such as:

fn is_output_compatible<T: IntoDatum>(mut type_oid: pg_sys::Oid) -> bool {
     loop {
         if T::is_compatible_with(type_oid) {
             return true;
         } else if pg_sys:: isBinaryCoercible(T::type_oid(), type_oid) {
            return true;
         }

         type_oid = unsafe {
             (*pg_sys::lookup_type_cache(type_oid, pg_sys::TYPECACHE_DOMAIN_BASE_INFO as i32))
                 .domainBaseType
         };
         if type_oid == pg_sys::InvalidOid {
             return false;
         }
     }
 }

I know we can't only consider it, but I think we can include it in the list.

Thoughts?

@EdMcBane
Copy link
Contributor Author

Sorry it took me so long, but I've finally taken the time to look back at it, and I think that getting rid of the loop and using just IsBinaryCoercible is the right thing to do:

  • we remove any code duplication / overlap with postgres;
  • we need to pay the FFI price at most once;
  • we get support for more binary coercible types such as cidr to inet, and any other custom type registered by an extension.

It ends up looking like this:

fn is_output_compatible<T: IntoDatum>(type_oid: pg_sys::Oid) -> bool {
    T::is_compatible_with(type_oid) 
     || unsafe { pg_sys:: isBinaryCoercible(type_oid, T::type_oid()) }
}

⚠️ T::type_oid() needs to be the second argument, as coercion is not symmetric.

I've updated the branch and added a test for cidr to inet.
Does this look right? Did I miss anything?

@eeeebbbbrrrr
Copy link
Contributor

This seems reasonable.

I hope one day soon I'll have time to look at automatic casting between types. That'd be the ideal solution here. Being able to get back any type as a String would be sweeeeeet, for example.

@EdMcBane EdMcBane changed the title Support in FromDatum for domain types Support in FromDatum for binary coercible types (including domain types( Feb 16, 2023
@EdMcBane EdMcBane changed the title Support in FromDatum for binary coercible types (including domain types( Support in FromDatum for binary coercible types (including domain types) Feb 16, 2023
@eeeebbbbrrrr eeeebbbbrrrr merged commit 5faa2c3 into pgcentralfoundation:develop Feb 22, 2023
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