Skip to content

Commit 14c57cb

Browse files
nmischnmisch
committed
For inplace update durability, make heap_update() callers wait.
The previous commit fixed some ways of losing an inplace update. It remained possible to lose one when a backend working toward a heap_update() copied a tuple into memory just before inplace update of that tuple. In catalogs eligible for inplace update, use LOCKTAG_TUPLE to govern admission to the steps of copying an old tuple, modifying it, and issuing heap_update(). This includes MERGE commands. To avoid changing most of the pg_class DDL, don't require LOCKTAG_TUPLE when holding a relation lock sufficient to exclude inplace updaters. Back-patch to v12 (all supported versions). In v13 and v12, "UPDATE pg_class" or "UPDATE pg_database" can still lose an inplace update. The v14+ UPDATE fix needs commit 86dc900, and it wasn't worth reimplementing that fix without such infrastructure. Reviewed by Nitin Motiani and (in earlier versions) Heikki Linnakangas. Discussion: https://postgr.es/m/20231027214946.79.nmisch@google.com
1 parent a8ad192 commit 14c57cb

File tree

17 files changed

+423
-34
lines changed

17 files changed

+423
-34
lines changed

‎src/backend/access/heap/README.tuplock

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,48 @@ The following infomask bits are applicable:
154154
We currently never set the HEAP_XMAX_COMMITTED when the HEAP_XMAX_IS_MULTI bit
155155
is set.
156156

157+
Locking to write inplace-updated tables
158+
---------------------------------------
159+
160+
If IsInplaceUpdateRelation() returns true for a table, the table is a system
161+
catalog that receives systable_inplace_update_begin() calls. Preparing a
162+
heap_update() of these tables follows additional locking rules, to ensure we
163+
don't lose the effects of an inplace update. In particular, consider a moment
164+
when a backend has fetched the old tuple to modify, not yet having called
165+
heap_update(). Another backend's inplace update starting then can't conclude
166+
until the heap_update() places its new tuple in a buffer. We enforce that
167+
using locktags as follows. While DDL code is the main audience, the executor
168+
follows these rules to make e.g. "MERGE INTO pg_class" safer. Locking rules
169+
are per-catalog:
170+
171+
pg_class systable_inplace_update_begin() callers: before the call, acquire a
172+
lock on the relation in mode ShareUpdateExclusiveLock or stricter. If the
173+
update targets a row of RELKIND_INDEX (but not RELKIND_PARTITIONED_INDEX),
174+
that lock must be on the table. Locking the index rel is not necessary.
175+
(This allows VACUUM to overwrite per-index pg_class while holding a lock on
176+
the table alone.) systable_inplace_update_begin() acquires and releases
177+
LOCKTAG_TUPLE in InplaceUpdateTupleLock, an alias for ExclusiveLock, on each
178+
tuple it overwrites.
179+
180+
pg_class heap_update() callers: before copying the tuple to modify, take a
181+
lock on the tuple, a ShareUpdateExclusiveLock on the relation, or a
182+
ShareRowExclusiveLock or stricter on the relation.
183+
184+
SearchSysCacheLocked1() is one convenient way to acquire the tuple lock.
185+
Most heap_update() callers already hold a suitable lock on the relation for
186+
other reasons and can skip the tuple lock. If you do acquire the tuple
187+
lock, release it immediately after the update.
188+
189+
190+
pg_database: before copying the tuple to modify, all updaters of pg_database
191+
rows acquire LOCKTAG_TUPLE. (Few updaters acquire LOCKTAG_OBJECT on the
192+
database OID, so it wasn't worth extending that as a second option.)
193+
194+
Ideally, DDL might want to perform permissions checks before LockTuple(), as
195+
we do with RangeVarGetRelidExtended() callbacks. We typically don't bother.
196+
LOCKTAG_TUPLE acquirers release it after each row, so the potential
197+
inconvenience is lower.
198+
157199
Reading inplace-updated columns
158200
-------------------------------
159201

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

Lines changed: 149 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@
5151
#include "access/xloginsert.h"
5252
#include "access/xlogutils.h"
5353
#include "catalog/catalog.h"
54+
#include "catalog/pg_database.h"
55+
#include "catalog/pg_database_d.h"
5456
#include "miscadmin.h"
5557
#include "pgstat.h"
5658
#include "port/atomics.h"
@@ -76,6 +78,12 @@ static XLogRecPtr log_heap_update(Relation reln, Buffer oldbuf,
7678
Buffer newbuf, HeapTuple oldtup,
7779
HeapTuple newtup, HeapTuple old_key_tuple,
7880
bool all_visible_cleared, bool new_all_visible_cleared);
81+
#ifdef USE_ASSERT_CHECKING
82+
static void check_lock_if_inplace_updateable_rel(Relation relation,
83+
ItemPointer otid,
84+
HeapTuple newtup);
85+
static void check_inplace_rel_lock(HeapTuple oldtup);
86+
#endif
7987
static Bitmapset *HeapDetermineColumnsInfo(Relation relation,
8088
Bitmapset *interesting_cols,
8189
Bitmapset *external_cols,
@@ -115,6 +123,8 @@ static HeapTuple ExtractReplicaIdentity(Relation rel, HeapTuple tup, bool key_re
115123
* heavyweight lock mode and MultiXactStatus values to use for any particular
116124
* tuple lock strength.
117125
*
126+
* These interact with InplaceUpdateTupleLock, an alias for ExclusiveLock.
127+
*
118128
* Don't look at lockstatus/updstatus directly! Use get_mxact_status_for_lock
119129
* instead.
120130
*/
@@ -2975,6 +2985,10 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
29752985
(errcode(ERRCODE_INVALID_TRANSACTION_STATE),
29762986
errmsg("cannot update tuples during a parallel operation")));
29772987

2988+
#ifdef USE_ASSERT_CHECKING
2989+
check_lock_if_inplace_updateable_rel(relation, otid, newtup);
2990+
#endif
2991+
29782992
/*
29792993
* Fetch the list of attributes to be checked for various operations.
29802994
*
@@ -3821,6 +3835,128 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
38213835
return TM_Ok;
38223836
}
38233837

3838+
#ifdef USE_ASSERT_CHECKING
3839+
/*
3840+
* Confirm adequate lock held during heap_update(), per rules from
3841+
* README.tuplock section "Locking to write inplace-updated tables".
3842+
*/
3843+
static void
3844+
check_lock_if_inplace_updateable_rel(Relation relation,
3845+
ItemPointer otid,
3846+
HeapTuple newtup)
3847+
{
3848+
/* LOCKTAG_TUPLE acceptable for any catalog */
3849+
switch (RelationGetRelid(relation))
3850+
{
3851+
case RelationRelationId:
3852+
case DatabaseRelationId:
3853+
{
3854+
LOCKTAG tuptag;
3855+
3856+
SET_LOCKTAG_TUPLE(tuptag,
3857+
relation->rd_lockInfo.lockRelId.dbId,
3858+
relation->rd_lockInfo.lockRelId.relId,
3859+
ItemPointerGetBlockNumber(otid),
3860+
ItemPointerGetOffsetNumber(otid));
3861+
if (LockHeldByMe(&tuptag, InplaceUpdateTupleLock))
3862+
return;
3863+
}
3864+
break;
3865+
default:
3866+
Assert(!IsInplaceUpdateRelation(relation));
3867+
return;
3868+
}
3869+
3870+
switch (RelationGetRelid(relation))
3871+
{
3872+
case RelationRelationId:
3873+
{
3874+
/* LOCKTAG_TUPLE or LOCKTAG_RELATION ok */
3875+
Form_pg_class classForm = (Form_pg_class) GETSTRUCT(newtup);
3876+
Oid relid = classForm->oid;
3877+
Oid dbid;
3878+
LOCKTAG tag;
3879+
3880+
if (IsSharedRelation(relid))
3881+
dbid = InvalidOid;
3882+
else
3883+
dbid = MyDatabaseId;
3884+
3885+
if (classForm->relkind == RELKIND_INDEX)
3886+
{
3887+
Relation irel = index_open(relid, AccessShareLock);
3888+
3889+
SET_LOCKTAG_RELATION(tag, dbid, irel->rd_index->indrelid);
3890+
index_close(irel, AccessShareLock);
3891+
}
3892+
else
3893+
SET_LOCKTAG_RELATION(tag, dbid, relid);
3894+
3895+
if (!LockHeldByMe(&tag, ShareUpdateExclusiveLock) &&
3896+
!LockOrStrongerHeldByMe(&tag, ShareRowExclusiveLock))
3897+
elog(WARNING,
3898+
"missing lock for relation \"%s\" (OID %u, relkind %c) @ TID (%u,%u)",
3899+
NameStr(classForm->relname),
3900+
relid,
3901+
classForm->relkind,
3902+
ItemPointerGetBlockNumber(otid),
3903+
ItemPointerGetOffsetNumber(otid));
3904+
}
3905+
break;
3906+
case DatabaseRelationId:
3907+
{
3908+
/* LOCKTAG_TUPLE required */
3909+
Form_pg_database dbForm = (Form_pg_database) GETSTRUCT(newtup);
3910+
3911+
elog(WARNING,
3912+
"missing lock on database \"%s\" (OID %u) @ TID (%u,%u)",
3913+
NameStr(dbForm->datname),
3914+
dbForm->oid,
3915+
ItemPointerGetBlockNumber(otid),
3916+
ItemPointerGetOffsetNumber(otid));
3917+
}
3918+
break;
3919+
}
3920+
}
3921+
3922+
/*
3923+
* Confirm adequate relation lock held, per rules from README.tuplock section
3924+
* "Locking to write inplace-updated tables".
3925+
*/
3926+
static void
3927+
check_inplace_rel_lock(HeapTuple oldtup)
3928+
{
3929+
Form_pg_class classForm = (Form_pg_class) GETSTRUCT(oldtup);
3930+
Oid relid = classForm->oid;
3931+
Oid dbid;
3932+
LOCKTAG tag;
3933+
3934+
if (IsSharedRelation(relid))
3935+
dbid = InvalidOid;
3936+
else
3937+
dbid = MyDatabaseId;
3938+
3939+
if (classForm->relkind == RELKIND_INDEX)
3940+
{
3941+
Relation irel = index_open(relid, AccessShareLock);
3942+
3943+
SET_LOCKTAG_RELATION(tag, dbid, irel->rd_index->indrelid);
3944+
index_close(irel, AccessShareLock);
3945+
}
3946+
else
3947+
SET_LOCKTAG_RELATION(tag, dbid, relid);
3948+
3949+
if (!LockOrStrongerHeldByMe(&tag, ShareUpdateExclusiveLock))
3950+
elog(WARNING,
3951+
"missing lock for relation \"%s\" (OID %u, relkind %c) @ TID (%u,%u)",
3952+
NameStr(classForm->relname),
3953+
relid,
3954+
classForm->relkind,
3955+
ItemPointerGetBlockNumber(&oldtup->t_self),
3956+
ItemPointerGetOffsetNumber(&oldtup->t_self));
3957+
}
3958+
#endif
3959+
38243960
/*
38253961
* Check if the specified attribute's values are the same. Subroutine for
38263962
* HeapDetermineColumnsInfo.
@@ -5848,15 +5984,21 @@ heap_inplace_lock(Relation relation,
58485984
TM_Result result;
58495985
bool ret;
58505986

5987+
#ifdef USE_ASSERT_CHECKING
5988+
if (RelationGetRelid(relation) == RelationRelationId)
5989+
check_inplace_rel_lock(oldtup_ptr);
5990+
#endif
5991+
58515992
Assert(BufferIsValid(buffer));
58525993

5994+
LockTuple(relation, &oldtup.t_self, InplaceUpdateTupleLock);
58535995
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
58545996

58555997
/*----------
58565998
* Interpret HeapTupleSatisfiesUpdate() like heap_update() does, except:
58575999
*
58586000
* - wait unconditionally
5859-
* - no tuple locks
6001+
* - already locked tuple above, since inplace needs that unconditionally
58606002
* - don't recheck header after wait: simpler to defer to next iteration
58616003
* - don't try to continue even if the updater aborts: likewise
58626004
* - no crosscheck
@@ -5940,7 +6082,10 @@ heap_inplace_lock(Relation relation,
59406082
* don't bother optimizing that.
59416083
*/
59426084
if (!ret)
6085+
{
6086+
UnlockTuple(relation, &oldtup.t_self, InplaceUpdateTupleLock);
59436087
InvalidateCatalogSnapshot();
6088+
}
59446089
return ret;
59456090
}
59466091

@@ -5949,6 +6094,8 @@ heap_inplace_lock(Relation relation,
59496094
*
59506095
* The tuple cannot change size, and therefore its header fields and null
59516096
* bitmap (if any) don't change either.
6097+
*
6098+
* Since we hold LOCKTAG_TUPLE, no updater has a local copy of this tuple.
59526099
*/
59536100
void
59546101
heap_inplace_update_and_unlock(Relation relation,
@@ -6032,6 +6179,7 @@ heap_inplace_unlock(Relation relation,
60326179
HeapTuple oldtup, Buffer buffer)
60336180
{
60346181
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
6182+
UnlockTuple(relation, &oldtup->t_self, InplaceUpdateTupleLock);
60356183
}
60366184

60376185
/*

‎src/backend/access/index/genam.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -674,7 +674,9 @@ systable_endscan_ordered(SysScanDesc sysscan)
674674
*
675675
* Overwriting violates both MVCC and transactional safety, so the uses of
676676
* this function in Postgres are extremely limited. Nonetheless we find some
677-
* places to use it. Standard flow:
677+
* places to use it. See README.tuplock section "Locking to write
678+
* inplace-updated tables" and later sections for expectations of readers and
679+
* writers of a table that gets inplace updates. Standard flow:
678680
*
679681
* ... [any slow preparation not requiring oldtup] ...
680682
* systable_inplace_update_begin([...], &tup, &inplace_state);

‎src/backend/catalog/aclchk.c

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@
6868
#include "nodes/makefuncs.h"
6969
#include "parser/parse_func.h"
7070
#include "parser/parse_type.h"
71+
#include "storage/lmgr.h"
7172
#include "utils/acl.h"
7273
#include "utils/aclchk_internal.h"
7374
#include "utils/builtins.h"
@@ -1796,7 +1797,7 @@ ExecGrant_Relation(InternalGrant *istmt)
17961797
HeapTuple tuple;
17971798
ListCell *cell_colprivs;
17981799

1799-
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relOid));
1800+
tuple = SearchSysCacheLocked1(RELOID, ObjectIdGetDatum(relOid));
18001801
if (!HeapTupleIsValid(tuple))
18011802
elog(ERROR, "cache lookup failed for relation %u", relOid);
18021803
pg_class_tuple = (Form_pg_class) GETSTRUCT(tuple);
@@ -2012,6 +2013,7 @@ ExecGrant_Relation(InternalGrant *istmt)
20122013
values, nulls, replaces);
20132014

20142015
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
2016+
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
20152017

20162018
/* Update initial privileges for extensions */
20172019
recordExtensionInitPriv(relOid, RelationRelationId, 0, new_acl);
@@ -2024,6 +2026,8 @@ ExecGrant_Relation(InternalGrant *istmt)
20242026

20252027
pfree(new_acl);
20262028
}
2029+
else
2030+
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
20272031

20282032
/*
20292033
* Handle column-level privileges, if any were specified or implied.
@@ -2133,7 +2137,7 @@ ExecGrant_Database(InternalGrant *istmt)
21332137
Oid *newmembers;
21342138
HeapTuple tuple;
21352139

2136-
tuple = SearchSysCache1(DATABASEOID, ObjectIdGetDatum(datId));
2140+
tuple = SearchSysCacheLocked1(DATABASEOID, ObjectIdGetDatum(datId));
21372141
if (!HeapTupleIsValid(tuple))
21382142
elog(ERROR, "cache lookup failed for database %u", datId);
21392143

@@ -2202,6 +2206,7 @@ ExecGrant_Database(InternalGrant *istmt)
22022206
nulls, replaces);
22032207

22042208
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
2209+
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
22052210

22062211
/* Update the shared dependency ACL info */
22072212
updateAclDependencies(DatabaseRelationId, pg_database_tuple->oid, 0,

‎src/backend/catalog/catalog.c

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,15 @@ IsCatalogRelationOid(Oid relid)
133133
/*
134134
* IsInplaceUpdateRelation
135135
* True iff core code performs inplace updates on the relation.
136+
*
137+
* This is used for assertions and for making the executor follow the
138+
* locking protocol described at README.tuplock section "Locking to write
139+
* inplace-updated tables". Extensions may inplace-update other heap
140+
* tables, but concurrent SQL UPDATE on the same table may overwrite
141+
* those modifications.
142+
*
143+
* The executor can assume these are not partitions or partitioned and
144+
* have no triggers.
136145
*/
137146
bool
138147
IsInplaceUpdateRelation(Relation relation)

0 commit comments

Comments
 (0)