Re: BUG #17994: Invalidating relcache corrupts tupDesc inside ExecEvalFieldStoreDeForm()

On 2023-06-28 We 17:57, Andrew Dunstan wrote:
>
>
> On 2023-06-28 We 16:54, Andres Freund wrote:
>> Hi,
>>
>> On 2023-06-28 15:52:28 -0400, Tom Lane wrote:
>>> Andres Freund<andres@anarazel.de>  writes:
>>>> If other sessions caused the tupledesc to be changed,
>>>> we should already hang onto the old definition via the
>>>> RememberToFreeTupleDescAtEOX() mechanism?
>>> I believe the tupdesc in question is actually in the typcache,
>>> which doesn't have anything like RememberToFreeTupleDescAtEOX
>>> (which is a horrid hack anyway if you ask me).
>> It's in the typecache, but that just uses the relcache's tupledesc for
>> non-record composites. But looks like it doesn't suffice, because
>> TypeCacheRelCallback() releases the refcount the typecache held, regardless of
>> the tupledesc having changed meaningfully or not.
>>
>> So even if there can't have been "important" changes to the tupledesc due to
>> locking, we end up with the newer tupledesc on a second lookup...
>>
>>
>> I agree that the RememberToFreeTupleDescAtEOX thing is a ugly hack, but I
>> don't think it's easy to come up with something good...
>>
>>
>>> We could probably make things better for this specific case by
>>> teaching the typcache not to replace a cached tupdesc unless its
>>> contents actually change.  But that just makes it harder to get
>>> to a bug instance; it's not a cure-all.
>> Yea :(.
>>
>
> :-(
>
> I thought about  whether the datumCopy() idea might be manageable, but 
> getmissingattr() is inlined by heap_getttr() which has distressingly 
> large number of call sites, including in third party code.
>
> Could we maybe cache missing values elsewhere in a way that's less 
> volatile? That's an extremely vague idea, just thinking out loud.
>
>
>

After (not) sleeping on this overnight, and discussing it with a 
colleague this morning, here's a suggestion. We have a hash table, keyed 
by (tdtypeid, attnum) where we store a datumCopy'd version of the value. 
If it's present just return the value instead of getting it from the 
tupdesc. The hash table is blown away at the end of the transaction. 
Assuming that's workable I think it would not be a large patch.

My colleague did ask if we had live reproducible case.


cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com