Hi, On 2019-02-05 22:44:38 -0300, Alvaro Herrera wrote: > On 2019-Feb-05, Andres Freund wrote: > > > @@ -82,7 +80,7 @@ static Datum getmissingattr(TupleDesc tupleDesc, int > > attnum, bool *isnull); > > /* > > * Return the missing value of an attribute, or NULL if there isn't one. > > */ > > -static Datum > > +Datum > > getmissingattr(TupleDesc tupleDesc, > > int attnum, bool *isnull) > > This is a terrible name for an exported function -- let's change it > before it gets exported. Heck, even heap_getmissingattr() would be > better.
I don't really aggree. Note that the relevant datastructure is named AttrMissing, and that the function isn't relevant for heap tuple. So I don't really see what we'd otherwise name it? I think if we wanted to improve this we should start with AttrMissing and not this function. > I notice that with this patch, heap_getattr() obtains a new Assert() > that the attr being fetched is no further than tupledesc->natts. > It previously just returned null for that case. Maybe we should change > it so that it returns null if an attr beyond end-of-array is fetched? > (I think in non-assert builds, it would dereference past the AttrMissing > array.) Hm, it seems like there's plenty issues with accessing datums after the end of the desc before this change, as well as after this change. Note that slot_getattr() (in 11, it looks a bit different in master) already calls getmissingattr(). And that's much more commonly used. Therefore I'm disinclined to see this as a problem? Greetings, Andres Freund
