Re: type cache cleanup improvements

On Thu, Oct 17, 2024 at 12:41 PM Andrei Lepikhov <lepihov(at)gmail(dot)com> wrote:
> On 10/15/24 15:08, Alexander Korotkov wrote:
> > 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.

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