Author: Noah Misch Commit: Noah Misch diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 14de815..8e51557 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -3730,6 +3730,15 @@ l2: id_has_external, &old_key_copied); + /* + * FIXME fail if updating pg_class without holding either (a) + * LOCKTAG_RELATION on this row's rel, in a mode conflicting w/ ShareLock, + * or (b) LOCKTAG_TUPLE. Fail if updating pg_database without holding + * either (a) LOCKTAG_OBJECT on this row's database OID, in FIXME mode, or + * (b) LOCKTAG_TUPLE. This may imply the executor taking the tuple locks + * during SQL UPDATE of those tables. + */ + /* NO EREPORT(ERROR) from here till changes are logged */ START_CRIT_SECTION(); @@ -5892,6 +5901,7 @@ heap_inplace_update(Relation relation, HeapTuple tuple) HeapTupleHeader htup; uint32 oldlen; uint32 newlen; + HeapTupleData oldtup; /* * For now, we don't allow parallel updates. Unlike a regular update, @@ -5904,6 +5914,8 @@ heap_inplace_update(Relation relation, HeapTuple tuple) (errcode(ERRCODE_INVALID_TRANSACTION_STATE), errmsg("cannot update tuples during a parallel operation"))); + /* FIXME fail unless holding LockTuple(relation, &tuple->t-self, AccessExclusiveLock */ + buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(&(tuple->t_self))); LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE); page = (Page) BufferGetPage(buffer); @@ -5922,6 +5934,16 @@ heap_inplace_update(Relation relation, HeapTuple tuple) if (oldlen != newlen || htup->t_hoff != tuple->t_data->t_hoff) elog(ERROR, "wrong tuple length"); + /* evaluate whether updated */ + /* FIXME wait for a concurrent updater, in case it aborts */ + oldtup.t_tableOid = RelationGetRelid(relation); + oldtup.t_data = htup; + oldtup.t_len = ItemIdGetLength(lp); + oldtup.t_self = tuple->t_self; + if (TM_Ok != + HeapTupleSatisfiesUpdate(&oldtup, GetCurrentCommandId(false), buffer)) + elog(ERROR, "tuple concurrently updated"); + /* NO EREPORT(ERROR) from here till changes are logged */ START_CRIT_SECTION(); diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 3ce6c09..dba3137 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -1853,7 +1853,7 @@ ExecGrant_Relation(InternalGrant *istmt) HeapTuple tuple; ListCell *cell_colprivs; - tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relOid)); + tuple = SearchSysCacheLocked1(relation, RELOID, ObjectIdGetDatum(relOid)); if (!HeapTupleIsValid(tuple)) elog(ERROR, "cache lookup failed for relation %u", relOid); pg_class_tuple = (Form_pg_class) GETSTRUCT(tuple); @@ -2190,7 +2190,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs, Oid *oldmembers; Oid *newmembers; - tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objectid)); + tuple = SearchSysCacheLocked1(relation, cacheid, ObjectIdGetDatum(objectid)); if (!HeapTupleIsValid(tuple)) elog(ERROR, "cache lookup failed for %s %u", get_object_class_descr(classid), objectid); diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 143fae0..7c02380 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -2860,11 +2860,19 @@ index_update_stats(Relation rel, tuple = heap_getnext(pg_class_scan, ForwardScanDirection); tuple = heap_copytuple(tuple); table_endscan(pg_class_scan); + LockTuple(pg_class, &tuple->t_self, AccessExclusiveLock); + /* FIXME loop in case the pg_class tuple for pg_class moved */ } else { + HeapTuple cachetup; + /* normal case, use syscache */ - tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relid)); + cachetup = SearchSysCacheLocked1(pg_class, + RELOID, ObjectIdGetDatum(relid)); + tuple = heap_copytuple(cachetup); + if (HeapTupleIsValid(cachetup)) + ReleaseSysCache(cachetup); } if (!HeapTupleIsValid(tuple)) @@ -2933,6 +2941,7 @@ index_update_stats(Relation rel, CacheInvalidateRelcacheByTuple(tuple); } + UnlockTuple(pg_class, &tuple->t_self, AccessExclusiveLock); heap_freetuple(tuple); table_close(pg_class, RowExclusiveLock); diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c index 8dbda00..76be8b5 100644 --- a/src/backend/utils/cache/syscache.c +++ b/src/backend/utils/cache/syscache.c @@ -76,6 +76,7 @@ #include "catalog/pg_type.h" #include "catalog/pg_user_mapping.h" #include "lib/qunique.h" +#include "storage/lmgr.h" #include "utils/catcache.h" #include "utils/lsyscache.h" #include "utils/rel.h" @@ -827,6 +828,70 @@ SearchSysCache1(int cacheId, return SearchCatCache1(SysCache[cacheId], key1); } +/* + * Like SearchSysCache1, but a LOCKTAG_TUPLE heavyweight lock is held in + * AccessExclusiveMode on return. + * + * pg_class and pg_database are subject to both heap_inplace_update() and + * regular heap_update(). We must not lose the effects of any inplace update. + * That might arise by heap_inplace_update() mutating a tuple, the xmax of + * which then commits. Alternatively, it could arise by heap_inplace_update() + * mutating a tuple before an ongoing transaction's heap_update(), after the + * ongoing transaction fetches the old tuple to modify. We prevent both with + * locking as follows. All heap_inplace_update(pg_class) callers acquire two + * locks. First, they acquire LOCKTAG_RELATION in mode ShareLock, + * ShareUpdateExclusiveLock, or a mode with strictly more conflicts. Second, + * they acquire LOCKTAG_TUPLE. heap_update(pg_class) callers can either take + * any lock that conflicts with one of those two. Acquire one's choice of + * lock before fetching the tuple to be modified. FIXME write corresponding + * story for pg_database. + * + * This function bundles the tasks of retrieving a tuple and acquiring the + * LOCKTAG_TUPLE. Since much DDL opts to acquire LOCKTAG_RELATION, the + * LOCKTAG_TUPLE doesn't block such updaters of the returned tuple. Callers + * shall reject superseded tuples, such as by checking with + * HeapTupleSatisfiesUpdate() while holding an exclusive lock on the tuple's + * buffer. + * + * We opt to loop until the search finds the same tuple we've locked. FIXME + * This might not be strictly necessary, but it likely avoids some spurious + * errors. In the happy case, this takes two fetches, one to determine the + * tid to lock and another to confirm that the TID remains the latest tuple. + * + * FIXME consider dropping Relation arg and deducing applicable locktag fields + * from cacheId. + * + * FIXME this may belong in a different file, with available key counts other + * than 1, etc. + */ +HeapTuple +SearchSysCacheLocked1(Relation rel, + int cacheId, + Datum key1) +{ + ItemPointerData tid; + bool retry = false; + + for (;;) + { + HeapTuple tuple; + + tuple = SearchSysCache1(cacheId, key1); + if (!HeapTupleIsValid(tuple) || + (retry && ItemPointerEquals(&tid, &tuple->t_self))) + return tuple; + + if (retry) + UnlockTuple(rel, &tid, AccessExclusiveLock); + + tid = tuple->t_self; + ReleaseSysCache(tuple); + LockTuple(rel, &tid, AccessExclusiveLock); + retry = true; + } +} + + HeapTuple SearchSysCache2(int cacheId, Datum key1, Datum key2) diff --git a/src/include/utils/syscache.h b/src/include/utils/syscache.h index 5d47a65..aba5385 100644 --- a/src/include/utils/syscache.h +++ b/src/include/utils/syscache.h @@ -18,6 +18,7 @@ #include "access/attnum.h" #include "access/htup.h" +#include "utils/rel.h" /* we intentionally do not include utils/catcache.h here */ /* @@ -174,6 +175,10 @@ extern bool RelationInvalidatesSnapshotsOnly(Oid relid); extern bool RelationHasSysCache(Oid relid); extern bool RelationSupportsSysCache(Oid relid); +extern HeapTuple SearchSysCacheLocked1(Relation rel, + int cacheId, + Datum key1); + /* * The use of the macros below rather than direct calls to the corresponding * functions is encouraged, as it insulates the caller from changes in the diff --git a/src/test/isolation/expected/intra-grant-inplace.out b/src/test/isolation/expected/intra-grant-inplace.out new file mode 100644 index 0000000..a1eed4e --- /dev/null +++ b/src/test/isolation/expected/intra-grant-inplace.out @@ -0,0 +1,23 @@ +Parsed test spec with 2 sessions + +starting permutation: b1 g1 s2 a2 c1 s2 +step b1: BEGIN ISOLATION LEVEL READ COMMITTED; +step g1: GRANT SELECT ON intra_grant_inplace TO PUBLIC; +step s2: SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass; +relhasindex +----------- +f +(1 row) + +step a2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); +step c1: COMMIT; +step a2: <... completed> +ERROR: tuple concurrently updated +step s2: SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass; +relhasindex +----------- +f +(1 row) + diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index b2be88e..fcc4ad4 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -86,6 +86,7 @@ test: alter-table-3 test: alter-table-4 test: create-trigger test: sequence-ddl +test: intra-grant-inplace test: async-notify test: vacuum-no-cleanup-lock test: timeouts diff --git a/src/test/isolation/specs/intra-grant-inplace.spec b/src/test/isolation/specs/intra-grant-inplace.spec new file mode 100644 index 0000000..85059ed --- /dev/null +++ b/src/test/isolation/specs/intra-grant-inplace.spec @@ -0,0 +1,25 @@ +# GRANT's lock is the catalog tuple xmax. GRANT doesn't acquire a heavyweight +# lock on the object undergoing an ACL change. In-place updates, such as +# relhasindex=true, need special code to cope. + +setup +{ + CREATE TABLE intra_grant_inplace (c int); +} + +teardown +{ + DROP TABLE intra_grant_inplace; +} + +session s1 +step b1 { BEGIN ISOLATION LEVEL READ COMMITTED; } +step g1 { GRANT SELECT ON intra_grant_inplace TO PUBLIC; } +step c1 { COMMIT; } + +session s2 +step s2 { SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass; } +step a2 { ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); } + +permutation b1 g1 s2 a2 c1 s2