pgcentralfoundation / pgrx

Build Postgres Extensions with Rust!
Other
3.64k stars 248 forks source link

HeapTuple::get_by_index is not callable with array type #961

Open semtexzv opened 1 year ago

semtexzv commented 1 year ago

I'd like to read arrays from tuples, but the method bound requires T: IntoDatum, which Array<'_, T> is not.

Method signature:

pub fn get_by_index<T: FromDatum + IntoDatum + 'static>(
    &self,
    attno: NonZeroUsize,
) -> Result<Option<T>, TryFromDatumError> {

}

Is this just an oversight, or is there an underyling reason why this isn't pssible?

workingjubilee commented 1 year ago

We could look into relaxing the bound if you provide some example code that somewhat resembles what you are actually working with. Arrays sometimes are handled oddly by Postgres, Array<'_, T> has a lot of implicit assumptions about layout in how it handles data, and so I would want to make sure there's tests before we enabled that, to make sure we are correctly unpacking Array from HeapTuple, an Array which contains the same kind of data types you want to unpack, ideally.

semtexzv commented 1 year ago

Sure, code is here: https://github.com/semtexzv/pboutput/blob/2a1062eee71acec95866a15086edb5060caf1d66/src/lib.rs#L200.

Code deals with logical decoding. And I'm not sure how the HeapTuple manages TOASTed values. Goal is to just manually check for a specific OIDs and convert those values.

eeeebbbbrrrr commented 1 year ago

I think the reason for this bound is because of IntoDatum::is_compatible_with(). I had a thought that that ought to be on FromDatum instead. This would let us relax some of these bounds throughout the code. I know you assigned this to yourself, @workingjubilee but I may take a look next week after xmas.

Scratch that idea. I got squirreled on this for a minute and it's not that simple. :(

workingjubilee commented 1 year ago

The lifetime bound of 'static is actually probably a bigger problem than IntoDatum. It's not clear to me that relaxing it is correct (for reasons why it might not be, see tcdi/pgx#971).

On the bright side, however, for the pure trait-bound concern, FromDatum should in fact be sufficient, I think, as all the heap tuple attributes are, by my reading of postgres/[...]/heaptuple.c, in fact "just" Datums. This doesn't rule out the toasting concerns, and we may need to move is_compatible_with into its own trait or something, but that's comparatively trivial (albeit a potentially annoying migration for some users).

semtexzv commented 1 year ago

Removing of the 'static bound is not necessary for me. I just need to be able to read arrays from tuples

workingjubilee commented 1 year ago

Unfortunately it is very related. Array<'a, T> has a lifetime bound inside it. This reflects that the data is "borrowed" from somewhere, and it is bounded by the Postgres lifetimes, so if I didn't relax the bound of 'static and it worked, I would start looking into fixing that so it didn't work.

Unless you mean copying it into a Vec<T> or something would be fine, which may also make sense.

workingjubilee commented 1 year ago

This is most definitely blocked on whatever our solution to https://github.com/tcdi/pgrx/issues/971 turns out to be.

eeeebbbbrrrr commented 1 year ago

I believe this should be working now as of #1147, which added an IntoDatum impl for Array<T>.

It didn't occur to me to add any tests around this, so I'll do that real quick just to make sure.