Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit788baa9

Browse files
committed
Fix crash in brininsertcleanup during logical replication.
Logical replication crashes if the subscriber's partitioned tablehas 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 stilllinked from ii_AmCache. brininsert would also fail in that state,so it's pretty hard to see any advantage to this coding. Fullyremove the BrinInsertState, instead, so that a new brininsertcall would create a new cache.2. A logical replication subscriber sometimes does ExecOpenIndicestwice on the same ResultRelInfo, followed by doing ExecCloseIndicestwice; the second call reaches the brininsertcleanup bug. Quiteaside from tickling unexpected cases in aminsertcleanup methods,this seems very wasteful, because the IndexInfos built in thefirst ExecOpenIndices call are just lost during the second call,and have to be rebuilt at possibly-nontrivial cost. We shouldestablish 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 doesExecOpenIndices (and expects that ExecCleanupTupleRouting willclose the indexes again). Using the ResultRelInfo made byExecFindPartition, it calls apply_handle_delete_internal orapply_handle_insert_internal, both of which think they need to doExecOpenIndices/ExecCloseIndices for themselves. They do in the mainnon-partitioned code paths, but not here. The simplest fix is to pulltheir ExecOpenIndices/ExecCloseIndices calls out and put them in thecall sites for the non-partitioned cases. (We could have refactoredapply_handle_update_internal similarly, but I did not do so todaybecause there's no bug there: the partitioned code path doesn'tcall it.)Also, remove the always-duplicative open/close calls withinapply_handle_tuple_routing itself.Since brininsertcleanup and indeed the whole aminsertcleanup mechanismare new in v17, there's no observable bug in older branches. A casecould be made for trying to avoid these duplicative open/close callsin the older branches, but for now it seems not worth the trouble andrisk of new bugs.Bug: #18815Reported-by: Sergey Belyashov <sergey.belyashov@gmail.com>Discussion:https://postgr.es/m/18815-2a0407cc7f40b327@postgresql.orgBackpatch-through: 17
1 parentf61769a commit788baa9

File tree

3 files changed

+32
-15
lines changed

3 files changed

+32
-15
lines changed

‎src/backend/access/brin/brin.c

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -505,16 +505,18 @@ brininsertcleanup(Relation index, IndexInfo *indexInfo)
505505
BrinInsertState*bistate= (BrinInsertState*)indexInfo->ii_AmCache;
506506

507507
/* bail out if cache not initialized */
508-
if (indexInfo->ii_AmCache==NULL)
508+
if (bistate==NULL)
509509
return;
510510

511+
/* do this first to avoid dangling pointer if we fail partway through */
512+
indexInfo->ii_AmCache=NULL;
513+
511514
/*
512515
* Clean up the revmap. Note that the brinDesc has already been cleaned up
513516
* as part of its own memory context.
514517
*/
515518
brinRevmapTerminate(bistate->bis_rmAccess);
516-
bistate->bis_rmAccess=NULL;
517-
bistate->bis_desc=NULL;
519+
pfree(bistate);
518520
}
519521

520522
/*

‎src/backend/replication/logical/worker.c

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2432,8 +2432,13 @@ apply_handle_insert(StringInfo s)
24322432
apply_handle_tuple_routing(edata,
24332433
remoteslot,NULL,CMD_INSERT);
24342434
else
2435-
apply_handle_insert_internal(edata,edata->targetRelInfo,
2436-
remoteslot);
2435+
{
2436+
ResultRelInfo*relinfo=edata->targetRelInfo;
2437+
2438+
ExecOpenIndices(relinfo, false);
2439+
apply_handle_insert_internal(edata,relinfo,remoteslot);
2440+
ExecCloseIndices(relinfo);
2441+
}
24372442

24382443
finish_edata(edata);
24392444

@@ -2460,15 +2465,14 @@ apply_handle_insert_internal(ApplyExecutionData *edata,
24602465
{
24612466
EState*estate=edata->estate;
24622467

2463-
/* We must open indexes here. */
2464-
ExecOpenIndices(relinfo, false);
2468+
/* Caller should have opened indexes already. */
2469+
Assert(relinfo->ri_IndexRelationDescs!=NULL||
2470+
!relinfo->ri_RelationDesc->rd_rel->relhasindex||
2471+
RelationGetIndexList(relinfo->ri_RelationDesc)==NIL);
24652472

24662473
/* Do the insert. */
24672474
TargetPrivilegesCheck(relinfo->ri_RelationDesc,ACL_INSERT);
24682475
ExecSimpleRelationInsert(relinfo,estate,remoteslot);
2469-
2470-
/* Cleanup. */
2471-
ExecCloseIndices(relinfo);
24722476
}
24732477

24742478
/*
@@ -2767,8 +2771,14 @@ apply_handle_delete(StringInfo s)
27672771
apply_handle_tuple_routing(edata,
27682772
remoteslot,NULL,CMD_DELETE);
27692773
else
2770-
apply_handle_delete_internal(edata,edata->targetRelInfo,
2774+
{
2775+
ResultRelInfo*relinfo=edata->targetRelInfo;
2776+
2777+
ExecOpenIndices(relinfo, false);
2778+
apply_handle_delete_internal(edata,relinfo,
27712779
remoteslot,rel->localindexoid);
2780+
ExecCloseIndices(relinfo);
2781+
}
27722782

27732783
finish_edata(edata);
27742784

@@ -2802,7 +2812,11 @@ apply_handle_delete_internal(ApplyExecutionData *edata,
28022812
boolfound;
28032813

28042814
EvalPlanQualInit(&epqstate,estate,NULL,NIL,-1,NIL);
2805-
ExecOpenIndices(relinfo, false);
2815+
2816+
/* Caller should have opened indexes already. */
2817+
Assert(relinfo->ri_IndexRelationDescs!=NULL||
2818+
!localrel->rd_rel->relhasindex||
2819+
RelationGetIndexList(localrel)==NIL);
28062820

28072821
found=FindReplTupleInLocalRel(edata,localrel,remoterel,localindexoid,
28082822
remoteslot,&localslot);
@@ -2831,7 +2845,6 @@ apply_handle_delete_internal(ApplyExecutionData *edata,
28312845
}
28322846

28332847
/* Cleanup. */
2834-
ExecCloseIndices(relinfo);
28352848
EvalPlanQualEnd(&epqstate);
28362849
}
28372850

@@ -3042,14 +3055,12 @@ apply_handle_tuple_routing(ApplyExecutionData *edata,
30423055
EPQStateepqstate;
30433056

30443057
EvalPlanQualInit(&epqstate,estate,NULL,NIL,-1,NIL);
3045-
ExecOpenIndices(partrelinfo, false);
30463058

30473059
EvalPlanQualSetSlot(&epqstate,remoteslot_part);
30483060
TargetPrivilegesCheck(partrelinfo->ri_RelationDesc,
30493061
ACL_UPDATE);
30503062
ExecSimpleRelationUpdate(partrelinfo,estate,&epqstate,
30513063
localslot,remoteslot_part);
3052-
ExecCloseIndices(partrelinfo);
30533064
EvalPlanQualEnd(&epqstate);
30543065
}
30553066
else

‎src/test/subscription/t/013_partition.pl

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,10 @@
4949
$node_subscriber1->safe_psql('postgres',
5050
"CREATE TABLE tab1 (c text, a int PRIMARY KEY, b text) PARTITION BY LIST (a)"
5151
);
52+
# make a BRIN index to test aminsertcleanup logic in subscriber
53+
$node_subscriber1->safe_psql('postgres',
54+
"CREATE INDEX tab1_c_brin_idx ON tab1 USING brin (c)"
55+
);
5256
$node_subscriber1->safe_psql('postgres',
5357
"CREATE TABLE tab1_1 (b text, c text DEFAULT 'sub1_tab1', a int NOT NULL)"
5458
);

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp