Re: Something seems weird inside tts_virtual_copyslot()

Hi,

On 2023-11-01 11:35:50 +1300, David Rowley wrote:
> Looking at the Assert inside tts_virtual_copyslot(), it does:
>
> Assert(srcdesc->natts <= dstslot->tts_tupleDescriptor->natts);

I think that assert was intended to be the other way round.

> So, that seems to indicate that it's ok for the src slot to have fewer
> attributes than the destination. The code then calls
> tts_virtual_clear(dstslot), then slot_getallattrs(srcslot); then does
> the following loop:

> for (int natt = 0; natt < srcdesc->natts; natt++)
> {
> dstslot->tts_values[natt] = srcslot->tts_values[natt];
> dstslot->tts_isnull[natt] = srcslot->tts_isnull[natt];
> }
>
> Seems ok so far.
>
> If the srcslot has fewer attributes then that'll leave the extra dstslot
> array elements untouched.

It is not ok even up to just here! Any access to dstslot->tts_{values,isnull}
for an attribute bigger than srcdesc->natts would now be stale, potentially
pointing to another attribute.

> Where it gets weird is inside tts_virtual_materialize(). In that
> function, we materialize *all* of the dstslot attributes, even the
> extra ones that were left alone in the for loop shown above. Why do
> we need to materialize all of those attributes? We only need to
> materialize up to srcslot->natts.
>
> Per the following code, only up to the srcdesc->natts would be
> accessible anyway:
>
> dstslot->tts_nvalid = srcdesc->natts;
>
> Virtual slots don't need any further deforming and
> tts_virtual_getsomeattrs() is coded in a way that we'll find out if
> anything tries to deform a virtual slot.
>
> 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.

> 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

The caller must ensure the slots have compatible tupdescs.

whatever that precisely means.

> Is the Assert() in tts_virtual_copyslot() wrong?

Yes, it's inverted.

Greetings,

Andres Freund