Hi, Pavel!
On Wed, Aug 21, 2024 at 4:28 PM Pavel Borisov <pashkin.e...@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
v9-0001-Avoid-looping-over-all-type-cache-entries-in-Type.patch
Description: Binary data
