Hi,
On 2023-06-28 14:49:34 -0400, Tom Lane wrote:
> The bad news is that while investigating this I came across
> another hazard that seems much messier to fix. To wit, if
> you have a tuple with "missing" pass-by-ref columns, then
> some of the columns output by heap_deform_tuple() will
> contain pointers into the tupdesc (cf. getmissingattr()).
> That's not sufficient lifespan in the scenario we're dealing
> with here: if the tupdesc gets trashed anytime before the
> eventual ExecEvalFieldStoreForm(), we'll have garbage in the
> result tuple.
What are the scenarios that could lead to the tupledesc being released before
ExecEvalFieldStoreForm()? I guess a function call that does an ALTER TABLE
might do the trick? But shouldn't such cases be prohibited by
CheckTableNotInUse()? If other sessions caused the tupledesc to be changed,
we should already hang onto the old definition via the
RememberToFreeTupleDescAtEOX() mechanism?
What about erroring out if the get_cached_rowtype() in
ExecEvalFieldStoreForm() indicates the tupledesc changed? Hm, I guess that
could cause spurious errors, our mechanism for determining whether tupledescs
changed is pretty coarse.
> Worse, I fear there may be many other places with latent bugs of the
> same ilk. Before the attmissingval code was added, one could assume
> that the result of heap_deform_tuple would hold good as long as you
> had a pin on the source tuple. But now it depends on metadata as
> well, and I don't think we have a coherent story about that.
>
> Any thoughts what to do? I considered making getmissingattr
> apply datumCopy, but that would probably lead to unpleasant
> memory leaks.
Ugh. This is pretty nasty.
The only real thing I can think of is to use refcounted tupledescs more
widely. Part of the problem would be dealing with releasing the refcounts,
another what to do about non-refcounted descs. WRT the former, I guess we
could add a variant of ResourceOwnerRememberTupleDesc() that doesn't warn on
commit if there are unreleased references.
Greetings,
Andres Freund