Skip to content

Commit 7aa772f

Browse files
tglsfdctglsfdc
committed
Now that we've rearranged relation open to get a lock before touching
the rel, it's easy to get rid of the narrow race-condition window that used to exist in VACUUM and CLUSTER. Did some minor code-beautification work in the same area, too.
1 parent e91600d commit 7aa772f

File tree

8 files changed

+151
-89
lines changed

8 files changed

+151
-89
lines changed

‎src/backend/access/heap/heapam.c

Lines changed: 63 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.218 2006/07/31 20:08:59 tgl Exp $
11+
* $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.219 2006/08/18 16:09:08 tgl Exp $
1212
*
1313
*
1414
* INTERFACE ROUTINES
@@ -53,6 +53,7 @@
5353
#include "utils/inval.h"
5454
#include "utils/lsyscache.h"
5555
#include "utils/relcache.h"
56+
#include "utils/syscache.h"
5657

5758

5859
static XLogRecPtr log_heap_update(Relation reln, Buffer oldbuf,
@@ -702,14 +703,56 @@ relation_open(Oid relationId, LOCKMODE lockmode)
702703
}
703704

704705
/* ----------------
705-
* conditional_relation_open - open with option not to wait
706+
* try_relation_open - open any relation by relation OID
706707
*
707-
* As above, but if nowait is true, then throw an error rather than
708-
* waiting when the lock is not immediately obtainable.
708+
* Same as relation_open, except return NULL instead of failing
709+
* if the relation does not exist.
709710
* ----------------
710711
*/
711712
Relation
712-
conditional_relation_open(Oid relationId, LOCKMODE lockmode, bool nowait)
713+
try_relation_open(Oid relationId, LOCKMODE lockmode)
714+
{
715+
Relation r;
716+
717+
Assert(lockmode >= NoLock && lockmode < MAX_LOCKMODES);
718+
719+
/* Get the lock first */
720+
if (lockmode != NoLock)
721+
LockRelationOid(relationId, lockmode);
722+
723+
/*
724+
* Now that we have the lock, probe to see if the relation really
725+
* exists or not.
726+
*/
727+
if (!SearchSysCacheExists(RELOID,
728+
ObjectIdGetDatum(relationId),
729+
0, 0, 0))
730+
{
731+
/* Release useless lock */
732+
if (lockmode != NoLock)
733+
UnlockRelationOid(relationId, lockmode);
734+
735+
return NULL;
736+
}
737+
738+
/* Should be safe to do a relcache load */
739+
r = RelationIdGetRelation(relationId);
740+
741+
if (!RelationIsValid(r))
742+
elog(ERROR, "could not open relation with OID %u", relationId);
743+
744+
return r;
745+
}
746+
747+
/* ----------------
748+
* relation_open_nowait - open but don't wait for lock
749+
*
750+
* Same as relation_open, except throw an error instead of waiting
751+
* when the requested lock is not immediately obtainable.
752+
* ----------------
753+
*/
754+
Relation
755+
relation_open_nowait(Oid relationId, LOCKMODE lockmode)
713756
{
714757
Relation r;
715758

@@ -718,27 +761,22 @@ conditional_relation_open(Oid relationId, LOCKMODE lockmode, bool nowait)
718761
/* Get the lock before trying to open the relcache entry */
719762
if (lockmode != NoLock)
720763
{
721-
if (nowait)
764+
if (!ConditionalLockRelationOid(relationId, lockmode))
722765
{
723-
if (!ConditionalLockRelationOid(relationId, lockmode))
724-
{
725-
/* try to throw error by name; relation could be deleted... */
726-
char *relname = get_rel_name(relationId);
727-
728-
if (relname)
729-
ereport(ERROR,
730-
(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
731-
errmsg("could not obtain lock on relation \"%s\"",
732-
relname)));
733-
else
734-
ereport(ERROR,
735-
(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
736-
errmsg("could not obtain lock on relation with OID %u",
737-
relationId)));
738-
}
766+
/* try to throw error by name; relation could be deleted... */
767+
char *relname = get_rel_name(relationId);
768+
769+
if (relname)
770+
ereport(ERROR,
771+
(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
772+
errmsg("could not obtain lock on relation \"%s\"",
773+
relname)));
774+
else
775+
ereport(ERROR,
776+
(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
777+
errmsg("could not obtain lock on relation with OID %u",
778+
relationId)));
739779
}
740-
else
741-
LockRelationOid(relationId, lockmode);
742780
}
743781

744782
/* The relcache does all the real work... */
@@ -753,7 +791,7 @@ conditional_relation_open(Oid relationId, LOCKMODE lockmode, bool nowait)
753791
/* ----------------
754792
* relation_openrv - open any relation specified by a RangeVar
755793
*
756-
* As above, but the relation is specified by a RangeVar.
794+
* Same as relation_open, but the relation is specified by a RangeVar.
757795
* ----------------
758796
*/
759797
Relation

‎src/backend/commands/analyze.c

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/commands/analyze.c,v 1.96 2006/07/14 14:52:18 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/commands/analyze.c,v 1.97 2006/08/18 16:09:08 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -129,20 +129,16 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt)
129129
CHECK_FOR_INTERRUPTS();
130130

131131
/*
132-
* Race condition -- if the pg_class tuple has gone away since the last
133-
* time we saw it, we don't need to process it.
132+
* Open the relation, getting only a read lock on it. If the rel has
133+
* been dropped since we last saw it, we don't need to process it.
134134
*/
135-
if (!SearchSysCacheExists(RELOID,
136-
ObjectIdGetDatum(relid),
137-
0, 0, 0))
135+
onerel = try_relation_open(relid, AccessShareLock);
136+
if (!onerel)
138137
return;
139138

140139
/*
141-
* Open the class, getting only a read lock on it, and check permissions.
142-
* Permissions check should match vacuum's check!
140+
* Check permissions --- this should match vacuum's check!
143141
*/
144-
onerel = relation_open(relid, AccessShareLock);
145-
146142
if (!(pg_class_ownercheck(RelationGetRelid(onerel), GetUserId()) ||
147143
(pg_database_ownercheck(MyDatabaseId, GetUserId()) && !onerel->rd_rel->relisshared)))
148144
{

‎src/backend/commands/cluster.c

Lines changed: 30 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
*
1212
*
1313
* IDENTIFICATION
14-
* $PostgreSQL: pgsql/src/backend/commands/cluster.c,v 1.152 2006/07/31 20:09:00 tgl Exp $
14+
* $PostgreSQL: pgsql/src/backend/commands/cluster.c,v 1.153 2006/08/18 16:09:08 tgl Exp $
1515
*
1616
*-------------------------------------------------------------------------
1717
*/
@@ -239,6 +239,18 @@ cluster_rel(RelToCluster *rvtc, bool recheck)
239239
/* Check for user-requested abort. */
240240
CHECK_FOR_INTERRUPTS();
241241

242+
/*
243+
* We grab exclusive access to the target rel and index for the duration
244+
* of the transaction. (This is redundant for the single-transaction
245+
* case, since cluster() already did it.) The index lock is taken inside
246+
* check_index_is_clusterable.
247+
*/
248+
OldHeap = try_relation_open(rvtc->tableOid, AccessExclusiveLock);
249+
250+
/* If the table has gone away, we can skip processing it */
251+
if (!OldHeap)
252+
return;
253+
242254
/*
243255
* Since we may open a new transaction for each relation, we have to check
244256
* that the relation still is what we think it is.
@@ -252,46 +264,45 @@ cluster_rel(RelToCluster *rvtc, bool recheck)
252264
HeapTuple tuple;
253265
Form_pg_index indexForm;
254266

267+
/* Check that the user still owns the relation */
268+
if (!pg_class_ownercheck(rvtc->tableOid, GetUserId()))
269+
{
270+
relation_close(OldHeap, AccessExclusiveLock);
271+
return;
272+
}
273+
255274
/*
256-
* Check if the relation and index still exist before opening them
275+
* Check that the index still exists
257276
*/
258277
if (!SearchSysCacheExists(RELOID,
259-
ObjectIdGetDatum(rvtc->tableOid),
260-
0, 0, 0) ||
261-
!SearchSysCacheExists(RELOID,
262278
ObjectIdGetDatum(rvtc->indexOid),
263279
0, 0, 0))
280+
{
281+
relation_close(OldHeap, AccessExclusiveLock);
264282
return;
265-
266-
/* Check that the user still owns the relation */
267-
if (!pg_class_ownercheck(rvtc->tableOid, GetUserId()))
268-
return;
283+
}
269284

270285
/*
271286
* Check that the index is still the one with indisclustered set.
272287
*/
273288
tuple = SearchSysCache(INDEXRELID,
274289
ObjectIdGetDatum(rvtc->indexOid),
275290
0, 0, 0);
276-
if (!HeapTupleIsValid(tuple))
277-
return; /* could have gone away... */
291+
if (!HeapTupleIsValid(tuple)) /* probably can't happen */
292+
{
293+
relation_close(OldHeap, AccessExclusiveLock);
294+
return;
295+
}
278296
indexForm = (Form_pg_index) GETSTRUCT(tuple);
279297
if (!indexForm->indisclustered)
280298
{
281299
ReleaseSysCache(tuple);
300+
relation_close(OldHeap, AccessExclusiveLock);
282301
return;
283302
}
284303
ReleaseSysCache(tuple);
285304
}
286305

287-
/*
288-
* We grab exclusive access to the target rel and index for the duration
289-
* of the transaction. (This is redundant for the single- transaction
290-
* case, since cluster() already did it.) The index lock is taken inside
291-
* check_index_is_clusterable.
292-
*/
293-
OldHeap = heap_open(rvtc->tableOid, AccessExclusiveLock);
294-
295306
/* Check index is valid to cluster on */
296307
check_index_is_clusterable(OldHeap, rvtc->indexOid, recheck);
297308

‎src/backend/commands/lockcmds.c

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/commands/lockcmds.c,v 1.14 2006/03/05 15:58:24 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/commands/lockcmds.c,v 1.15 2006/08/18 16:09:08 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -59,7 +59,10 @@ LockTableCommand(LockStmt *lockstmt)
5959
aclcheck_error(aclresult, ACL_KIND_CLASS,
6060
get_rel_name(reloid));
6161

62-
rel = conditional_relation_open(reloid, lockstmt->mode, lockstmt->nowait);
62+
if (lockstmt->nowait)
63+
rel = relation_open_nowait(reloid, lockstmt->mode);
64+
else
65+
rel = relation_open(reloid, lockstmt->mode);
6366

6467
/* Currently, we only allow plain tables to be locked */
6568
if (rel->rd_rel->relkind != RELKIND_RELATION)

0 commit comments

Comments
 (0)