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

Commit2ad36c4

Browse files
committed
Improve table locking behavior in the face of current DDL.
In the previous coding, callers were faced with an awkward choice:look up the name, do permissions checks, and then lock the table; orlook up the name, lock the table, and then do permissions checks.The first choice was wrong because the results of the name lookupand permissions checks might be out-of-date by the time the tablelock was acquired, while the second allowed a user with no privilegesto interfere with access to a table by users who do have privileges(e.g. if a malicious backend queues up for an AccessExclusiveLock ona table on which AccessShareLock is already held, further attemptsto access the table will be blocked until the AccessExclusiveLockis obtained and the malicious backend's transaction rolls back).To fix, allow callers of RangeVarGetRelid() to pass a callback whichgets executed after performing the name lookup but before acquiringthe relation lock. If the name lookup is retried (becauseinvalidation messages are received), the callback will be re-executedas well, so we get the best of both worlds. RangeVarGetRelid() isrenamed to RangeVarGetRelidExtended(); callers not wishing to supplya callback can continue to invoke it as RangeVarGetRelid(), which isnow a macro. Since the only one caller that uses nowait = true nowpasses a callback anyway, the RangeVarGetRelid() macro defaults nowaitas well. The callback can also be used for supplemental locking - forexample, REINDEX INDEX needs to acquire the table lock before the indexlock to reduce deadlock possibilities.There's a lot more work to be done here to fix all the cases where thiscan be a problem, but this commit provides the general infrastructureand fixes the following specific cases: REINDEX INDEX, REINDEX TABLE,LOCK TABLE, and and DROP TABLE/INDEX/SEQUENCE/VIEW/FOREIGN TABLE.Per discussion with Noah Misch and Alvaro Herrera.
1 parenta87ebac commit2ad36c4

File tree

21 files changed

+339
-199
lines changed

21 files changed

+339
-199
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -979,7 +979,7 @@ relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
979979
OidrelOid;
980980

981981
/* Look up and lock the appropriate relation using namespace search */
982-
relOid=RangeVarGetRelid(relation,lockmode, false, false);
982+
relOid=RangeVarGetRelid(relation,lockmode, false);
983983

984984
/* Let relation_open do the rest */
985985
returnrelation_open(relOid,NoLock);
@@ -1001,7 +1001,7 @@ relation_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
10011001
OidrelOid;
10021002

10031003
/* Look up and lock the appropriate relation using namespace search */
1004-
relOid=RangeVarGetRelid(relation,lockmode,missing_ok, false);
1004+
relOid=RangeVarGetRelid(relation,lockmode,missing_ok);
10051005

10061006
/* Return NULL on not-found */
10071007
if (!OidIsValid(relOid))

‎src/backend/catalog/aclchk.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -606,7 +606,7 @@ objectNamesToOids(GrantObjectType objtype, List *objnames)
606606
RangeVar*relvar= (RangeVar*)lfirst(cell);
607607
OidrelOid;
608608

609-
relOid=RangeVarGetRelid(relvar,NoLock, false, false);
609+
relOid=RangeVarGetRelid(relvar,NoLock, false);
610610
objects=lappend_oid(objects,relOid);
611611
}
612612
break;

‎src/backend/catalog/index.c

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,6 @@ static void validate_index_heapscan(Relation heapRelation,
111111
IndexInfo*indexInfo,
112112
Snapshotsnapshot,
113113
v_i_state*state);
114-
staticOidIndexGetRelation(OidindexId);
115114
staticboolReindexIsCurrentlyProcessingIndex(OidindexOid);
116115
staticvoidSetReindexProcessing(OidheapOid,OidindexOid);
117116
staticvoidResetReindexProcessing(void);
@@ -1301,7 +1300,7 @@ index_drop(Oid indexId)
13011300
* proceeding until we commit and send out a shared-cache-inval notice
13021301
* that will make them update their index lists.
13031302
*/
1304-
heapId=IndexGetRelation(indexId);
1303+
heapId=IndexGetRelation(indexId, false);
13051304
userHeapRelation=heap_open(heapId,AccessExclusiveLock);
13061305

13071306
userIndexRelation=index_open(indexId,AccessExclusiveLock);
@@ -2763,16 +2762,20 @@ validate_index_heapscan(Relation heapRelation,
27632762
* IndexGetRelation: given an index's relation OID, get the OID of the
27642763
* relation it is an index on.Uses the system cache.
27652764
*/
2766-
staticOid
2767-
IndexGetRelation(OidindexId)
2765+
Oid
2766+
IndexGetRelation(OidindexId,boolmissing_ok)
27682767
{
27692768
HeapTupletuple;
27702769
Form_pg_indexindex;
27712770
Oidresult;
27722771

27732772
tuple=SearchSysCache1(INDEXRELID,ObjectIdGetDatum(indexId));
27742773
if (!HeapTupleIsValid(tuple))
2774+
{
2775+
if (missing_ok)
2776+
returnInvalidOid;
27752777
elog(ERROR,"cache lookup failed for index %u",indexId);
2778+
}
27762779
index= (Form_pg_index)GETSTRUCT(tuple);
27772780
Assert(index->indexrelid==indexId);
27782781

@@ -2800,7 +2803,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks)
28002803
* Open and lock the parent heap relation.ShareLock is sufficient since
28012804
* we only need to be sure no schema or data changes are going on.
28022805
*/
2803-
heapId=IndexGetRelation(indexId);
2806+
heapId=IndexGetRelation(indexId, false);
28042807
heapRelation=heap_open(heapId,ShareLock);
28052808

28062809
/*

‎src/backend/catalog/namespace.c

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,10 +219,14 @@ Datumpg_is_other_temp_schema(PG_FUNCTION_ARGS);
219219
* otherwise raise an error.
220220
*
221221
* If nowait = true, throw an error if we'd have to wait for a lock.
222+
*
223+
* Callback allows caller to check permissions or acquire additional locks
224+
* prior to grabbing the relation lock.
222225
*/
223226
Oid
224-
RangeVarGetRelid(constRangeVar*relation,LOCKMODElockmode,boolmissing_ok,
225-
boolnowait)
227+
RangeVarGetRelidExtended(constRangeVar*relation,LOCKMODElockmode,
228+
boolmissing_ok,boolnowait,
229+
RangeVarGetRelidCallbackcallback,void*callback_arg)
226230
{
227231
uint64inval_count;
228232
OidrelId;
@@ -308,6 +312,19 @@ RangeVarGetRelid(const RangeVar *relation, LOCKMODE lockmode, bool missing_ok,
308312
relId=RelnameGetRelid(relation->relname);
309313
}
310314

315+
/*
316+
* Invoke caller-supplied callback, if any.
317+
*
318+
* This callback is a good place to check permissions: we haven't taken
319+
* the table lock yet (and it's really best to check permissions before
320+
* locking anything!), but we've gotten far enough to know what OID we
321+
* think we should lock. Of course, concurrent DDL might things while
322+
* we're waiting for the lock, but in that case the callback will be
323+
* invoked again for the new OID.
324+
*/
325+
if (callback)
326+
callback(relation,relId,oldRelId,callback_arg);
327+
311328
/*
312329
* If no lock requested, we assume the caller knows what they're
313330
* doing. They should have already acquired a heavyweight lock on

‎src/backend/commands/alter.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ ExecRenameStmt(RenameStmt *stmt)
111111
* in RenameRelation, renameatt, or renametrig.
112112
*/
113113
relid=RangeVarGetRelid(stmt->relation,AccessExclusiveLock,
114-
false, false);
114+
false);
115115

116116
switch (stmt->renameType)
117117
{

‎src/backend/commands/indexcmds.c

Lines changed: 97 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,10 @@ static void ComputeIndexAttrs(IndexInfo *indexInfo,
6363
staticOidGetIndexOpClass(List*opclass,OidattrType,
6464
char*accessMethodName,OidaccessMethodId);
6565
staticchar*ChooseIndexNameAddition(List*colnames);
66-
66+
staticvoidRangeVarCallbackForReindexTable(constRangeVar*relation,
67+
OidrelId,OidoldRelId,void*arg);
68+
staticvoidRangeVarCallbackForReindexIndex(constRangeVar*relation,
69+
OidrelId,OidoldRelId,void*arg);
6770

6871
/*
6972
* CheckIndexCompatible
@@ -129,7 +132,7 @@ CheckIndexCompatible(Oid oldId,
129132
Datumd;
130133

131134
/* Caller should already have the relation locked in some way. */
132-
relationId=RangeVarGetRelid(heapRelation,NoLock, false, false);
135+
relationId=RangeVarGetRelid(heapRelation,NoLock, false);
133136
/*
134137
* We can pretend isconstraint = false unconditionally. It only serves to
135138
* decide the text of an error message that should never happen for us.
@@ -1725,33 +1728,74 @@ void
17251728
ReindexIndex(RangeVar*indexRelation)
17261729
{
17271730
OidindOid;
1728-
HeapTupletuple;
1731+
OidheapOid=InvalidOid;
1732+
1733+
/* lock level used here should match index lock reindex_index() */
1734+
indOid=RangeVarGetRelidExtended(indexRelation,AccessExclusiveLock,
1735+
false, false,
1736+
RangeVarCallbackForReindexIndex,
1737+
(void*)&heapOid);
1738+
1739+
reindex_index(indOid, false);
1740+
}
1741+
1742+
/*
1743+
* Check permissions on table before acquiring relation lock; also lock
1744+
* the heap before the RangeVarGetRelidExtended takes the index lock, to avoid
1745+
* deadlocks.
1746+
*/
1747+
staticvoid
1748+
RangeVarCallbackForReindexIndex(constRangeVar*relation,
1749+
OidrelId,OidoldRelId,void*arg)
1750+
{
1751+
charrelkind;
1752+
Oid*heapOid= (Oid*)arg;
17291753

17301754
/*
1731-
* XXX: This is not safe in the presence of concurrent DDL. We should
1732-
* take AccessExclusiveLock here, but that would violate the rule that
1733-
* indexes should only be locked after their parent tables. For now,
1734-
* we live with it.
1755+
* If we previously locked some other index's heap, and the name we're
1756+
* looking up no longer refers to that relation, release the now-useless
1757+
* lock.
17351758
*/
1736-
indOid=RangeVarGetRelid(indexRelation,NoLock, false, false);
1737-
tuple=SearchSysCache1(RELOID,ObjectIdGetDatum(indOid));
1738-
if (!HeapTupleIsValid(tuple))
1739-
elog(ERROR,"cache lookup failed for relation %u",indOid);
1759+
if (relId!=oldRelId&&OidIsValid(oldRelId))
1760+
{
1761+
/* lock level here should match reindex_index() heap lock */
1762+
UnlockRelationOid(*heapOid,ShareLock);
1763+
*heapOid=InvalidOid;
1764+
}
1765+
1766+
/* If the relation does not exist, there's nothing more to do. */
1767+
if (!OidIsValid(relId))
1768+
return;
17401769

1741-
if (((Form_pg_class)GETSTRUCT(tuple))->relkind!=RELKIND_INDEX)
1770+
/*
1771+
* If the relation does exist, check whether it's an index. But note
1772+
* that the relation might have been dropped between the time we did the
1773+
* name lookup and now. In that case, there's nothing to do.
1774+
*/
1775+
relkind=get_rel_relkind(relId);
1776+
if (!relkind)
1777+
return;
1778+
if (relkind!=RELKIND_INDEX)
17421779
ereport(ERROR,
17431780
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
1744-
errmsg("\"%s\" is not an index",
1745-
indexRelation->relname)));
1781+
errmsg("\"%s\" is not an index",relation->relname)));
17461782

17471783
/* Check permissions */
1748-
if (!pg_class_ownercheck(indOid,GetUserId()))
1749-
aclcheck_error(ACLCHECK_NOT_OWNER,ACL_KIND_CLASS,
1750-
indexRelation->relname);
1751-
1752-
ReleaseSysCache(tuple);
1784+
if (!pg_class_ownercheck(relId,GetUserId()))
1785+
aclcheck_error(ACLCHECK_NOT_OWNER,ACL_KIND_CLASS,relation->relname);
17531786

1754-
reindex_index(indOid, false);
1787+
/* Lock heap before index to avoid deadlock. */
1788+
if (relId!=oldRelId)
1789+
{
1790+
/*
1791+
* Lock level here should match reindex_index() heap lock.
1792+
* If the OID isn't valid, it means the index as concurrently dropped,
1793+
* which is not a problem for us; just return normally.
1794+
*/
1795+
*heapOid=IndexGetRelation(relId, true);
1796+
if (OidIsValid(*heapOid))
1797+
LockRelationOid(*heapOid,ShareLock);
1798+
}
17551799
}
17561800

17571801
/*
@@ -1762,34 +1806,48 @@ void
17621806
ReindexTable(RangeVar*relation)
17631807
{
17641808
OidheapOid;
1765-
HeapTupletuple;
17661809

17671810
/* The lock level used here should match reindex_relation(). */
1768-
heapOid=RangeVarGetRelid(relation,ShareLock, false, false);
1769-
tuple=SearchSysCache1(RELOID,ObjectIdGetDatum(heapOid));
1770-
if (!HeapTupleIsValid(tuple))/* shouldn't happen */
1771-
elog(ERROR,"cache lookup failed for relation %u",heapOid);
1772-
1773-
if (((Form_pg_class)GETSTRUCT(tuple))->relkind!=RELKIND_RELATION&&
1774-
((Form_pg_class)GETSTRUCT(tuple))->relkind!=RELKIND_TOASTVALUE)
1775-
ereport(ERROR,
1776-
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
1777-
errmsg("\"%s\" is not a table",
1778-
relation->relname)));
1779-
1780-
/* Check permissions */
1781-
if (!pg_class_ownercheck(heapOid,GetUserId()))
1782-
aclcheck_error(ACLCHECK_NOT_OWNER,ACL_KIND_CLASS,
1783-
relation->relname);
1784-
1785-
ReleaseSysCache(tuple);
1811+
heapOid=RangeVarGetRelidExtended(relation,ShareLock, false, false,
1812+
RangeVarCallbackForReindexTable,NULL);
17861813

17871814
if (!reindex_relation(heapOid,REINDEX_REL_PROCESS_TOAST))
17881815
ereport(NOTICE,
17891816
(errmsg("table \"%s\" has no indexes",
17901817
relation->relname)));
17911818
}
17921819

1820+
/*
1821+
* Check permissions on table before acquiring relation lock.
1822+
*/
1823+
staticvoid
1824+
RangeVarCallbackForReindexTable(constRangeVar*relation,
1825+
OidrelId,OidoldRelId,void*arg)
1826+
{
1827+
charrelkind;
1828+
1829+
/* Nothing to do if the relation was not found. */
1830+
if (!OidIsValid(relId))
1831+
return;
1832+
1833+
/*
1834+
* If the relation does exist, check whether it's an index. But note
1835+
* that the relation might have been dropped between the time we did the
1836+
* name lookup and now. In that case, there's nothing to do.
1837+
*/
1838+
relkind=get_rel_relkind(relId);
1839+
if (!relkind)
1840+
return;
1841+
if (relkind!=RELKIND_RELATION&&relkind!=RELKIND_TOASTVALUE)
1842+
ereport(ERROR,
1843+
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
1844+
errmsg("\"%s\" is not a table",relation->relname)));
1845+
1846+
/* Check permissions */
1847+
if (!pg_class_ownercheck(relId,GetUserId()))
1848+
aclcheck_error(ACLCHECK_NOT_OWNER,ACL_KIND_CLASS,relation->relname);
1849+
}
1850+
17931851
/*
17941852
* ReindexDatabase
17951853
*Recreate indexes of a database.

0 commit comments

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