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

Commit9c703c1

Browse files
committed
Make queries' locking of indexes more consistent.
The assertions added by commitb04aeb0 exposed that there are somecode paths wherein the executor will try to open an index withoutholding any lock on it. We do have some lock on the index's table,so it seems likely that there's no fatal problem with this (forinstance, the index couldn't get dropped from under us). Still,it's bad practice and we should fix it.To do so, remove the optimizations in ExecInitIndexScan and friendsthat tried to avoid taking a lock on an index belonging to a targetrelation, and just take the lock always. In non-bug cases, thiswill result in no additional shared-memory access, since we'll findin the local lock table that we already have a lock of the desiredtype; hence, no significant performance degradation should occur.Also, adjust the planner and executor so that the type of lock takenon an index is always identical to the type of lock taken for its table,by relying on the recently added RangeTblEntry.rellockmode field.This avoids some corner cases where that might not have been truebefore (possibly resulting in extra locking overhead), and preventsfuture maintenance issues from having multiple bits of logic thatall needed to be in sync. In addition, this change removes all corecalls to ExecRelationIsTargetRelation, which avoids a possible O(N^2)startup penalty for queries with large numbers of target relations.(We'd probably remove that function altogether, were it not that weadvertise it as something that FDWs might want to use.)Also adjust some places in selfuncs.c to not take any lock on indexesthey are transiently opening, since we can assume that plancat.cdid that already.In passing, change gin_clean_pending_list() to take RowExclusiveLocknot AccessShareLock on its target index. Although it's not clear thatthat's actually a bug, it seemed very strange for a function that'sexplicitly going to modify the index to use only AccessShareLock.David Rowley, reviewed by Julien Rouhaud and Amit Langote,a bit of further tweaking by meDiscussion:https://postgr.es/m/19465.1541636036@sss.pgh.pa.us
1 parenta96c41f commit9c703c1

File tree

8 files changed

+39
-59
lines changed

8 files changed

+39
-59
lines changed

‎src/backend/access/gin/ginfast.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1031,7 +1031,7 @@ Datum
10311031
gin_clean_pending_list(PG_FUNCTION_ARGS)
10321032
{
10331033
Oidindexoid=PG_GETARG_OID(0);
1034-
RelationindexRel=index_open(indexoid,AccessShareLock);
1034+
RelationindexRel=index_open(indexoid,RowExclusiveLock);
10351035
IndexBulkDeleteResultstats;
10361036
GinStateginstate;
10371037

@@ -1068,7 +1068,7 @@ gin_clean_pending_list(PG_FUNCTION_ARGS)
10681068
initGinState(&ginstate,indexRel);
10691069
ginInsertCleanup(&ginstate, true, true, true,&stats);
10701070

1071-
index_close(indexRel,AccessShareLock);
1071+
index_close(indexRel,RowExclusiveLock);
10721072

10731073
PG_RETURN_INT64((int64)stats.pages_deleted);
10741074
}

‎src/backend/executor/execUtils.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -664,6 +664,10 @@ ExecCreateScanSlotFromOuterPlan(EState *estate,
664664
*
665665
*Detect whether a relation (identified by rangetable index)
666666
*is one of the target relations of the query.
667+
*
668+
* Note: This is currently no longer used in core. We keep it around
669+
* because FDWs may wish to use it to determine if their foreign table
670+
* is a target relation.
667671
* ----------------------------------------------------------------
668672
*/
669673
bool

‎src/backend/executor/nodeBitmapIndexscan.c

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ BitmapIndexScanState *
211211
ExecInitBitmapIndexScan(BitmapIndexScan*node,EState*estate,inteflags)
212212
{
213213
BitmapIndexScanState*indexstate;
214-
boolrelistarget;
214+
LOCKMODElockmode;
215215

216216
/* check for unsupported flags */
217217
Assert(!(eflags& (EXEC_FLAG_BACKWARD |EXEC_FLAG_MARK)));
@@ -260,16 +260,9 @@ ExecInitBitmapIndexScan(BitmapIndexScan *node, EState *estate, int eflags)
260260
if (eflags&EXEC_FLAG_EXPLAIN_ONLY)
261261
returnindexstate;
262262

263-
/*
264-
* Open the index relation.
265-
*
266-
* If the parent table is one of the target relations of the query, then
267-
* InitPlan already opened and write-locked the index, so we can avoid
268-
* taking another lock here. Otherwise we need a normal reader's lock.
269-
*/
270-
relistarget=ExecRelationIsTargetRelation(estate,node->scan.scanrelid);
271-
indexstate->biss_RelationDesc=index_open(node->indexid,
272-
relistarget ?NoLock :AccessShareLock);
263+
/* Open the index relation. */
264+
lockmode=exec_rt_fetch(node->scan.scanrelid,estate)->rellockmode;
265+
indexstate->biss_RelationDesc=index_open(node->indexid,lockmode);
273266

274267
/*
275268
* Initialize index-specific scan state

‎src/backend/executor/nodeIndexonlyscan.c

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -493,7 +493,7 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
493493
{
494494
IndexOnlyScanState*indexstate;
495495
RelationcurrentRelation;
496-
boolrelistarget;
496+
LOCKMODElockmode;
497497
TupleDesctupDesc;
498498

499499
/*
@@ -556,16 +556,9 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
556556
if (eflags&EXEC_FLAG_EXPLAIN_ONLY)
557557
returnindexstate;
558558

559-
/*
560-
* Open the index relation.
561-
*
562-
* If the parent table is one of the target relations of the query, then
563-
* InitPlan already opened and write-locked the index, so we can avoid
564-
* taking another lock here. Otherwise we need a normal reader's lock.
565-
*/
566-
relistarget=ExecRelationIsTargetRelation(estate,node->scan.scanrelid);
567-
indexstate->ioss_RelationDesc=index_open(node->indexid,
568-
relistarget ?NoLock :AccessShareLock);
559+
/* Open the index relation. */
560+
lockmode=exec_rt_fetch(node->scan.scanrelid,estate)->rellockmode;
561+
indexstate->ioss_RelationDesc=index_open(node->indexid,lockmode);
569562

570563
/*
571564
* Initialize index-specific scan state

‎src/backend/executor/nodeIndexscan.c

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -901,7 +901,7 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
901901
{
902902
IndexScanState*indexstate;
903903
RelationcurrentRelation;
904-
boolrelistarget;
904+
LOCKMODElockmode;
905905

906906
/*
907907
* create state structure
@@ -964,16 +964,9 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
964964
if (eflags&EXEC_FLAG_EXPLAIN_ONLY)
965965
returnindexstate;
966966

967-
/*
968-
* Open the index relation.
969-
*
970-
* If the parent table is one of the target relations of the query, then
971-
* InitPlan already opened and write-locked the index, so we can avoid
972-
* taking another lock here. Otherwise we need a normal reader's lock.
973-
*/
974-
relistarget=ExecRelationIsTargetRelation(estate,node->scan.scanrelid);
975-
indexstate->iss_RelationDesc=index_open(node->indexid,
976-
relistarget ?NoLock :AccessShareLock);
967+
/* Open the index relation. */
968+
lockmode=exec_rt_fetch(node->scan.scanrelid,estate)->rellockmode;
969+
indexstate->iss_RelationDesc=index_open(node->indexid,lockmode);
977970

978971
/*
979972
* Initialize index-specific scan state

‎src/backend/optimizer/plan/planner.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6285,6 +6285,7 @@ plan_create_index_workers(Oid tableOid, Oid indexOid)
62856285
/* Build RelOptInfo */
62866286
rel=build_simple_rel(root,1,NULL);
62876287

6288+
/* Rels are assumed already locked by the caller */
62886289
heap=table_open(tableOid,NoLock);
62896290
index=index_open(indexOid,NoLock);
62906291

‎src/backend/optimizer/util/plancat.c

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -162,8 +162,8 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
162162
if (hasindex)
163163
{
164164
List*indexoidlist;
165-
ListCell*l;
166165
LOCKMODElmode;
166+
ListCell*l;
167167

168168
indexoidlist=RelationGetIndexList(relation);
169169

@@ -172,13 +172,10 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
172172
* need, and do not release it. This saves a couple of trips to the
173173
* shared lock manager while not creating any real loss of
174174
* concurrency, because no schema changes could be happening on the
175-
* index while we hold lock on the parent rel, andneither lock type
176-
* blocks any other kind of index operation.
175+
* index while we hold lock on the parent rel, andno lock type used
176+
*for queriesblocks any other kind of index operation.
177177
*/
178-
if (rel->relid==root->parse->resultRelation)
179-
lmode=RowExclusiveLock;
180-
else
181-
lmode=AccessShareLock;
178+
lmode=root->simple_rte_array[varno]->rellockmode;
182179

183180
foreach(l,indexoidlist)
184181
{
@@ -592,8 +589,8 @@ infer_arbiter_indexes(PlannerInfo *root)
592589
OnConflictExpr*onconflict=root->parse->onConflict;
593590

594591
/* Iteration state */
592+
RangeTblEntry*rte;
595593
Relationrelation;
596-
OidrelationObjectId;
597594
OidindexOidFromConstraint=InvalidOid;
598595
List*indexList;
599596
ListCell*l;
@@ -620,10 +617,9 @@ infer_arbiter_indexes(PlannerInfo *root)
620617
* the rewriter or when expand_inherited_rtentry() added it to the query's
621618
* rangetable.
622619
*/
623-
relationObjectId=rt_fetch(root->parse->resultRelation,
624-
root->parse->rtable)->relid;
620+
rte=rt_fetch(root->parse->resultRelation,root->parse->rtable);
625621

626-
relation=table_open(relationObjectId,NoLock);
622+
relation=table_open(rte->relid,NoLock);
627623

628624
/*
629625
* Build normalized/BMS representation of plain indexed attributes, as
@@ -687,15 +683,14 @@ infer_arbiter_indexes(PlannerInfo *root)
687683
ListCell*el;
688684

689685
/*
690-
* Extract info from the relation descriptor for the index. We know
691-
* that this is a target, so get lock type it is known will ultimately
692-
* be required by the executor.
686+
* Extract info from the relation descriptor for the index. Obtain
687+
* the same lock type that the executor will ultimately use.
693688
*
694689
* Let executor complain about !indimmediate case directly, because
695690
* enforcement needs to occur there anyway when an inference clause is
696691
* omitted.
697692
*/
698-
idxRel=index_open(indexoid,RowExclusiveLock);
693+
idxRel=index_open(indexoid,rte->rellockmode);
699694
idxForm=idxRel->rd_index;
700695

701696
if (!idxForm->indisvalid)

‎src/backend/utils/adt/selfuncs.c

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5187,11 +5187,10 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata,
51875187

51885188
/*
51895189
* Open the table and index so we can read from them. We should
5190-
* already have at least AccessShareLock on the table, but not
5191-
* necessarily on the index.
5190+
* already have some type of lock on each.
51925191
*/
51935192
heapRel=table_open(rte->relid,NoLock);
5194-
indexRel=index_open(index->indexoid,AccessShareLock);
5193+
indexRel=index_open(index->indexoid,NoLock);
51955194

51965195
/* extract index key information from the index's pg_index info */
51975196
indexInfo=BuildIndexInfo(indexRel);
@@ -5305,7 +5304,7 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata,
53055304
/* Clean everything up */
53065305
ExecDropSingleTupleTableSlot(slot);
53075306

5308-
index_close(indexRel,AccessShareLock);
5307+
index_close(indexRel,NoLock);
53095308
table_close(heapRel,NoLock);
53105309

53115310
MemoryContextSwitchTo(oldcontext);
@@ -6472,9 +6471,10 @@ gincostestimate(PlannerInfo *root, IndexPath *path, double loop_count,
64726471
*/
64736472
if (!index->hypothetical)
64746473
{
6475-
indexRel=index_open(index->indexoid,AccessShareLock);
6474+
/* Lock should have already been obtained in plancat.c */
6475+
indexRel=index_open(index->indexoid,NoLock);
64766476
ginGetStats(indexRel,&ginStats);
6477-
index_close(indexRel,AccessShareLock);
6477+
index_close(indexRel,NoLock);
64786478
}
64796479
else
64806480
{
@@ -6781,11 +6781,12 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count,
67816781
&spc_seq_page_cost);
67826782

67836783
/*
6784-
* Obtain some data from the index itself.
6784+
* Obtain some data from the index itself. A lock should have already
6785+
* been obtained on the index in plancat.c.
67856786
*/
6786-
indexRel=index_open(index->indexoid,AccessShareLock);
6787+
indexRel=index_open(index->indexoid,NoLock);
67876788
brinGetStats(indexRel,&statsData);
6788-
index_close(indexRel,AccessShareLock);
6789+
index_close(indexRel,NoLock);
67896790

67906791
/*
67916792
* Compute index correlation

0 commit comments

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