Fix crash in brininsertcleanup during logical replication.

Logical replication crashes if the subscriber's partitioned table
has a BRIN index.  There are two independently blamable causes,
and this patch fixes both:

1. brininsertcleanup fails if called twice for the same IndexInfo,
because it half-destroys its BrinInsertState but leaves it still
linked from ii_AmCache.  brininsert would also fail in that state,
so it's pretty hard to see any advantage to this coding.  Fully
remove the BrinInsertState, instead, so that a new brininsert
call would create a new cache.

2. A logical replication subscriber sometimes does ExecOpenIndices
twice on the same ResultRelInfo, followed by doing ExecCloseIndices
twice; the second call reaches the brininsertcleanup bug.  Quite
aside from tickling unexpected cases in aminsertcleanup methods,
this seems very wasteful, because the IndexInfos built in the
first ExecOpenIndices call are just lost during the second call,
and have to be rebuilt at possibly-nontrivial cost.  We should
establish a coding rule that you don't do that.

The problematic coding is that when the target table is partitioned,
apply_handle_tuple_routing calls ExecFindPartition which does
ExecOpenIndices (and expects that ExecCleanupTupleRouting will
close the indexes again).  Using the ResultRelInfo made by
ExecFindPartition, it calls apply_handle_delete_internal or
apply_handle_insert_internal, both of which think they need to do
ExecOpenIndices/ExecCloseIndices for themselves.  They do in the main
non-partitioned code paths, but not here.  The simplest fix is to pull
their ExecOpenIndices/ExecCloseIndices calls out and put them in the
call sites for the non-partitioned cases.  (We could have refactored
apply_handle_update_internal similarly, but I did not do so today
because there's no bug there: the partitioned code path doesn't
call it.)

Also, remove the always-duplicative open/close calls within
apply_handle_tuple_routing itself.

Since brininsertcleanup and indeed the whole aminsertcleanup mechanism
are new in v17, there's no observable bug in older branches.  A case
could be made for trying to avoid these duplicative open/close calls
in the older branches, but for now it seems not worth the trouble and
risk of new bugs.

Bug: #18815
Reported-by: Sergey Belyashov <sergey.belyashov@gmail.com>
Discussion: https://postgr.es/m/18815-2a0407cc7f40b327@postgresql.org
Backpatch-through: 17

diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 92cb72ebba6ed3a9fbfae287f46032d816025254..6cbd31f0a3d8b6dbc69d65a3168436894f573b65 100644 (file)
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -505,16 +505,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);
 }
 
 /*

diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index d091a1dd27cdc1994d2b7ccddcee4c6706e75f98..65e22306c4863c804d4d1a484195aa1770dd7935 100644 (file)
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2432,8 +2432,13 @@ apply_handle_insert(StringInfo s)
        apply_handle_tuple_routing(edata,
                                   remoteslot, NULL, CMD_INSERT);
    else
-       apply_handle_insert_internal(edata, edata->targetRelInfo,
-                                    remoteslot);
+   {
+       ResultRelInfo *relinfo = edata->targetRelInfo;
+
+       ExecOpenIndices(relinfo, false);
+       apply_handle_insert_internal(edata, relinfo, remoteslot);
+       ExecCloseIndices(relinfo);
+   }
 
    finish_edata(edata);
 
@@ -2460,15 +2465,14 @@ apply_handle_insert_internal(ApplyExecutionData *edata,
 {
    EState     *estate = edata->estate;
 
-   /* We must open indexes here. */
-   ExecOpenIndices(relinfo, false);
+   /* Caller should have opened indexes already. */
+   Assert(relinfo->ri_IndexRelationDescs != NULL ||
+          !relinfo->ri_RelationDesc->rd_rel->relhasindex ||
+          RelationGetIndexList(relinfo->ri_RelationDesc) == NIL);
 
    /* Do the insert. */
    TargetPrivilegesCheck(relinfo->ri_RelationDesc, ACL_INSERT);
    ExecSimpleRelationInsert(relinfo, estate, remoteslot);
-
-   /* Cleanup. */
-   ExecCloseIndices(relinfo);
 }
 
 /*
@@ -2767,8 +2771,14 @@ apply_handle_delete(StringInfo s)
        apply_handle_tuple_routing(edata,
                                   remoteslot, NULL, CMD_DELETE);
    else
-       apply_handle_delete_internal(edata, edata->targetRelInfo,
+   {
+       ResultRelInfo *relinfo = edata->targetRelInfo;
+
+       ExecOpenIndices(relinfo, false);
+       apply_handle_delete_internal(edata, relinfo,
                                     remoteslot, rel->localindexoid);
+       ExecCloseIndices(relinfo);
+   }
 
    finish_edata(edata);
 
@@ -2802,7 +2812,11 @@ apply_handle_delete_internal(ApplyExecutionData *edata,
    bool        found;
 
    EvalPlanQualInit(&epqstate, estate, NULL, NIL, -1, NIL);
-   ExecOpenIndices(relinfo, false);
+
+   /* Caller should have opened indexes already. */
+   Assert(relinfo->ri_IndexRelationDescs != NULL ||
+          !localrel->rd_rel->relhasindex ||
+          RelationGetIndexList(localrel) == NIL);
 
    found = FindReplTupleInLocalRel(edata, localrel, remoterel, localindexoid,
                                    remoteslot, &localslot);
@@ -2831,7 +2845,6 @@ apply_handle_delete_internal(ApplyExecutionData *edata,
    }
 
    /* Cleanup. */
-   ExecCloseIndices(relinfo);
    EvalPlanQualEnd(&epqstate);
 }
 
@@ -3042,14 +3055,12 @@ apply_handle_tuple_routing(ApplyExecutionData *edata,
                    EPQState    epqstate;
 
                    EvalPlanQualInit(&epqstate, estate, NULL, NIL, -1, NIL);
-                   ExecOpenIndices(partrelinfo, false);
 
                    EvalPlanQualSetSlot(&epqstate, remoteslot_part);
                    TargetPrivilegesCheck(partrelinfo->ri_RelationDesc,
                                          ACL_UPDATE);
                    ExecSimpleRelationUpdate(partrelinfo, estate, &epqstate,
                                             localslot, remoteslot_part);
-                   ExecCloseIndices(partrelinfo);
                    EvalPlanQualEnd(&epqstate);
                }
                else

diff --git a/src/test/subscription/t/013_partition.pl b/src/test/subscription/t/013_partition.pl
index 29580525a97d8f86f29d702ee0cada0426fe973a..db7a1604643b769dfa76a8c8f271ab7dbfcb534c 100644 (file)
--- a/src/test/subscription/t/013_partition.pl
+++ b/src/test/subscription/t/013_partition.pl
@@ -49,6 +49,10 @@ $node_publisher->safe_psql('postgres',
 $node_subscriber1->safe_psql('postgres',
    "CREATE TABLE tab1 (c text, a int PRIMARY KEY, b text) PARTITION BY LIST (a)"
 );
+# make a BRIN index to test aminsertcleanup logic in subscriber
+$node_subscriber1->safe_psql('postgres',
+   "CREATE INDEX tab1_c_brin_idx ON tab1 USING brin (c)"
+);
 $node_subscriber1->safe_psql('postgres',
    "CREATE TABLE tab1_1 (b text, c text DEFAULT 'sub1_tab1', a int NOT NULL)"
 );