Re: type cache cleanup improvements

Hi, Pavel!

On Wed, Aug 21, 2024 at 4:28 PM Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com> wrote:
> I've looked at patch v8.
>
> 1.
> In function check_insert_rel_type_cache() the block:
>
> +#ifdef USE_ASSERT_CHECKING
> +
> + /*
> + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash
> + * entry if it should exist.
> + */
> + if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) &&
> + typentry->tupDesc == NULL)
> + {
> + bool found;
> +
> + (void) hash_search(RelIdToTypeIdCacheHash,
> + &typentry->typrelid,
> + HASH_FIND, &found);
> + Assert(found);
> + }
> +#endif
>
> As I understand it does HASH_FIND after the same value just inserted by HASH_ENT
> ER above under the same if condition:
>
> if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) &&
> + typentry->tupDesc == NULL)
>
> Why do we need to do this re-check HASH_ENTER? Also I see "otherwise" in comment in a quoted block, but if condition is the same.

Yep, these are remains from one of my previous attempt. No sense to
check for HASH_FIND right after HASH_ENTER. Removed.

> 2.
> For function check_delete_rel_type_cache():
> I'd modify the block:
> +#ifdef USE_ASSERT_CHECKING
> +
> + /*
> + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash
> + * entry if it should exist.
> + */
> + if ((typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) ||
> + (typentry->flags & TCFLAGS_OPERATOR_FLAGS) ||
> + typentry->tupDesc != NULL)
> + {
> + bool found;
> +
> + (void) hash_search(RelIdToTypeIdCacheHash,
> + &typentry->typrelid,
> + HASH_FIND, &found);
> + Assert(found);
> + }
> +#endif
>
> as:
> +
> + /*
> + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash
> + * entry if it should exist.
> + */
> + else
> +{
> + #ifdef USE_ASSERT_CHECKING
> + bool found;
> +
> + (void) hash_search(RelIdToTypeIdCacheHash,
> + &typentry->typrelid,
> + HASH_FIND, &found);
> + Assert(found);
> +#endif
> +}

Changed in the way you proposed, except I put the comment inside the
#ifdef. I this it's easier to understand this way.

> 3. I think check_delete_rel_type_cache and check_insert_rel_type_cache are better to be renamed to be more clear, though I don't have exact proposals yet,

Renamed to delete_rel_type_cache_if_needed and
insert_rel_type_cache_if_needed. I've checked that

> 4. I haven't looked into comments, though I'd recommend oid -> OID replacement in the comments.

I've changed oid -> OID in the comments and in the commit message.

> Thank you for working on this patchset!

Thank you for review!

------
Regards,
Alexander Korotkov
Supabase