Re: type cache cleanup improvements - Mailing list pgsql-hackers

Hi, Pavel!


On Wed, Aug 21, 2024 at 4:28 PM Pavel Borisov <pashkin.elfe@gmail.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!