Re: Something seems weird inside tts_virtual_copyslot()

On Sat, 4 Nov 2023 at 15:15, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> On 2023-11-01 11:35:50 +1300, David Rowley wrote:
> > I changed the Assert in tts_virtual_copyslot() to check the natts
> > match in each of the slots and all of the regression tests still pass,
> > so it seems we have no tests where there's an attribute number
> > mismatch...
>
> If we want to prohibit that, I think we ought to assert this in
> ExecCopySlot(), rather than just tts_virtual_copyslot.
>
> Even that does survive the test - but I don't think it'd be really wrong to
> copy from a slot with more columns into one with fewer. And it seems plausible
> that that could happen somewhere, e.g. when copying from a slot in a child
> partition with additional columns into a slot from the parent, where the
> column types/order otherwise matches, so we don't have to use the attribute
> mapping infrastructure.

Do you have any examples of when this could happen?

I played around with partitioned tables and partitions with various
combinations of dropped columns and can't see any cases of this. Given
the assert's condition has been backwards for 5 years now
(4da597edf1b), it just seems a bit unlikely that we have cases where
the source slot can have more attributes than the destination.

Given the Assert has been that way around for this long without any
complaints, I think we should just insist that the natts must match in
each slot. If we later discover some reason that there's some corner
case where they don't match, we can adjust the code then.

I played around with the attached patch which removes the Assert and
puts some additional Assert checks inside ExecCopySlot() which
additionally checks the attribute types also match. There are valid
cases where they don't match and that seems to be limited to cases
where we're performing DML on a table with a dropped column.
expand_insert_targetlist() will add NULL::int4 constants to the
targetlist in place of dropped columns but the tupledesc of the table
will have the atttypid set to InvalidOid per what that gets set to
when a column is dropped in RemoveAttributeById().

> > I think if we are going to support copying slots where the source and
> > destination don't have the same number of attributes then the
> > following comment should explain what's allowed and what's not
> > allowed:
> >
> > /*
> > * Copy the contents of the source slot into the destination slot's own
> > * context. Invoked using callback of the destination slot.
> > */
> > void (*copyslot) (TupleTableSlot *dstslot, TupleTableSlot *srcslot);
>
> Arguably the more relevant place would be document this in ExecCopySlot(), as
> that's what "users" of ExecCopySlot() would presumably would look at. I dug a
> bit in the history, and we used to say

I think it depends on what you're documenting. Writing comments above
the copyslot API function declaration is useful to define the API
standard for what new implementations of that interface must abide by.
Comments in ExecCopySlot() are useful to users of that function. It
seems to me that both locations are relevant. New implementations of
copyslot need to know what they must handle, else they're left just to
look at what other implementations do and guess the rest.

David