Re: type cache cleanup improvements

Hi, Alexander!

On Tue, 20 Aug 2024 at 23:01, Alexander Korotkov <aekorotkov(at)gmail(dot)com>
wrote:

> On Mon, Aug 5, 2024 at 4:16 AM Alexander Korotkov <aekorotkov(at)gmail(dot)com>
> wrote:
> > I've revised the patchset. First of all, I've re-ordered the patches.
> >
> > 0001-0002 (former 0002-0003)
> > Comprises hash_search_with_hash_value() function and its application
> > to avoid full hash iteration in InvalidateAttoptCacheCallback() and
> > TypeCacheTypCallback(). I think this is quite straightforward
> > optimization without negative side effects. I've revised comments,
> > commit message and did some code beautification. I'm going to push
> > this if no objections.
> >
> > 0003 (former 0001)
> > I've revised this patch. I think main concerns expressed in the
> > thread about this path is that we don't have invalidation mechanism
> > for relid => typid map. Finally due to oid wraparound same relids
> > could get reused. That could lead to invalid entries in the map about
> > existing relids and typeids. This is rather messy, but I don't think
> > this could cause a material bug. The maps items are used only for
> > cache invalidation. Extra invalidation doesn't cause a bug. If type
> > with same relid will be cached, then correspoding map item will be
> > overridden, so no missing invalidation. However, I see the following
> > reasons for keeping consistent state of relid => typid map.
> >
> > 1) As the main use-case for this optimization is flood of temporary
> > tables, it would be nice not let relid => typid map bloat in this
> > case. I see that TypeCacheHash would get bloated, because its entries
> > are never deleted. However, I would prefer to not get this situation
> > even worse.
> > 2) In future we may find some more use-cases for relid => typid map
> > besides cache invalidation. Keeping that in consistent state could be
> > advantage then.
> >
> > In the attached patch, I'm keeping relid => typid map when
> > corresponding typentry have either TCFLAGS_HAVE_PG_TYPE_DATA, or
> > TCFLAGS_OPERATOR_FLAGS, or tupdesc. Thus, when temporary table gets
> > deleted, we would invalidate the map item.
> >
> > It will be also nice to get rid of iteration over all the cached
> > domain types in TypeCacheRelCallback(). However, this typically
> > shouldn't be a problem since domain types are less tended to bloat.
> > Domain types are created manually, unlike composite types which are
> > automatically created for every temporary table. We will probably
> > need to optimize this in future, but I don't feel this to be necessary
> > in present patch.
> >
> > I think the revised 0003 requires review.
>
> The rebased remaining patch is attached.
>
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.

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
+}

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,
4. I haven't looked into comments, though I'd recommend oid -> OID
replacement in the comments.

Thank you for working on this patchset!

Regards,
Pavel Borisov
Supabase