On Fri, 27 Oct 2023 at 22:05, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Fri, Oct 27, 2023 at 8:48 AM David Rowley <dgrowleyml@gmail.com> wrote:
> > I was uncertain if the old behaviour of when srcslot contains fewer
> > attributes than dstslot was intended or not. What happens there is
> > that we'd leave the additional old dstslot tts_values in place and
> > only overwrite up to srcslot->natts but then we'd go on and
> > materialize all the dstslot attributes. I think this might not be
> > needed as we do dstslot->tts_nvalid = srcdesc->natts. I suspect we may
> > be ok just to materialize the srcslot attributes and ignore the
> > previous additional dstslot attributes. Changing it to that would
> > make the attached patch more simple.
>
> We seem to use both tt_nvalid and tts_tupleDescriptor->natts. I forgot
> what's the difference. If we do what you say, we might end up trying
> to access unmaterialized values beyond tts_nvalid. Better to
> investigate whether such a hazard exists.
The TupleDesc's natts is the number of attributes in the tuple
descriptor. tts_nvalid is the greatest attribute number that's been
deformed in the tuple slot. For slot types other than virtual slots,
we'll call slot_getsomeattrs() to deform more attributes from the
tuple.
The reason the code in question looks suspicious to me is that we do
"dstslot->tts_nvalid = srcdesc->natts;" and there's no way to deform
more attributes in a virtual slot. Note that
tts_virtual_getsomeattrs() unconditionally does elog(ERROR,
"getsomeattrs is not required to be called on a virtual tuple table
slot");. We shouldn't ever be accessing tts_values elements above
what tts_nvalid is set to, so either we should be setting
dstslot->tts_nvalid = to the dstdesc->natts so that we can access
tts_values elements above srcdesc->natts or we're needlessly
materialising too many attributes in tts_virtual_copyslot().
David
David