Skip to content

Commit c139317

Browse files
tglsfdctglsfdc
committed
Avoid redundant relation lock grabs during planning, and make sure
that we acquire a lock on relations added to the query due to inheritance. Formerly, no such lock was held throughout planning, which meant that a schema change could occur to invalidate the plan before it's even been completed.
1 parent 353f111 commit c139317

File tree

3 files changed

+40
-21
lines changed

3 files changed

+40
-21
lines changed

‎src/backend/optimizer/prep/preptlist.c

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
* Portions Copyright (c) 1994, Regents of the University of California
1616
*
1717
* IDENTIFICATION
18-
* $PostgreSQL: pgsql/src/backend/optimizer/prep/preptlist.c,v 1.75 2005/04/28 21:47:14 tgl Exp $
18+
* $PostgreSQL: pgsql/src/backend/optimizer/prep/preptlist.c,v 1.76 2005/05/23 03:01:13 tgl Exp $
1919
*
2020
*-------------------------------------------------------------------------
2121
*/
@@ -189,9 +189,11 @@ expand_targetlist(List *tlist, int command_type,
189189
* attributes.
190190
*
191191
* Scan the tuple description in the relation's relcache entry to make
192-
* sure we have all the user attributes in the right order.
192+
* sure we have all the user attributes in the right order. We assume
193+
* that the rewriter already acquired at least AccessShareLock on the
194+
* relation, so we need no lock here.
193195
*/
194-
rel = heap_open(getrelid(result_relation, range_table), AccessShareLock);
196+
rel = heap_open(getrelid(result_relation, range_table), NoLock);
195197

196198
numattrs = RelationGetNumberOfAttributes(rel);
197199

@@ -326,7 +328,7 @@ expand_targetlist(List *tlist, int command_type,
326328
tlist_item = lnext(tlist_item);
327329
}
328330

329-
heap_close(rel, AccessShareLock);
331+
heap_close(rel, NoLock);
330332

331333
return new_tlist;
332334
}

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

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
*
1010
*
1111
* IDENTIFICATION
12-
* $PostgreSQL: pgsql/src/backend/optimizer/util/plancat.c,v 1.107 2005/05/22 22:30:20 tgl Exp $
12+
* $PostgreSQL: pgsql/src/backend/optimizer/util/plancat.c,v 1.108 2005/05/23 03:01:14 tgl Exp $
1313
*
1414
*-------------------------------------------------------------------------
1515
*/
@@ -67,7 +67,25 @@ get_relation_info(Oid relationObjectId, RelOptInfo *rel)
6767
bool hasindex;
6868
List *indexinfos = NIL;
6969

70-
relation = heap_open(relationObjectId, AccessShareLock);
70+
/*
71+
* Normally, we can assume the rewriter already acquired at least
72+
* AccessShareLock on each relation used in the query. However this
73+
* will not be the case for relations added to the query because they
74+
* are inheritance children of some relation mentioned explicitly.
75+
* For them, this is the first access during the parse/rewrite/plan
76+
* pipeline, and so we need to obtain and keep a suitable lock.
77+
*
78+
* XXX really, a suitable lock is RowShareLock if the relation is
79+
* an UPDATE/DELETE target, and AccessShareLock otherwise. However
80+
* we cannot easily tell here which to get, so for the moment just
81+
* get AccessShareLock always. The executor will get the right lock
82+
* when it runs, which means there is a very small chance of deadlock
83+
* trying to upgrade our lock.
84+
*/
85+
if (rel->reloptkind == RELOPT_BASEREL)
86+
relation = heap_open(relationObjectId, NoLock);
87+
else
88+
relation = heap_open(relationObjectId, AccessShareLock);
7189

7290
rel->min_attr = FirstLowInvalidHeapAttributeNumber + 1;
7391
rel->max_attr = RelationGetNumberOfAttributes(relation);
@@ -205,8 +223,8 @@ get_relation_info(Oid relationObjectId, RelOptInfo *rel)
205223

206224
rel->indexlist = indexinfos;
207225

208-
/* XXX keep the lock here? */
209-
heap_close(relation, AccessShareLock);
226+
/* close rel, but keep lock if any */
227+
heap_close(relation, NoLock);
210228
}
211229

212230
/*
@@ -374,7 +392,8 @@ build_physical_tlist(Query *root, RelOptInfo *rel)
374392
switch (rte->rtekind)
375393
{
376394
case RTE_RELATION:
377-
relation = heap_open(rte->relid, AccessShareLock);
395+
/* Assume we already have adequate lock */
396+
relation = heap_open(rte->relid, NoLock);
378397

379398
numattrs = RelationGetNumberOfAttributes(relation);
380399
for (attrno = 1; attrno <= numattrs; attrno++)
@@ -401,7 +420,7 @@ build_physical_tlist(Query *root, RelOptInfo *rel)
401420
false));
402421
}
403422

404-
heap_close(relation, AccessShareLock);
423+
heap_close(relation, NoLock);
405424
break;
406425

407426
case RTE_SUBQUERY:

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

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/optimizer/util/relnode.c,v 1.65 2005/04/22 21:58:31 tgl Exp $
11+
* $PostgreSQL: pgsql/src/backend/optimizer/util/relnode.c,v 1.66 2005/05/23 03:01:14 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -23,7 +23,8 @@
2323
#include "parser/parsetree.h"
2424

2525

26-
static RelOptInfo *make_base_rel(Query *root, int relid);
26+
static RelOptInfo *make_reloptinfo(Query *root, int relid,
27+
RelOptKind reloptkind);
2728
static void build_joinrel_tlist(Query *root, RelOptInfo *joinrel);
2829
static List *build_joinrel_restrictlist(Query *root,
2930
RelOptInfo *joinrel,
@@ -67,7 +68,7 @@ build_base_rel(Query *root, int relid)
6768
}
6869

6970
/* No existing RelOptInfo for this base rel, so make a new one */
70-
rel = make_base_rel(root, relid);
71+
rel = make_reloptinfo(root, relid, RELOPT_BASEREL);
7172

7273
/* and add it to the list */
7374
root->base_rel_list = lcons(rel, root->base_rel_list);
@@ -102,11 +103,8 @@ build_other_rel(Query *root, int relid)
102103
}
103104

104105
/* No existing RelOptInfo for this other rel, so make a new one */
105-
rel = make_base_rel(root, relid);
106-
107106
/* presently, must be an inheritance child rel */
108-
Assert(rel->reloptkind == RELOPT_BASEREL);
109-
rel->reloptkind = RELOPT_OTHER_CHILD_REL;
107+
rel = make_reloptinfo(root, relid, RELOPT_OTHER_CHILD_REL);
110108

111109
/* and add it to the list */
112110
root->other_rel_list = lcons(rel, root->other_rel_list);
@@ -115,18 +113,18 @@ build_other_rel(Query *root, int relid)
115113
}
116114

117115
/*
118-
* make_base_rel
119-
* Construct a base-relation RelOptInfo for the specified rangetable index.
116+
* make_reloptinfo
117+
* Construct a RelOptInfo for the specified rangetable index.
120118
*
121119
* Common code for build_base_rel and build_other_rel.
122120
*/
123121
static RelOptInfo *
124-
make_base_rel(Query *root, int relid)
122+
make_reloptinfo(Query *root, int relid, RelOptKind reloptkind)
125123
{
126124
RelOptInfo *rel = makeNode(RelOptInfo);
127125
RangeTblEntry *rte = rt_fetch(relid, root->rtable);
128126

129-
rel->reloptkind = RELOPT_BASEREL;
127+
rel->reloptkind = reloptkind;
130128
rel->relids = bms_make_singleton(relid);
131129
rel->rows = 0;
132130
rel->width = 0;

0 commit comments

Comments
 (0)