Skip to content

PostgreSQL UUID and JSONB support #3175

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

itowlson
Copy link
Collaborator

Fixes #1551. Fixes #2936.

floating64(f64),
str(string),
binary(list<u8>),
date(tuple<s32, u8, u8>), // (year, month, day)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A tuple that needs a comment to explain its elements sounds like it wants to be a record . 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes: unfortunately, this was a mistake I made in v3 and I don't see value in changing it for v4 (yes, I know I am allowed to).

@itowlson
Copy link
Collaborator Author

This PR now also adds the following types:

  • NUMERIC. Currently I have this represented at the WIT level as a string, which I realise will be controversial. The reason for suggesting this was that the decimal representation is highly portable and unambiguous. After looking at how Go and Rust decimal libraries construct their types, I was concerned that a more sensible mantissa-scale approach would likely be error prone. But I'm very open to better ideas (heck, I'm open to worse ones if you can think of any).
  • INTERVAL
  • Arrays: INT4[], INT8[], NUMERIC[], TEXT[]. Only single level supported, and these are done with option lists to allow for NULLs. (It is technically inconsistent to use option-none rather than the DbNull value, but WIT doesn't allow for recursive variants, so here we are. Again, open to better ideas.)
  • Ranges: INT4RANGE, INT8RANGE, NUMRANGE
  • UUID and JSONB as mentioned in the title

cc @tharpooljha

There is still significant tidying to do: the new types make the Postgres code more complicated and easier to miss cases, and I had to add some helper types DIDN'T I THE ORPHAN RULE so it's pretty messy right now. I'll work on cleaning it up but anyway here it is so far.

array-str(list<option<string>>),
interval(interval),
db-null,
unsupported,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have a e.g. raw(list<u8>)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you say more about what you're envisaging?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess raw would effectively be "other".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Intercepting that may require us a bit more FromSql magic, I am not sure - we access values via get, which expects to return into a FromSql implementation that matches the column type, but maybe it will let us return anything into a Vec<u8>, or otherwise I can create a fake FromSql that accepts all types and just captures the raw bytes. I will take a look!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thought: maybe this "other" variant could be a resource that additionally exposes type information about the value, like https://docs.rs/tokio-postgres/latest/tokio_postgres/types/struct.Type.html#method.oid

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your lightest whim is my command (subject to applicable law and terms of service). I provisionally put the type name rather than the OID but if you have opinions then bring 'em on, I am utterly naive about this stuff.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know how these are used in practice either, I'm just trying to save you from spending the rest of your days adding support for an neverending list of increasingly obscure data types. 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed - this has me thinking "what if I rewrote the whole interface to just tunnel EVERYTHING as blobs and let the guest language use whatever decoder is idiomatic" so your plan to free me from this rabbit hole may have backfired...

(Fear not. I do not intend to do this as part of this PR. Nor without a lot more research into the factoring of guest language DB libraries.)

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.

Feature request: Support for JSONB in postgresql uuid data type on spin_sdk::pg threw an exception
2 participants