Re: BUG #18815: Logical replication worker Segmentation fault

Sergey Belyashov <sergey.belyashov@gmail.com> writes:
> I think backtrace will help.
> Core was generated by `postgres: 17/main: logical replication apply
> worker for subscription 602051860'.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  0x00005635402c869c in brinRevmapTerminate (revmap=0x0)
>     at ./build/../src/backend/access/brin/brin_revmap.c:102
> (gdb) backtrace
> #0  0x00005635402c869c in brinRevmapTerminate (revmap=0x0)
>     at ./build/../src/backend/access/brin/brin_revmap.c:102
> #1  0x00005635402bfddd in brininsertcleanup (index=<optimized out>,
> indexInfo=<optimized out>)
>     at ./build/../src/backend/access/brin/brin.c:515
> #2  0x0000563540479309 in ExecCloseIndices
> (resultRelInfo=resultRelInfo@entry=0x563541cab8d0)
>     at ./build/../src/backend/executor/execIndexing.c:248

Thanks!  It seems clear from that that the fault is basically in
brininsertcleanup(), which is trashing the BrinInsertState but
leaving indexInfo->ii_AmCache still pointing at it, so that
the next brininsert() will think it has a valid cache entry.
I suspect that the attached will fix it.  What I don't understand
is why it's apparently so hard to trigger the crash, because it
looks to me like any two successive insert commands on the same
BRIN index should hit this.

BTW, I'm also a bit suspicious of the comment's claim that the
brinDesc doesn't need cleanup.  That looks like a potential
memory leak.

            regards, tom lane

diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 4265687afa..60320440fc 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -511,16 +511,18 @@ brininsertcleanup(Relation index, IndexInfo *indexInfo)
     BrinInsertState *bistate = (BrinInsertState *) indexInfo->ii_AmCache;
 
     /* bail out if cache not initialized */
-    if (indexInfo->ii_AmCache == NULL)
+    if (bistate == NULL)
         return;
 
+    /* do this first to avoid dangling pointer if we fail partway through */
+    indexInfo->ii_AmCache = NULL;
+
     /*
      * Clean up the revmap. Note that the brinDesc has already been cleaned up
      * as part of its own memory context.
      */
     brinRevmapTerminate(bistate->bis_rmAccess);
-    bistate->bis_rmAccess = NULL;
-    bistate->bis_desc = NULL;
+    pfree(bistate);
 }
 
 /*