Skip to content

Commit 5c837f8

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 8590c94 commit 5c837f8

File tree

19 files changed

+490
-49
lines changed

19 files changed

+490
-49
lines changed

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

+42
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

+149-1
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@
5252
#include "access/xloginsert.h"
5353
#include "access/xlogutils.h"
5454
#include "catalog/catalog.h"
55+
#include "catalog/pg_database.h"
56+
#include "catalog/pg_database_d.h"
5557
#include "miscadmin.h"
5658
#include "pgstat.h"
5759
#include "port/atomics.h"
@@ -78,6 +80,12 @@ static XLogRecPtr log_heap_update(Relation reln, Buffer oldbuf,
7880
Buffer newbuf, HeapTuple oldtup,
7981
HeapTuple newtup, HeapTuple old_key_tuple,
8082
bool all_visible_cleared, bool new_all_visible_cleared);
83+
#ifdef USE_ASSERT_CHECKING
84+
static void check_lock_if_inplace_updateable_rel(Relation relation,
85+
ItemPointer otid,
86+
HeapTuple newtup);
87+
static void check_inplace_rel_lock(HeapTuple oldtup);
88+
#endif
8189
static Bitmapset *HeapDetermineColumnsInfo(Relation relation,
8290
Bitmapset *interesting_cols,
8391
Bitmapset *external_cols,
@@ -119,6 +127,8 @@ static HeapTuple ExtractReplicaIdentity(Relation rel, HeapTuple tup, bool key_re
119127
* heavyweight lock mode and MultiXactStatus values to use for any particular
120128
* tuple lock strength.
121129
*
130+
* These interact with InplaceUpdateTupleLock, an alias for ExclusiveLock.
131+
*
122132
* Don't look at lockstatus/updstatus directly! Use get_mxact_status_for_lock
123133
* instead.
124134
*/
@@ -3187,6 +3197,10 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
31873197
(errcode(ERRCODE_INVALID_TRANSACTION_STATE),
31883198
errmsg("cannot update tuples during a parallel operation")));
31893199

3200+
#ifdef USE_ASSERT_CHECKING
3201+
check_lock_if_inplace_updateable_rel(relation, otid, newtup);
3202+
#endif
3203+
31903204
/*
31913205
* Fetch the list of attributes to be checked for various operations.
31923206
*
@@ -4014,6 +4028,128 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
40144028
return TM_Ok;
40154029
}
40164030

4031+
#ifdef USE_ASSERT_CHECKING
4032+
/*
4033+
* Confirm adequate lock held during heap_update(), per rules from
4034+
* README.tuplock section "Locking to write inplace-updated tables".
4035+
*/
4036+
static void
4037+
check_lock_if_inplace_updateable_rel(Relation relation,
4038+
ItemPointer otid,
4039+
HeapTuple newtup)
4040+
{
4041+
/* LOCKTAG_TUPLE acceptable for any catalog */
4042+
switch (RelationGetRelid(relation))
4043+
{
4044+
case RelationRelationId:
4045+
case DatabaseRelationId:
4046+
{
4047+
LOCKTAG tuptag;
4048+
4049+
SET_LOCKTAG_TUPLE(tuptag,
4050+
relation->rd_lockInfo.lockRelId.dbId,
4051+
relation->rd_lockInfo.lockRelId.relId,
4052+
ItemPointerGetBlockNumber(otid),
4053+
ItemPointerGetOffsetNumber(otid));
4054+
if (LockHeldByMe(&tuptag, InplaceUpdateTupleLock))
4055+
return;
4056+
}
4057+
break;
4058+
default:
4059+
Assert(!IsInplaceUpdateRelation(relation));
4060+
return;
4061+
}
4062+
4063+
switch (RelationGetRelid(relation))
4064+
{
4065+
case RelationRelationId:
4066+
{
4067+
/* LOCKTAG_TUPLE or LOCKTAG_RELATION ok */
4068+
Form_pg_class classForm = (Form_pg_class) GETSTRUCT(newtup);
4069+
Oid relid = classForm->oid;
4070+
Oid dbid;
4071+
LOCKTAG tag;
4072+
4073+
if (IsSharedRelation(relid))
4074+
dbid = InvalidOid;
4075+
else
4076+
dbid = MyDatabaseId;
4077+
4078+
if (classForm->relkind == RELKIND_INDEX)
4079+
{
4080+
Relation irel = index_open(relid, AccessShareLock);
4081+
4082+
SET_LOCKTAG_RELATION(tag, dbid, irel->rd_index->indrelid);
4083+
index_close(irel, AccessShareLock);
4084+
}
4085+
else
4086+
SET_LOCKTAG_RELATION(tag, dbid, relid);
4087+
4088+
if (!LockHeldByMe(&tag, ShareUpdateExclusiveLock) &&
4089+
!LockOrStrongerHeldByMe(&tag, ShareRowExclusiveLock))
4090+
elog(WARNING,
4091+
"missing lock for relation \"%s\" (OID %u, relkind %c) @ TID (%u,%u)",
4092+
NameStr(classForm->relname),
4093+
relid,
4094+
classForm->relkind,
4095+
ItemPointerGetBlockNumber(otid),
4096+
ItemPointerGetOffsetNumber(otid));
4097+
}
4098+
break;
4099+
case DatabaseRelationId:
4100+
{
4101+
/* LOCKTAG_TUPLE required */
4102+
Form_pg_database dbForm = (Form_pg_database) GETSTRUCT(newtup);
4103+
4104+
elog(WARNING,
4105+
"missing lock on database \"%s\" (OID %u) @ TID (%u,%u)",
4106+
NameStr(dbForm->datname),
4107+
dbForm->oid,
4108+
ItemPointerGetBlockNumber(otid),
4109+
ItemPointerGetOffsetNumber(otid));
4110+
}
4111+
break;
4112+
}
4113+
}
4114+
4115+
/*
4116+
* Confirm adequate relation lock held, per rules from README.tuplock section
4117+
* "Locking to write inplace-updated tables".
4118+
*/
4119+
static void
4120+
check_inplace_rel_lock(HeapTuple oldtup)
4121+
{
4122+
Form_pg_class classForm = (Form_pg_class) GETSTRUCT(oldtup);
4123+
Oid relid = classForm->oid;
4124+
Oid dbid;
4125+
LOCKTAG tag;
4126+
4127+
if (IsSharedRelation(relid))
4128+
dbid = InvalidOid;
4129+
else
4130+
dbid = MyDatabaseId;
4131+
4132+
if (classForm->relkind == RELKIND_INDEX)
4133+
{
4134+
Relation irel = index_open(relid, AccessShareLock);
4135+
4136+
SET_LOCKTAG_RELATION(tag, dbid, irel->rd_index->indrelid);
4137+
index_close(irel, AccessShareLock);
4138+
}
4139+
else
4140+
SET_LOCKTAG_RELATION(tag, dbid, relid);
4141+
4142+
if (!LockOrStrongerHeldByMe(&tag, ShareUpdateExclusiveLock))
4143+
elog(WARNING,
4144+
"missing lock for relation \"%s\" (OID %u, relkind %c) @ TID (%u,%u)",
4145+
NameStr(classForm->relname),
4146+
relid,
4147+
classForm->relkind,
4148+
ItemPointerGetBlockNumber(&oldtup->t_self),
4149+
ItemPointerGetOffsetNumber(&oldtup->t_self));
4150+
}
4151+
#endif
4152+
40174153
/*
40184154
* Check if the specified attribute's values are the same. Subroutine for
40194155
* HeapDetermineColumnsInfo.
@@ -6039,15 +6175,21 @@ heap_inplace_lock(Relation relation,
60396175
TM_Result result;
60406176
bool ret;
60416177

6178+
#ifdef USE_ASSERT_CHECKING
6179+
if (RelationGetRelid(relation) == RelationRelationId)
6180+
check_inplace_rel_lock(oldtup_ptr);
6181+
#endif
6182+
60426183
Assert(BufferIsValid(buffer));
60436184

6185+
LockTuple(relation, &oldtup.t_self, InplaceUpdateTupleLock);
60446186
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
60456187

60466188
/*----------
60476189
* Interpret HeapTupleSatisfiesUpdate() like heap_update() does, except:
60486190
*
60496191
* - wait unconditionally
6050-
* - no tuple locks
6192+
* - already locked tuple above, since inplace needs that unconditionally
60516193
* - don't recheck header after wait: simpler to defer to next iteration
60526194
* - don't try to continue even if the updater aborts: likewise
60536195
* - no crosscheck
@@ -6131,7 +6273,10 @@ heap_inplace_lock(Relation relation,
61316273
* don't bother optimizing that.
61326274
*/
61336275
if (!ret)
6276+
{
6277+
UnlockTuple(relation, &oldtup.t_self, InplaceUpdateTupleLock);
61346278
InvalidateCatalogSnapshot();
6279+
}
61356280
return ret;
61366281
}
61376282

@@ -6140,6 +6285,8 @@ heap_inplace_lock(Relation relation,
61406285
*
61416286
* The tuple cannot change size, and therefore its header fields and null
61426287
* bitmap (if any) don't change either.
6288+
*
6289+
* Since we hold LOCKTAG_TUPLE, no updater has a local copy of this tuple.
61436290
*/
61446291
void
61456292
heap_inplace_update_and_unlock(Relation relation,
@@ -6223,6 +6370,7 @@ heap_inplace_unlock(Relation relation,
62236370
HeapTuple oldtup, Buffer buffer)
62246371
{
62256372
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
6373+
UnlockTuple(relation, &oldtup->t_self, InplaceUpdateTupleLock);
62266374
}
62276375

62286376
/*

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

+3-1
Original file line numberDiff line numberDiff line change
@@ -755,7 +755,9 @@ systable_endscan_ordered(SysScanDesc sysscan)
755755
*
756756
* Overwriting violates both MVCC and transactional safety, so the uses of
757757
* this function in Postgres are extremely limited. Nonetheless we find some
758-
* places to use it. Standard flow:
758+
* places to use it. See README.tuplock section "Locking to write
759+
* inplace-updated tables" and later sections for expectations of readers and
760+
* writers of a table that gets inplace updates. Standard flow:
759761
*
760762
* ... [any slow preparation not requiring oldtup] ...
761763
* systable_inplace_update_begin([...], &tup, &inplace_state);

‎src/backend/catalog/aclchk.c

+7-2
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@
7070
#include "nodes/makefuncs.h"
7171
#include "parser/parse_func.h"
7272
#include "parser/parse_type.h"
73+
#include "storage/lmgr.h"
7374
#include "utils/acl.h"
7475
#include "utils/aclchk_internal.h"
7576
#include "utils/builtins.h"
@@ -1822,7 +1823,7 @@ ExecGrant_Relation(InternalGrant *istmt)
18221823
HeapTuple tuple;
18231824
ListCell *cell_colprivs;
18241825

1825-
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relOid));
1826+
tuple = SearchSysCacheLocked1(RELOID, ObjectIdGetDatum(relOid));
18261827
if (!HeapTupleIsValid(tuple))
18271828
elog(ERROR, "cache lookup failed for relation %u", relOid);
18281829
pg_class_tuple = (Form_pg_class) GETSTRUCT(tuple);
@@ -2038,6 +2039,7 @@ ExecGrant_Relation(InternalGrant *istmt)
20382039
values, nulls, replaces);
20392040

20402041
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
2042+
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
20412043

20422044
/* Update initial privileges for extensions */
20432045
recordExtensionInitPriv(relOid, RelationRelationId, 0, new_acl);
@@ -2050,6 +2052,8 @@ ExecGrant_Relation(InternalGrant *istmt)
20502052

20512053
pfree(new_acl);
20522054
}
2055+
else
2056+
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
20532057

20542058
/*
20552059
* Handle column-level privileges, if any were specified or implied.
@@ -2159,7 +2163,7 @@ ExecGrant_Database(InternalGrant *istmt)
21592163
Oid *newmembers;
21602164
HeapTuple tuple;
21612165

2162-
tuple = SearchSysCache1(DATABASEOID, ObjectIdGetDatum(datId));
2166+
tuple = SearchSysCacheLocked1(DATABASEOID, ObjectIdGetDatum(datId));
21632167
if (!HeapTupleIsValid(tuple))
21642168
elog(ERROR, "cache lookup failed for database %u", datId);
21652169

@@ -2228,6 +2232,7 @@ ExecGrant_Database(InternalGrant *istmt)
22282232
nulls, replaces);
22292233

22302234
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
2235+
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
22312236

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

‎src/backend/catalog/catalog.c

+9
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,15 @@ IsCatalogRelationOid(Oid relid)
140140
/*
141141
* IsInplaceUpdateRelation
142142
* True iff core code performs inplace updates on the relation.
143+
*
144+
* This is used for assertions and for making the executor follow the
145+
* locking protocol described at README.tuplock section "Locking to write
146+
* inplace-updated tables". Extensions may inplace-update other heap
147+
* tables, but concurrent SQL UPDATE on the same table may overwrite
148+
* those modifications.
149+
*
150+
* The executor can assume these are not partitions or partitioned and
151+
* have no triggers.
143152
*/
144153
bool
145154
IsInplaceUpdateRelation(Relation relation)

0 commit comments

Comments
 (0)