Hi, Alexander!
On Wed, 21 Aug 2024 at 19:29, Alexander Korotkov <aekorot...@gmail.com>
wrote:
> 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!
>
Looked at v9:
Patch looks good to me. I'd only suggest comments changes:
"The map from relation's OID to the corresponding composite type OID" ->
"The mapping of relation's OID to the corresponding composite type OID"
"We're keeping the map entry when corresponding typentry have either
TCFLAGS_HAVE_PG_TYPE_DATA, or TCFLAGS_OPERATOR_FLAGS, or tupdesc. That is
we're keeping map entry if the entry has something to clear." -> "We're
keeping the map entry when the corresponding typentry has something to
clear i.e it has either TCFLAGS_HAVE_PG_TYPE_DATA, or
TCFLAGS_OPERATOR_FLAGS, or tupdesc."
"Invalidate particular TypeCacheEntry on Relcache inval callback" - remove
extra tabs before. Maybe also add empty line above.
"Typically shouldn't be a problem" -> "Typically this shouldn't affect
performance"
"Relid = 0, so we need" -> "Relid is invalid. By convention we need"
"if cleaned TCFLAGS_HAVE_PG_TYPE_DATA flag" -> "if we cleaned
TCFLAGS_HAVE_PG_TYPE_DATA flag previously"
"+/*
+ * Delete entry RelIdToTypeIdCacheHash if needed after resetting of the
+ * TCFLAGS_HAVE_PG_TYPE_DATA flag, or any of TCFLAGS_OPERATOR_FLAGS flags,
+ * or tupDesc if needed." - remove one "if needed"
Regards,
Pavel Borisov
Supabase
