Improved "HeapTuple" support #532
Conversation
This constant was set to false in all every `FromDatum` implementation, and simply isn't necessary. Further changes to `FromDatum` are going to change how this idea worked in the first place.
This argument was never actually used, only passed around. And in many cases it was set to `pg_sys::InvalidOid` anyways.
```rust
} else if datum == 0 {
panic!("array was flagged not null but datum is zero");
}
```
As the function is already unsafe, there's no need for us to have a check for a case that is likely never going to happen. Postgres may send us a null datum, but as long as we check `is_null` first, we're okay.
This function ensures the desired Rust type is compatible with the backing Postgres type of the Datum being converted. If they're not compatible, an `Err` is returned rather than either panicking or leading to undefined behavior. It's not actually used anywhere yet.
Currently it only knows how to construct itself in a few ways, along with retrieving named/indexed datum values (as runtime type-checked Rust values). There's a few breaking API changes in other places, especially in `htup.rs` and `PgTupleDesc`
…mposite_type examples (the latter of which really isn't quite finished yet) accordingly.
|
Once this gets merged, I can move on to doing work on issue #499 (which I'm mentally calling "composite type" support) and then also retool our |
Hoverbear
changed the title
| &mut self, | ||
| attno: NonZeroUsize, | ||
| value: T, | ||
| ) -> FromDatumResult<()> { |
There was a problem hiding this comment.
It strikes me that this we could have some of this function extracted into a replace_at_index(..) -> Result<Self, _> which returns the new heaptuple, and call it here as well as exposing that.
There was a problem hiding this comment.
I don't follow?
There was a problem hiding this comment.
Given that this function always must create a copy, it might desirable to support a pattern like:
let cloned = my_tuple.replace_at_index(1, "thing");Instead of
let mut cloned = my_tuple.clone();
cloned.set_at_index(1, "thing");This would allow the user to avoid an extra clone happening. (and this function could call that one, so it's not added complexity on our side)
There was a problem hiding this comment.
PgHeapTuple doesn’t implement Clone so I still don’t follow…
| } | ||
| } | ||
|
|
||
| impl<'a, AllocatedBy: WhoAllocated<pg_sys::HeapTupleData>> PgHeapTuple<'a, AllocatedBy> { |
There was a problem hiding this comment.
Noting for @workingjubilee this is a similar pattern to how PgBox works.
There was a problem hiding this comment.
That's really poorly named. Instead of WhoAllocated it should be WhoNeedsToFreeThisThing.
The reason is, despite if it's AllocatedByRust or AllocatedByPostgres, it's a postgres palloc()'d pointer.
We can address this at a later time. Not relevant to this PR.
There was a problem hiding this comment.
Ya know, @workingjubilee, why don't you put up a PR for that (just off of current develop branch) and otherwise give PgBox an audit that only someone with your skills can do.
It's looking like our next release is going to have some pretty big API breakage anyways, so now would be a good time to rejigger naming things...
|
I'd like to make a detailed changelog entry for this and add it to the issue description for me to copy from on release. :) |
|
oh, I wanted to /cc @einhverfr, as it relates to: #528. This doesn't touch any of the Spi code yet, but this will become the foundation for retooling our Spi API. |
|
Reviewing this to my satisfaction (and getting a start on auditing PgBox's "drop logic") required doing a deep-dive into how PGX and indeed Postgres handles transmitting data types, and auditing that code took my yesterday. However, at my current level of understanding, this looks good to me. |
Cool. Thanks a bunch. Go ahead and smash that "approve" button. I'll probably want to hold off on merging this until you're done poking around with |
|
Hmm. I would probably feel best if this went ahead first, precisely because my audit started touching on potentially conflicting areas and it would give me freedom to rampage over this (or more likely it would reduce the diff, really). |
Okay. Let’s ask @Hoverbear to merge it in the morning. |
| @@ -18,7 +18,6 @@ pub struct Array<'a, T: FromDatum> { | |||
| array_type: *mut pg_sys::ArrayType, | |||
| elements: *mut pg_sys::Datum, | |||
| nulls: *mut bool, | |||
| typoid: pg_sys::Oid, | |||
There was a problem hiding this comment.
Carrying less state around is always an improvement.
| if is_null { | ||
| None | ||
| } else if datum == 0 { |
There was a problem hiding this comment.
Being able to skip these kinds of checks is what particularly helps a lot in reducing the diff I am working on.
|
After @workingjubilee 's feedback I'd love to see this merge, and can't wait to see what they follow up with! |
This PR provides a new
PgHeapTupletype that is capable of being constructed in various ways, along with reading and writing the individual attribute values in the underlyingHeapTuple.Fixes #502.
Breaking changes
FromDatum::from_datum's type signature has changed. It no longer needs anoidparameter.Other changes
HeapTupletype has been introduced which provides nice wrappers aroundheap_getattrand friends.FromDatum::try_from_datum()which returns aResult<T, TryFromDatumError>instead of anOptionallowing users to get context of why a converstion failed.