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

Customizable serialization of PostgresType #965

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

yrashk
Copy link
Contributor

@yrashk yrashk commented Dec 15, 2022

Problem: CBOR is not the most universal answer to everything

However, all Serialize/Deserialize PostgresTypes are forced to use with the blanket implementation of IntoDatum/FromDatum

Solution: extract a Serializer and Deserializer traits to abstract this away

By default, it uses CBOR and encodes it into a Varlena, replicating existing behavior.

However, this gives a good amount of flexibility to end-user to determine their encoding needs.

I've also removed JSON-encoding/decoding functions from varlena module, as they don't seem to be used anywhere.


A message to pgx users: my ability to actively contribute to pgx largely depends on your support. Please consider sponsoring my work

@syvb
Copy link
Contributor

syvb commented Dec 22, 2022

This is useful for the pgx extension I work on, since we use a non-CBOR serialization for our aggregate states (Bincode) and it would be great if we were able to use pg_aggregate for them.

@eeeebbbbrrrr
Copy link
Contributor

hey @yrashk! this fell off my radar somehow. I think it looks good in general. Would you mind rebasing so we can see it without any conflicts? I'd like to merge if we can

@yrashk
Copy link
Contributor Author

yrashk commented Mar 8, 2023

Absolutely! Will look into this shortly.

However, all `Serialize/Deserialize` PostgresTypes are forced to use it
with the blanket implementation of `IntoDatum`/`FromDatum`

Solution: extract a `Serializer` trait to abstract this away

By default, it uses CBOR and encodes it into a Varlena, replicating
existing behavior.

However, this gives a good amount of flexibility to end-user to
determine their encoding needs.

I've also removed JSON-encoding/decoding functions from varlena module,
as they don't seem to be used anywhere.
Makes it unclear whether it's advisable to use it externally.

Solution: document it
It takes care of both serialization and deserialization. However, some
types can potentially be only deserialized, or serialized. Besides, the
naming of the trait itself showed that it is not a perfect match.

Solution: split it into Serializer and Deserializer, respectively
@yrashk
Copy link
Contributor Author

yrashk commented Mar 8, 2023

Here's my first take on rebasing. It still requires a bit more of a review as things have changed and I may have missed something. The test passes.

For all we know, it may still be using CBOR.

Solution: ensure we're getting our custom serialization
@yrashk
Copy link
Contributor Author

yrashk commented Mar 29, 2023

@eeeebbbbrrrr did you have a chance to look at this updated PR yet?

@eeeebbbbrrrr
Copy link
Contributor

I have not. :( I’ll tackle it next week. Thanks for the ping.

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.

3 participants