Re: Crash in record_type_typmod_compare

Hi,

On 2021-03-31 13:10:50 -0400, Tom Lane wrote:
> Sait Talha Nisanci <Sait.Nisanci@microsoft.com> writes:
> >> We should probably do HASH_ENTER<https://github.com/postgres/postgres/blob/1509c6fc29c07d13c9a590fbd6f37c7576f58ba6/src/backend/utils/cache/typcache.c#L1974> only after we have a valid entry so that we don't end up with a NULL entry in the cache even if an intermediate error happens. I will share a fix in this thread soon.
> > I am attaching this patch.
> 
> I see the hazard, but this seems like an expensive way to fix it,
> as it forces two hash searches for every insertion.

Obviously not free - but at least it'd be overhead only in the insertion
path. And the bucket should still be in L1 cache for the second
insertion...

I doubt that the cost of a separate HASH_ENTER is all that significant
compared to find_or_make_matching_shared_tupledesc/CreateTupleDescCopy?

We do the separate HASH_FIND/HASH_ENTER in plenty other places that are
much hotter than assign_record_type_typmod(),
e.g. RelationIdGetRelation().

It could even be that the additional branches in the comparator would
end up costing more in the aggregate...


> Couldn't we just
> teach record_type_typmod_compare to say "not equal" if it sees a
> null tupdesc?

Won't that lead to an accumulation of dead hash table entries over time?

It also just seems quite wrong to have hash table entries that cannot
ever be found via HASH_FIND/HASH_REMOVE, because
record_type_typmod_compare() returns false once there's a NULL in there.

Greetings,

Andres Freund