pgrx icon indicating copy to clipboard operation
pgrx copied to clipboard

HeapTuple::get_by_index is not callable with array type

Open semtexzv opened this issue 2 years ago • 8 comments

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?

semtexzv avatar Dec 14 '22 21:12 semtexzv

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.

workingjubilee avatar Dec 15 '22 20:12 workingjubilee

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.

semtexzv avatar Dec 16 '22 12:12 semtexzv

~~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. :(

eeeebbbbrrrr avatar Dec 22 '22 00:12 eeeebbbbrrrr

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).

workingjubilee avatar Jan 04 '23 09:01 workingjubilee

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

semtexzv avatar Jan 04 '23 18:01 semtexzv

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 avatar Jan 04 '23 23:01 workingjubilee

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

workingjubilee avatar Apr 21 '23 23:04 workingjubilee

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.

eeeebbbbrrrr avatar May 26 '23 14:05 eeeebbbbrrrr