From a98871b7ac601b4ebe6ba050b1f9cbfdd5d71ded Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 3 Dec 2005 05:51:03 +0000 Subject: [PATCH] Tweak indexscan machinery to avoid taking an AccessShareLock on an index if we already have a stronger lock due to the index's table being the update target table of the query. Same optimization I applied earlier at the table level. There doesn't seem to be much interest in the more radical idea of not locking indexes at all, so do what we can ... --- src/backend/access/heap/tuptoaster.c | 14 +++++------ src/backend/access/index/genam.c | 9 ++++--- src/backend/access/index/indexam.c | 29 +++++++++++++++++----- src/backend/catalog/catalog.c | 5 ++-- src/backend/commands/cluster.c | 5 ++-- src/backend/executor/execUtils.c | 24 +++++++++++++++++- src/backend/executor/nodeBitmapIndexscan.c | 13 +++++++--- src/backend/executor/nodeIndexscan.c | 12 ++++++--- src/backend/storage/large_object/inv_api.c | 8 +++--- src/include/access/genam.h | 4 ++- src/include/access/relscan.h | 3 ++- src/include/executor/executor.h | 4 ++- 12 files changed, 95 insertions(+), 35 deletions(-) diff --git a/src/backend/access/heap/tuptoaster.c b/src/backend/access/heap/tuptoaster.c index 1b762597cb..4a1ed4e7ed 100644 --- a/src/backend/access/heap/tuptoaster.c +++ b/src/backend/access/heap/tuptoaster.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/access/heap/tuptoaster.c,v 1.56 2005/11/22 18:17:06 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/access/heap/tuptoaster.c,v 1.57 2005/12/03 05:50:59 tgl Exp $ * * * INTERFACE ROUTINES @@ -1142,8 +1142,8 @@ toast_delete_datum(Relation rel, Datum value) /* * Find the chunks by index */ - toastscan = index_beginscan(toastrel, toastidx, SnapshotToast, - 1, &toastkey); + toastscan = index_beginscan(toastrel, toastidx, true, + SnapshotToast, 1, &toastkey); while ((toasttup = index_getnext(toastscan, ForwardScanDirection)) != NULL) { /* @@ -1219,8 +1219,8 @@ toast_fetch_datum(varattrib *attr) */ nextidx = 0; - toastscan = index_beginscan(toastrel, toastidx, SnapshotToast, - 1, &toastkey); + toastscan = index_beginscan(toastrel, toastidx, true, + SnapshotToast, 1, &toastkey); while ((ttup = index_getnext(toastscan, ForwardScanDirection)) != NULL) { /* @@ -1394,8 +1394,8 @@ toast_fetch_datum_slice(varattrib *attr, int32 sliceoffset, int32 length) * The index is on (valueid, chunkidx) so they will come in order */ nextidx = startchunk; - toastscan = index_beginscan(toastrel, toastidx, SnapshotToast, - nscankeys, toastkey); + toastscan = index_beginscan(toastrel, toastidx, true, + SnapshotToast, nscankeys, toastkey); while ((ttup = index_getnext(toastscan, ForwardScanDirection)) != NULL) { /* diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c index d32a9f9db9..6ca2fe594b 100644 --- a/src/backend/access/index/genam.c +++ b/src/backend/access/index/genam.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/access/index/genam.c,v 1.51 2005/11/22 18:17:06 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/access/index/genam.c,v 1.52 2005/12/03 05:51:00 tgl Exp $ * * NOTES * many of the old access method routines have been turned into @@ -86,7 +86,8 @@ RelationGetIndexScan(Relation indexRelation, else scan->keyData = NULL; - scan->is_multiscan = false; /* caller may change this */ + scan->is_multiscan = false; /* caller may change this */ + scan->have_lock = false; /* ditto */ scan->kill_prior_tuple = false; scan->ignore_killed_tuples = true; /* default setting */ scan->keys_are_unique = false; /* may be set by index AM */ @@ -211,8 +212,8 @@ systable_beginscan(Relation heapRelation, key[i].sk_attno = i + 1; } - sysscan->iscan = index_beginscan(heapRelation, irel, snapshot, - nkeys, key); + sysscan->iscan = index_beginscan(heapRelation, irel, true, + snapshot, nkeys, key); sysscan->scan = NULL; } else diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c index bd2e3bdd06..c1e61ff2bd 100644 --- a/src/backend/access/index/indexam.c +++ b/src/backend/access/index/indexam.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/access/index/indexam.c,v 1.86 2005/10/15 02:49:09 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/access/index/indexam.c,v 1.87 2005/12/03 05:51:00 tgl Exp $ * * INTERFACE ROUTINES * index_open - open an index relation by relation OID @@ -111,6 +111,7 @@ do { \ } while(0) static IndexScanDesc index_beginscan_internal(Relation indexRelation, + bool need_index_lock, int nkeys, ScanKey key); @@ -229,16 +230,23 @@ index_insert(Relation indexRelation, * heapRelation link (nor the snapshot). However, the caller had better * be holding some kind of lock on the heap relation in any case, to ensure * no one deletes it (or the index) out from under us. + * + * Most callers should pass need_index_lock = true to cause the index code + * to take AccessShareLock on the index for the duration of the scan. But + * if it is known that a lock is already held on the index, pass false to + * skip taking an unnecessary lock. */ IndexScanDesc index_beginscan(Relation heapRelation, Relation indexRelation, + bool need_index_lock, Snapshot snapshot, int nkeys, ScanKey key) { IndexScanDesc scan; - scan = index_beginscan_internal(indexRelation, nkeys, key); + scan = index_beginscan_internal(indexRelation, need_index_lock, + nkeys, key); /* * Save additional parameters into the scandesc. Everything else was set @@ -259,12 +267,14 @@ index_beginscan(Relation heapRelation, */ IndexScanDesc index_beginscan_multi(Relation indexRelation, + bool need_index_lock, Snapshot snapshot, int nkeys, ScanKey key) { IndexScanDesc scan; - scan = index_beginscan_internal(indexRelation, nkeys, key); + scan = index_beginscan_internal(indexRelation, need_index_lock, + nkeys, key); /* * Save additional parameters into the scandesc. Everything else was set @@ -281,6 +291,7 @@ index_beginscan_multi(Relation indexRelation, */ static IndexScanDesc index_beginscan_internal(Relation indexRelation, + bool need_index_lock, int nkeys, ScanKey key) { IndexScanDesc scan; @@ -291,13 +302,15 @@ index_beginscan_internal(Relation indexRelation, RelationIncrementReferenceCount(indexRelation); /* - * Acquire AccessShareLock for the duration of the scan + * Acquire AccessShareLock for the duration of the scan, unless caller + * says it already has lock on the index. * * Note: we could get an SI inval message here and consequently have to * rebuild the relcache entry. The refcount increment above ensures that * we will rebuild it and not just flush it... */ - LockRelation(indexRelation, AccessShareLock); + if (need_index_lock) + LockRelation(indexRelation, AccessShareLock); /* * LockRelation can clean rd_aminfo structure, so fill procedure after @@ -315,6 +328,9 @@ index_beginscan_internal(Relation indexRelation, Int32GetDatum(nkeys), PointerGetDatum(key))); + /* Save flag to tell index_endscan whether to release lock */ + scan->have_lock = need_index_lock; + return scan; } @@ -380,7 +396,8 @@ index_endscan(IndexScanDesc scan) /* Release index lock and refcount acquired by index_beginscan */ - UnlockRelation(scan->indexRelation, AccessShareLock); + if (scan->have_lock) + UnlockRelation(scan->indexRelation, AccessShareLock); RelationDecrementReferenceCount(scan->indexRelation); diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c index 69313ea86a..b63a754959 100644 --- a/src/backend/catalog/catalog.c +++ b/src/backend/catalog/catalog.c @@ -9,7 +9,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/catalog/catalog.c,v 1.64 2005/10/15 02:49:12 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/catalog/catalog.c,v 1.65 2005/12/03 05:51:00 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -309,7 +309,8 @@ GetNewOidWithIndex(Relation relation, Relation indexrel) ObjectIdGetDatum(newOid)); /* see notes above about using SnapshotDirty */ - scan = index_beginscan(relation, indexrel, SnapshotDirty, 1, &key); + scan = index_beginscan(relation, indexrel, true, + SnapshotDirty, 1, &key); collides = HeapTupleIsValid(index_getnext(scan, ForwardScanDirection)); diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 671c8bf091..e97cc83669 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -11,7 +11,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/cluster.c,v 1.142 2005/11/22 18:17:08 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/commands/cluster.c,v 1.143 2005/12/03 05:51:01 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -643,7 +643,8 @@ copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex) * Scan through the OldHeap on the OldIndex and copy each tuple into the * NewHeap. */ - scan = index_beginscan(OldHeap, OldIndex, SnapshotNow, 0, (ScanKey) NULL); + scan = index_beginscan(OldHeap, OldIndex, true, + SnapshotNow, 0, (ScanKey) NULL); while ((tuple = index_getnext(scan, ForwardScanDirection)) != NULL) { diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c index c814b97147..500ff0f4f2 100644 --- a/src/backend/executor/execUtils.c +++ b/src/backend/executor/execUtils.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/executor/execUtils.c,v 1.130 2005/12/02 20:03:40 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/executor/execUtils.c,v 1.131 2005/12/03 05:51:01 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -693,6 +693,28 @@ ExecAssignScanTypeFromOuterPlan(ScanState *scanstate) * ---------------------------------------------------------------- */ +/* ---------------------------------------------------------------- + * ExecRelationIsTargetRelation + * + * Detect whether a relation (identified by rangetable index) + * is one of the target relations of the query. + * ---------------------------------------------------------------- + */ +bool +ExecRelationIsTargetRelation(EState *estate, Index scanrelid) +{ + ResultRelInfo *resultRelInfos; + int i; + + resultRelInfos = estate->es_result_relations; + for (i = 0; i < estate->es_num_result_relations; i++) + { + if (resultRelInfos[i].ri_RangeTableIndex == scanrelid) + return true; + } + return false; +} + /* ---------------------------------------------------------------- * ExecOpenScanRelation * diff --git a/src/backend/executor/nodeBitmapIndexscan.c b/src/backend/executor/nodeBitmapIndexscan.c index 4217de3952..114de29ba8 100644 --- a/src/backend/executor/nodeBitmapIndexscan.c +++ b/src/backend/executor/nodeBitmapIndexscan.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/executor/nodeBitmapIndexscan.c,v 1.13 2005/12/02 20:03:40 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/executor/nodeBitmapIndexscan.c,v 1.14 2005/12/03 05:51:01 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -214,6 +214,7 @@ BitmapIndexScanState * ExecInitBitmapIndexScan(BitmapIndexScan *node, EState *estate) { BitmapIndexScanState *indexstate; + bool relistarget; /* * create state structure @@ -294,13 +295,19 @@ ExecInitBitmapIndexScan(BitmapIndexScan *node, EState *estate) indexstate->ss.ss_currentScanDesc = NULL; /* - * open the index relation and initialize relation and scan descriptors. + * Open the index relation and initialize relation and scan descriptors. * Note we acquire no locks here; the index machinery does its own locks - * and unlocks. + * and unlocks. (We rely on having a lock on the parent table to + * ensure the index won't go away!) Furthermore, if the parent table + * is one of the target relations of the query, then InitPlan already + * opened and write-locked the index, so we can tell the index machinery + * not to bother getting an extra lock. */ indexstate->biss_RelationDesc = index_open(node->indexid); + relistarget = ExecRelationIsTargetRelation(estate, node->scan.scanrelid); indexstate->biss_ScanDesc = index_beginscan_multi(indexstate->biss_RelationDesc, + !relistarget, estate->es_snapshot, indexstate->biss_NumScanKeys, indexstate->biss_ScanKeys); diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c index 4beecfbd57..94495b4bc7 100644 --- a/src/backend/executor/nodeIndexscan.c +++ b/src/backend/executor/nodeIndexscan.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/executor/nodeIndexscan.c,v 1.108 2005/12/02 20:03:40 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/executor/nodeIndexscan.c,v 1.109 2005/12/03 05:51:02 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -461,6 +461,7 @@ ExecInitIndexScan(IndexScan *node, EState *estate) { IndexScanState *indexstate; Relation currentRelation; + bool relistarget; /* * create state structure @@ -557,14 +558,19 @@ ExecInitIndexScan(IndexScan *node, EState *estate) ExecAssignScanType(&indexstate->ss, RelationGetDescr(currentRelation), false); /* - * open the index relation and initialize relation and scan descriptors. + * Open the index relation and initialize relation and scan descriptors. * Note we acquire no locks here; the index machinery does its own locks * and unlocks. (We rely on having a lock on the parent table to - * ensure the index won't go away!) + * ensure the index won't go away!) Furthermore, if the parent table + * is one of the target relations of the query, then InitPlan already + * opened and write-locked the index, so we can tell the index machinery + * not to bother getting an extra lock. */ indexstate->iss_RelationDesc = index_open(node->indexid); + relistarget = ExecRelationIsTargetRelation(estate, node->scan.scanrelid); indexstate->iss_ScanDesc = index_beginscan(currentRelation, indexstate->iss_RelationDesc, + !relistarget, estate->es_snapshot, indexstate->iss_NumScanKeys, indexstate->iss_ScanKeys); diff --git a/src/backend/storage/large_object/inv_api.c b/src/backend/storage/large_object/inv_api.c index 74409f3cd0..f6d4cf8045 100644 --- a/src/backend/storage/large_object/inv_api.c +++ b/src/backend/storage/large_object/inv_api.c @@ -9,7 +9,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/storage/large_object/inv_api.c,v 1.113 2005/10/15 02:49:26 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/storage/large_object/inv_api.c,v 1.114 2005/12/03 05:51:02 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -299,7 +299,7 @@ inv_getsize(LargeObjectDesc *obj_desc) BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(obj_desc->id)); - sd = index_beginscan(lo_heap_r, lo_index_r, + sd = index_beginscan(lo_heap_r, lo_index_r, true, obj_desc->snapshot, 1, skey); /* @@ -410,7 +410,7 @@ inv_read(LargeObjectDesc *obj_desc, char *buf, int nbytes) BTGreaterEqualStrategyNumber, F_INT4GE, Int32GetDatum(pageno)); - sd = index_beginscan(lo_heap_r, lo_index_r, + sd = index_beginscan(lo_heap_r, lo_index_r, true, obj_desc->snapshot, 2, skey); while ((tuple = index_getnext(sd, ForwardScanDirection)) != NULL) @@ -526,7 +526,7 @@ inv_write(LargeObjectDesc *obj_desc, char *buf, int nbytes) BTGreaterEqualStrategyNumber, F_INT4GE, Int32GetDatum(pageno)); - sd = index_beginscan(lo_heap_r, lo_index_r, + sd = index_beginscan(lo_heap_r, lo_index_r, false /* got lock */, obj_desc->snapshot, 2, skey); oldtuple = NULL; diff --git a/src/include/access/genam.h b/src/include/access/genam.h index acae7277a0..7dc33d3138 100644 --- a/src/include/access/genam.h +++ b/src/include/access/genam.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/access/genam.h,v 1.53 2005/10/15 02:49:42 momjian Exp $ + * $PostgreSQL: pgsql/src/include/access/genam.h,v 1.54 2005/12/03 05:51:03 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -79,9 +79,11 @@ extern bool index_insert(Relation indexRelation, extern IndexScanDesc index_beginscan(Relation heapRelation, Relation indexRelation, + bool need_index_lock, Snapshot snapshot, int nkeys, ScanKey key); extern IndexScanDesc index_beginscan_multi(Relation indexRelation, + bool need_index_lock, Snapshot snapshot, int nkeys, ScanKey key); extern void index_rescan(IndexScanDesc scan, ScanKey key); diff --git a/src/include/access/relscan.h b/src/include/access/relscan.h index c0b7c92cd5..dd6336ca26 100644 --- a/src/include/access/relscan.h +++ b/src/include/access/relscan.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/access/relscan.h,v 1.42 2005/11/26 03:03:07 tgl Exp $ + * $PostgreSQL: pgsql/src/include/access/relscan.h,v 1.43 2005/12/03 05:51:03 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -61,6 +61,7 @@ typedef struct IndexScanDescData int numberOfKeys; /* number of scan keys */ ScanKey keyData; /* array of scan key descriptors */ bool is_multiscan; /* TRUE = using amgetmulti */ + bool have_lock; /* TRUE = holding AccessShareLock for scan */ /* signaling to index AM about killing index tuples */ bool kill_prior_tuple; /* last-returned tuple is dead */ diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index f1d4e37c41..facb10a2e2 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/executor/executor.h,v 1.122 2005/12/02 20:03:42 tgl Exp $ + * $PostgreSQL: pgsql/src/include/executor/executor.h,v 1.123 2005/12/03 05:51:03 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -230,6 +230,8 @@ extern void ExecAssignScanType(ScanState *scanstate, TupleDesc tupDesc, bool shouldFree); extern void ExecAssignScanTypeFromOuterPlan(ScanState *scanstate); +extern bool ExecRelationIsTargetRelation(EState *estate, Index scanrelid); + extern Relation ExecOpenScanRelation(EState *estate, Index scanrelid); extern void ExecCloseScanRelation(Relation scanrel);