Re: Something seems weird inside tts_virtual_copyslot()

Hi,

On 2023-11-06 11:16:26 +1300, David Rowley wrote:
> 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.

I think my concerns might be unfounded - I was worried about stuff like
attribute mapping deciding that it's safe to copy without an attribute
mapping, because all the types match. But it looks like we do check that the
attnums match as well. There's similar code in a bunch of other places,
e.g. ExecEvalWholeRowVar(), but that also verifies ->natts matches.

So I think adding an assert to ExecCopySlot(), perhaps with a comment saying
that the restriction could be lifted with a bit of work, would be fine.

Greetings,

Andres Freund