Thread: Crash in record_type_typmod_compare

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>
onlyafter 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.  Couldn't we just
teach record_type_typmod_compare to say "not equal" if it sees a
null tupdesc?

            regards, tom lane

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>
onlyafter 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

Andres Freund <andres@anarazel.de> writes:
> On 2021-03-31 13:10:50 -0400, Tom Lane wrote:
>> 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?

Yeah, if you have repeat failures there, which doesn't seem very likely.
Still, I take your point that we're doing it the first way in other
places, so maybe inventing a different way here isn't good.

            regards, tom lane

Hi,

On 2021-03-31 13:26:34 +0000, Sait Talha Nisanci wrote:
> diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
> index 4915ef5934..4757e8fa80 100644
> --- a/src/backend/utils/cache/typcache.c
> +++ b/src/backend/utils/cache/typcache.c
> @@ -1970,18 +1970,16 @@ assign_record_type_typmod(TupleDesc tupDesc)
>              CreateCacheMemoryContext();
>      }
>  
> -    /* Find or create a hashtable entry for this tuple descriptor */
> +    /* Find a hashtable entry for this tuple descriptor */
>      recentry = (RecordCacheEntry *) hash_search(RecordCacheHash,
>                                                  (void *) &tupDesc,
> -                                                HASH_ENTER, &found);
> +                                                HASH_FIND, &found);
>      if (found && recentry->tupdesc != NULL)
>      {
>          tupDesc->tdtypmod = recentry->tupdesc->tdtypmod;
>          return;
>      }
>  
> -    /* Not present, so need to manufacture an entry */
> -    recentry->tupdesc = NULL;
>      oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
>  
>      /* Look in the SharedRecordTypmodRegistry, if attached */
> @@ -1995,6 +1993,10 @@ assign_record_type_typmod(TupleDesc tupDesc)
>      }
>      ensure_record_cache_typmod_slot_exists(entDesc->tdtypmod);
>      RecordCacheArray[entDesc->tdtypmod] = entDesc;
> +    /* Not present, so need to manufacture an entry */
> +    recentry = (RecordCacheEntry *) hash_search(RecordCacheHash,
> +                                                (void *) &tupDesc,
> +                                                HASH_ENTER, NULL);
>      recentry->tupdesc = entDesc;

ISTM that the ensure_record_cache_typmod_slot_exists() should be moved
to before find_or_make_matching_shared_tupledesc /
CreateTupleDescCopy. Otherwise we can leak if the CreateTupleDescCopy()
succeeds but ensure_record_cache_typmod_slot_exists() fails. Conversely,
if ensure_record_cache_typmod_slot_exists() succeeds, but
CreateTupleDescCopy() we won't leak, since the former is just
repalloc()ing allocations.

Greetings,

Andres Freund

On Mon, 2021-04-05 at 12:07 +0000, Sait Talha Nisanci wrote:
> Hi Andres,
> 
> Please see the updated patch, do you mean something like this? (there
> might be a simpler way for doing this)
> 

Committed with minor revisions.

My patch also avoids incrementing NextRecordTypmod until we've already
called CreateTupleDescCopy(). Otherwise, an allocation failure could
leave NextRecordTypmod unnecessarily incremented, which is a tiny leak.

Regards,
    Jeff Davis