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