Subject: Re: type cache cleanup improvements

From: Artur Zakirov <[email-masked]>
Date: Thu, 10 Oct 2024 17:54:01 +0200

Hi all,

On Fri, 13 Sept 2024 at 01:38, Alexander Korotkov <[email-masked]> wrote:
>
> 0001 - adds comment about concurrent invalidation handling
> 0002 - revised c14d4acb8.  Now we track type oids, whose
> TypeCacheEntry's filing is in-progress.  Add entry to
> RelIdToTypeIdCacheHash at the end of lookup_type_cache() or on the
> transaction abort.  During invalidation don't assert
> RelIdToTypeIdCacheHash to be here if TypeCacheEntry is in-progress.

Thank you Alexander for the patch. I reviewed and tested it.

I used Teodor's script to check the performance. On my laptop on
master ROLLBACK runs 11496.219 ms. With patch ROLLBACK runs 378.990
ms.

It seems to me that there are couple of possible issues in the patch:

In `lookup_type_cache()` `in_progress_list` is allocated using
`CacheMemoryContext`, on the other hand it seems there might be a case
when `CacheMemoryContext` is not created yet. It is created below in
the code if it doesn't exist:

    /* Also make sure CacheMemoryContext exists */
    if (!CacheMemoryContext)
        CreateCacheMemoryContext();

It is probably a very rare case, but it might be better to allocate
`in_progress_list` after that line, or move creation of
`CacheMemoryContext` higher.

Within `insert_rel_type_cache_if_needed()` and
`delete_rel_type_cache_if_needed()` there is an if condition:

    if ((typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) ||
        (typentry->flags & TCFLAGS_OPERATOR_FLAGS) ||
        typentry->tupDesc != NULL)

Based on the logic of the rest of the code does it make sense to use
TCFLAGS_DOMAIN_BASE_IS_COMPOSITE instead of TCFLAGS_OPERATOR_FLAGS?
Otherwise the condition doesn't look logical.

-- 
Kind regards,
Artur

---

From: Alexander Korotkov <[email-masked]>
Date: Sun, 13 Oct 2024 15:08:38 +0300

Hi, Arthur!

Thank you so much for your review!

> `CacheMemoryContext` higher.

Yes, it makes sense to initialize `in_progress_list ` when
`CacheMemoryContext` is guaranteed to be initialized.  Fixed in the
attached patch.

> Otherwise the condition doesn't look logical.

I'm not sure I get the point.  This check ensures that type entry has
something to be cleared.  In this case we need to keep
RelIdToTypeIdCacheHash entry to find this item on invalidation
message.  I'm not sure how TCFLAGS_DOMAIN_BASE_IS_COMPOSITE is
relevant here, because it's valid only for TYPTYPE_DOMAIN while this
patch deals with TYPTYPE_COMPOSITE.

------
Regards,
Alexander Korotkov
Supabase

---

From: jian he <[email-masked]>
Date: Tue, 15 Oct 2024 15:34:41 +0800

On Sun, Oct 13, 2024 at 8:09 PM Alexander Korotkov <[email-masked]> wrote:

hi. Alexander.
I don't fully understand all of it. but I did some tests anyway.

static void
cleanup_in_progress_typentries(void)
{
    int            i;
    if (in_progress_list_len > 1)
        elog(INFO, "%s:%d in_progress_list_len > 1", __FILE_NAME__, __LINE__);
    for (i = 0; i < in_progress_list_len; i++)
    {
        TypeCacheEntry *typentry;
        typentry = (TypeCacheEntry *) hash_search(TypeCacheHash,
                                                  &in_progress_list[i],
                                                  HASH_FIND, NULL);
        insert_rel_type_cache_if_needed(typentry);
    }
    in_progress_list_len = 0;
}

the regress still passed.
I assume "elog(INFO, " won't interfere in cleanup_in_progress_typentries.
So we lack tests for larger in_progress_list_len values or i missed something?

    /* Call check_delete_rel_type_cache() if we actually cleared something */
    if (hadTupDescOrOpclass)
        delete_rel_type_cache_if_needed(typentry);

        /*
         * Call check_delete_rel_type_cache() if we cleaned
         * TCFLAGS_HAVE_PG_TYPE_DATA flag previously.
         */
        if (hadPgTypeData)
            delete_rel_type_cache_if_needed(typentry);

check_delete_rel_type_cache don't exist, so these comments are wrong?

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 15 Oct 2024 11:08:54 +0300

Hi, Jian!

Thank you for your review.

> So we lack tests for larger in_progress_list_len values or i missed something?

Try to run test suite with -DCLOBBER_CACHE_ALWAYS.

> check_delete_rel_type_cache don't exist, so these comments are wrong?

Yep, they didn't get updated.  Fixed in the attached patchset.

------
Regards,
Alexander Korotkov
Supabase

---

From: Artur Zakirov <[email-masked]>
Date: Tue, 15 Oct 2024 10:45:23 +0200

> Yep, they didn't get updated.  Fixed in the attached patchset.

Thank you Alexander for the fixes. The last version of the patch looks
good to me.

> patch deals with TYPTYPE_COMPOSITE.

Regarding this discussion earlier, I assumed that TYPTYPE_DOMAIN also
needs to be handled by `insert_rel_type_cache_if_needed()`. And it
seems that handling of TYPTYPE_DOMAIN will remain the same as before.

-- 
Kind regards,
Artur

---

From: jian he <[email-masked]>
Date: Tue, 15 Oct 2024 17:50:15 +0800

> Try to run test suite with -DCLOBBER_CACHE_ALWAYS.

build from source, DCLOBBER_CACHE_ALWAYS takes a very long time.
So I gave up.

in lookup_type_cache, we unconditionally do
in_progress_list_len++;
in_progress_list_len--;

"static int    in_progress_list_len;"
means in_progress_list_len value change is confined in
src/backend/utils/cache/typcache.c.

based on above information, i am still  confused with
cleanup_in_progress_typentries, in_progress_list_len
is there any simple sql example to demo
cleanup_in_progress_typentries,  in_progress_list_len> 0.

---

From: Artur Zakirov <[email-masked]>
Date: Tue, 15 Oct 2024 15:16:07 +0200

> cleanup_in_progress_typentries,  in_progress_list_len> 0.

AFAIK to reproduce cases when `in_progress_list_len > 0`
`lookup_type_cache()` should fail during its execution.

To do so you can call `lookup_type_cache()` with non-existing type_id
from a C function.

-- 
Kind regards,
Artur

---

From: Andrei Lepikhov <[email-masked]>
Date: Thu, 17 Oct 2024 16:41:15 +0700

> Yep, they didn't get updated.  Fixed in the attached patchset.
Let me wear Alexander Lakhin's mask for a moment and say that the code 
may cause a segfault:

#0  0x000055e0da186000 in insert_rel_type_cache_if_needed (typentry=0x0) 
at typcache.c:3066
b3066		if (typentry->typtype != TYPTYPE_COMPOSITE)
(gdb) bt 20
#0  0x000055e0da186000 in insert_rel_type_cache_if_needed (typentry=0x0) 
at typcache.c:3066
#1  0x000055e0da18844f in cleanup_in_progress_typentries () at 
typcache.c:3172
#2  0x000055e0da1883f9 in AtEOXact_TypeCache () at typcache.c:3181
#3  0x000055e0d9a22e59 in AbortTransaction () at xact.c:2961
#4  0x000055e0d9a1f75c in AbortCurrentTransactionInternal () at xact.c:3491
#5  0x000055e0d9a1f6be in AbortCurrentTransaction () at xact.c:3445
#6  0x000055e0d9f55f28 in PostgresMain (dbname=0x55e1057fb838 
"regression", username=0x55e1057fb818 "danolivo") at postgres.c:4508
#7  0x000055e0d9f4e4a3 in BackendMain (startup_data=0x7ffeaf051310 "", 
startup_data_len=4) at backend_startup.c:107
#8  0x000055e0d9e4bfee in postmaster_child_launch (child_type=B_BACKEND, 
startup_data=0x7ffeaf051310 "", startup_data_len=4, 
client_sock=0x7ffeaf051358) at launch_backend.c:274
#9  0x000055e0d9e522a3 in BackendStartup (client_sock=0x7ffeaf051358) at 
postmaster.c:3420
#10 0x000055e0d9e502f9 in ServerLoop () at postmaster.c:1653
#11 0x000055e0d9e4f4fe in PostmasterMain (argc=3, argv=0x55e1057bb520) 
at postmaster.c:1351
#12 0x000055e0d9cf4b2d in main (argc=3, argv=0x55e1057bb520) at main.c:197

It can happen if something triggers an error in the middle of 
lookup_type_cache when in_progress_list[i] is already filled, but the 
typentry wasn't created.
I think it can be easily shielded (see attached). Also, the name 
cleanup_in_progress_typentries causes a lot of ponderings, I guess it 
would be better to rename it likewise  finalize_in_progress_typentries.
Also, I added trivial comments to better understand what the function does.

I think the first patch may already be committed, and this little burden 
may be avoided in future versions.

-- 
regards, Andrei Lepikhov
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index f54e7d531a..45aed74019 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -3058,7 +3058,7 @@ static void
 insert_rel_type_cache_if_needed(TypeCacheEntry *typentry)
 {
 	/* Immediately quit for non-composite types */
-	if (typentry->typtype != TYPTYPE_COMPOSITE)
+	if (!typentry || typentry->typtype != TYPTYPE_COMPOSITE)
 		return;
 
 	/* typrelid should be given for composite types */
@@ -3147,8 +3147,13 @@ delete_rel_type_cache_if_needed(TypeCacheEntry *typentry)
 	}
 }
 
+/*
+ * Add into the accessory hash table entries, added into TypCache and not added
+ * into the RelIdToTypeId matching hash table.
+ * It may happen in case of an error raised during the lookup_type_cache call.
+ */
 static void
-cleanup_in_progress_typentries(void)
+finalize_in_progress_typentries(void)
 {
 	int			i;
 
@@ -3168,11 +3173,11 @@ cleanup_in_progress_typentries(void)
 void
 AtEOXact_TypeCache(void)
 {
-	cleanup_in_progress_typentries();
+	finalize_in_progress_typentries();
 }
 
 void
 AtEOSubXact_TypeCache(void)
 {
-	cleanup_in_progress_typentries();
+	finalize_in_progress_typentries();
 }

---

From: Alexander Korotkov <[email-masked]>
Date: Sun, 20 Oct 2024 20:36:13 +0300

> may be avoided in future versions.

Thank you!
I've integrated your patch.  But I think
finalize_in_progress_typentries() is more appropriate place to check
typentry for NULL.  Also, I've revised the
finalize_in_progress_typentries() header comment.  I'm going to push
these patches if no objections.

------
Regards,
Alexander Korotkov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Sun, 20 Oct 2024 20:47:28 +0300

> in lookup_type_cache, we unconditionally do

Yes, this should work OK when no errors.  On error or interruption,
finalize_in_progress_typentries() will clean the things up.

> src/backend/utils/cache/typcache.c.

Yep.

> cleanup_in_progress_typentries,  in_progress_list_len> 0.

I don't think there is simple sql to reliably reproduce that.  In
order to hit that, we must process invalidation messages in some
(short) moment of time during lookup_type_cache().  You can reproduce
that by setting a breakpoint in lookup_type_cache() and in parallel do
something to invalidate the type cache entry (for instance, ALTER
TABLE ... ADD COLUMN ... would invalidate the composite type).  In
principle, we can reproduce that using injection points.  However, I'm
not intended to do that as long as we have buildfarm members with
-DCLOBBER_CACHE_ALWAYS.  FWIW, I will for sure run tests with
-DCLOBBER_CACHE_ALWAYS before committing this.

------
Regards,
Alexander Korotkov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Sun, 20 Oct 2024 21:00:18 +0300

On Sun, Oct 20, 2024 at 8:47 PM Alexander Korotkov <[email-masked]> wrote:
> TABLE ... ADD COLUMN ... would invalidate the composite type).

Oops, concurrent invalidation message is not enough here.  So,
-DCLOBBER_CACHE_ALWAYS is also not enough to reproduce the situation.
Injection-point test is required.  I'm going to add this.

------
Regards,
Alexander Korotkov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Mon, 21 Oct 2024 01:09:23 +0300

> Injection-point test is required.  I'm going to add this.

Here you go.  The test with injection point is implemented.

------
Regards,
Alexander Korotkov
Supabase

---

From: Dagfinn Ilmari Mannsåker <[email-masked]>
Date: Mon, 21 Oct 2024 00:32:08 +0100

Alexander Korotkov <[email-masked]> writes:

> +static Oid *in_progress_list;
> +static int	in_progress_list_len;
> +static int	in_progress_list_maxlen;

Is there any particular reason not to use pg_list.h for this?

- ilmari

---

From: Andrei Lepikhov <[email-masked]>
Date: Mon, 21 Oct 2024 09:36:04 +0400

> these patches if no objections.
I agree with your idea. Also, I think it would be more conventional not 
to check the type entry for a NULL value but to test the 'found' value 
instead.
And thanks for the injection points tests! I must have slipped my mind 
about this option.
Now, the patch set looks good for me.

-- 
regards, Andrei Lepikhov

---

From: Andrei Lepikhov <[email-masked]>
Date: Mon, 21 Oct 2024 09:40:14 +0400

> Is there any particular reason not to use pg_list.h for this?
Sure. The type cache lookup has to be as much optimal as possible.
Using an array and relating sequential access to it, we avoid memory 
allocations and deallocations 99.9% of the time. Also, quick access to 
the single element (which we will have in real life almost all of the 
time) is much faster than employing list machinery.

-- 
regards, Andrei Lepikhov

---

From: jian he <[email-masked]>
Date: Mon, 21 Oct 2024 15:51:40 +0800

thanks for the
INJECTION_POINT("typecache-before-rel-type-cache-insert");
Now I have better understanding of the whole changes.

+/*
+ * Add possibly missing RelIdToTypeId entries related to TypeCacheHas
+ * entries, marked as in-progress by lookup_type_cache().  It may happen
+ * in case of an error or interruption during the lookup_type_cache() call.
+ */
+static void
+finalize_in_progress_typentries(void)
comment typo. "TypeCacheHas" should be "TypeCacheHash"

---

From: Alexander Korotkov <[email-masked]>
Date: Mon, 21 Oct 2024 11:10:46 +0300

> time) is much faster than employing list machinery.

+1,
List with zero elements has to be NIL.  That means continuous
allocations/deallocations.

------
Regards,
Alexander Korotkov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Mon, 21 Oct 2024 11:11:47 +0300

> comment typo. "TypeCacheHas" should be "TypeCacheHash"

Thank you.  This also has been spotted by Alexander Lakhin (off-list).
Fixed in the attached revision of the patchset.

------
Regards,
Alexander Korotkov
Supabase

---

From: Dagfinn Ilmari Mannsåker <[email-masked]>
Date: Mon, 21 Oct 2024 11:16:25 +0100

>> time) is much faster than employing list machinery.

Lists are actually dynamically resized arrays these days (see commit
1cff1b95ab6ddae32faa3efe0d95a820dbfdc164), not linked lists, so
accessing arbitrary elements is O(1), not O(n). Just like this patch,
the size is doubled (starting at 16) whenever array is full.

> allocations/deallocations.

This however is a valid point (unless we keep a dummy zeroth element to
avoid it, which is even uglier than open-coding the array extension
logic), so objection withdrawn.

- ilmari

---

From: Alexander Korotkov <[email-masked]>
Date: Mon, 21 Oct 2024 14:30:41 +0300

> logic), so objection withdrawn.

OK, thank you!

The attached revision fixes EXTRA_INSTALL in
src/test/modules/typcache/Makefile.  Spotted off-list by Arthur
Zakirov.

------
Regards,
Alexander Korotkov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 22 Oct 2024 10:34:15 +0300

> src/test/modules/typcache/Makefile.  Spotted off-list by Arthur

I've re-checked that regression tests pass with
-DCLOBBER_CACHE_ALWAYS.  Also did some grammar corrections for
comments and commit message.  I'm going to push this if no objections.

------
Regards,
Alexander Korotkov
Supabase

---

From: Pavel Borisov <[email-masked]>
Date: Tue, 22 Oct 2024 19:09:59 +0400

Hi, Alexander!

> comments and commit message.  I'm going to push this if no objections.
Thank you for working on this patch!
Looked through the patchset once more.

Patch 0001 (minor): "in the last" -> "after everything else" or "after
other TypeCacheEntry contents"

Patch 0002 looks ready to me.

Regards,
Pavel Borisov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 22 Oct 2024 20:33:24 +0300

On Tue, Oct 22, 2024 at 6:10 PM Pavel Borisov <[email-masked]> wrote:
> On Tue, 22 Oct 2024 at 11:34, Alexander Korotkov <[email-masked]> wrote:
>>
> Patch 0001 (minor): "in the last" -> "after everything else" or "after other TypeCacheEntry contents"
>

Thank you, Pavel!  0001 revised according to your suggestion.

------
Regards,
Alexander Korotkov
Supabase

---

From: Andres Freund <[email-masked]>
Date: Fri, 25 Oct 2024 04:35:29 -0400

Hi,

> Thank you, Pavel!  0001 revised according to your suggestion.

Starting with this commit CI fails.

https://cirrus-ci.com/task/6668851469877248
https://api.cirrus-ci.com/v1/artifact/task/6668851469877248/testrun/build/testrun/regress-running/regress/regression.diffs

diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out	2024-10-24 11:38:43.829712000 +0000
+++ /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out	2024-10-24 11:44:57.154238000 +0000
@@ -1338,14 +1338,9 @@
 ERROR:  cannot drop inherited constraint "f1_pos" of relation "p1_c1"
 alter table p1 drop constraint f1_pos;
 \d p1_c1
-               Table "public.p1_c1"
- Column |  Type   | Collation | Nullable | Default
---------+---------+-----------+----------+---------
- f1     | integer |           |          |
-Check constraints:
-    "f1_pos" CHECK (f1 > 0)
-Inherits: p1
-
+ERROR:  error triggered for injection point typecache-before-rel-type-cache-insert
+LINE 4: ORDER BY 1;
+                 ^
 drop table p1 cascade;
 NOTICE:  drop cascades to table p1_c1
 create table p1(f1 int constraint f1_pos CHECK (f1 > 0));

Greetings,

Andres

---

From: Alexander Korotkov <[email-masked]>
Date: Fri, 25 Oct 2024 12:48:49 +0300

> +++ /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out      2024-10-24 11:44:57.154238000 +0000

Thank you for reporting this.
Looks weird that injection point, which isn't used in these tests, got
triggered here.
I'm looking into this.

------
Regards,
Alexander Korotkov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Fri, 25 Oct 2024 13:13:29 +0300

> Looks weird that injection point, which isn't used in these tests, got

Oh, I forgot to make injection points in typcache_rel_type_cache.sql
local.  Thus, it affects concurrent tests.  Must be fixed in
aa1e898dea.

------
Regards,
Alexander Korotkov
Supabase

---

From: Noah Misch <[email-masked]>
Date: Fri, 11 Apr 2025 13:32:41 -0700

On Tue, Oct 22, 2024 at 08:33:24PM +0300, Alexander Korotkov wrote:
> >> I'm going to push this if no objections.

(This became commit b85a9d0.)

> +	/* Call delete_rel_type_cache() if we actually cleared something */
> +	if (hadTupDescOrOpclass)
> +		delete_rel_type_cache_if_needed(typentry);

I think the intent was to maintain the invariant that a RelIdToTypeIdCacheHash
entry exists if and only if certain kinds of data appear in the TypeCacheHash
entry.  However, TypeCacheOpcCallback() clears TCFLAGS_OPERATOR_FLAGS without
maintaining RelIdToTypeIdCacheHash.  Is it right to do that?

---

From: Alexander Korotkov <[email-masked]>
Date: Sat, 12 Apr 2025 00:43:19 +0300

> maintaining RelIdToTypeIdCacheHash.  Is it right to do that?

Thank you for the question.  I'll recheck this in next couple of days.

------
Regards,
Alexander Korotkov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Mon, 21 Apr 2025 04:54:08 +0300

Hi, Noah!

> Thank you for the question.  I'll recheck this in next couple of days.

Sorry for the delay.  Generally, your finding is correct.  But, I
didn't manage to reproduce the situation, where existing code leads to
real error.  In order to have it, we must have typcache entry without
TCFLAGS_HAVE_PG_TYPE_DATA and tupDesc, but with some of
TCFLAGS_OPERATOR_FLAGS.  Reseting TCFLAGS_HAVE_PG_TYPE_DATA for a
composite type doesn't seem to be possible without resetting the rest
at the same time.

Nevertheless, I think it would be fragile to leave the current code
"as is".  If even there is no case of real error (or it's just me
didn't manage to find it), it could appear after further changes of
type cache code.  So, the fix is attached.

------
Regards,
Alexander Korotkov
Supabase

---

From: Noah Misch <[email-masked]>
Date: Mon, 28 Apr 2025 17:56:12 -0700

On Mon, Apr 21, 2025 at 04:54:08AM +0300, Alexander Korotkov wrote:
> On Sat, Apr 12, 2025 at 12:43 AM Alexander Korotkov <[email-masked]> wrote:

> TCFLAGS_OPERATOR_FLAGS.

That makes sense.

> Reseting TCFLAGS_HAVE_PG_TYPE_DATA for a

This change looks appropriate.  Thanks.

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 29 Apr 2025 13:00:05 +0300

Hi, Noah!

> This change looks appropriate.  Thanks.

Thank you for your feedback!

------
Regards,
Alexander Korotkov
Supabase