Horton

Re: type cache cleanup improvements

Alexander Korotkov <aekorotkov@gmail.com>

View thread
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Pavel Borisov <pashkin.elfe@gmail.com>
Cc: Andrei Lepikhov <a.lepikhov@postgrespro.ru>, Teodor Sigaev <teodor@sigaev.ru>, Pgsql Hackers <pgsql-hackers@postgresql.org>, Aleksander Alekseev <aleksander@timescale.com>, Tom Lane <tgl@sss.pgh.pa.us>, Michael Paquier <michael@paquier.xyz>
Date: 2024-08-21T15:28:57Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Maintain RelIdToTypeIdCacheHash in TypeCacheOpcCallback()

  2. Fix concurrrently in typcache_rel_type_cache.sql

  3. Avoid looping over all type cache entries in TypeCacheRelCallback()

  4. Update header comment for lookup_type_cache()

  5. Revert: Avoid looping over all type cache entries in TypeCacheRelCallback()

  6. Introduce hash_search_with_hash_value() function

  7. Optimize InvalidateAttoptCacheCallback() and TypeCacheTypCallback()

  8. Refactor initial hash lookup in dynahash.c

  9. Rationalize and improve error messages for some jsonpath items

  10. Avoid race in RelationBuildDesc() affecting CREATE INDEX CONCURRENTLY.

  11. Represent Lists as expansible arrays, not chains of cons-cells.

Attachments

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!

------
Regards,
Alexander Korotkov
Supabase