Skip to content

Conversation

@mxork
Copy link

@mxork mxork commented Feb 1, 2020

Adds support for bytea.

This PR is rough. I mostly ripped off the implementation for Text where possible.

There's a lot of unsafe in here that I'm not sure I'm respecting the invariants of. I also don't understand the current state of allocation in the codebase (should I palloc or MemoryContextAlloc?).

If anyone has time to explain to me the Right Way to do things, I am happy to do a rewrite. Otherwise, hopefully someone can use this as stub code.

Copy link
Owner

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

This is looking good so far. Looks like you have a lot of todos, but I think the overall strategy looks good. I left comments in a few spots.

impl From<&[u8]> for PgDatum<'_> {
fn from(value: &[u8]) -> Self {
let ptr = unsafe{
// :consider should palloc be guard_pg'd?
Copy link
Owner

@bluejekyll bluejekyll Feb 1, 2020

Choose a reason for hiding this comment

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

yes, all calls (or blocks of calls) into PG functions should be guarded.

l &= 0x3fffffff;
l <<= 2;

unsafe {
Copy link
Owner

Choose a reason for hiding this comment

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

This all feels like it could be captured into a generic allocate and align function, maybe?

}
}

impl<'s> TryFromPgDatum<'s> for &[u8] {
Copy link
Owner

Choose a reason for hiding this comment

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

I think we want &'s [u8] here, right? and then we should make sure that 's is bound by 'mc, right? in other words, this byte array should be bounded by the lifetime of the memory context that encapsulates it.

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