Skip to content

Commit f51b34b

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 82c2d9e commit f51b34b

File tree

19 files changed

+437
-34
lines changed

19 files changed

+437
-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
@@ -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
*/
@@ -3250,6 +3260,10 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
32503260
(errcode(ERRCODE_INVALID_TRANSACTION_STATE),
32513261
errmsg("cannot update tuples during a parallel operation")));
32523262

3263+
#ifdef USE_ASSERT_CHECKING
3264+
check_lock_if_inplace_updateable_rel(relation, otid, newtup);
3265+
#endif
3266+
32533267
/*
32543268
* Fetch the list of attributes to be checked for various operations.
32553269
*
@@ -4095,6 +4109,128 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
40954109
return TM_Ok;
40964110
}
40974111

4112+
#ifdef USE_ASSERT_CHECKING
4113+
/*
4114+
* Confirm adequate lock held during heap_update(), per rules from
4115+
* README.tuplock section "Locking to write inplace-updated tables".
4116+
*/
4117+
static void
4118+
check_lock_if_inplace_updateable_rel(Relation relation,
4119+
ItemPointer otid,
4120+
HeapTuple newtup)
4121+
{
4122+
/* LOCKTAG_TUPLE acceptable for any catalog */
4123+
switch (RelationGetRelid(relation))
4124+
{
4125+
case RelationRelationId:
4126+
case DatabaseRelationId:
4127+
{
4128+
LOCKTAG tuptag;
4129+
4130+
SET_LOCKTAG_TUPLE(tuptag,
4131+
relation->rd_lockInfo.lockRelId.dbId,
4132+
relation->rd_lockInfo.lockRelId.relId,
4133+
ItemPointerGetBlockNumber(otid),
4134+
ItemPointerGetOffsetNumber(otid));
4135+
if (LockHeldByMe(&tuptag, InplaceUpdateTupleLock))
4136+
return;
4137+
}
4138+
break;
4139+
default:
4140+
Assert(!IsInplaceUpdateRelation(relation));
4141+
return;
4142+
}
4143+
4144+
switch (RelationGetRelid(relation))
4145+
{
4146+
case RelationRelationId:
4147+
{
4148+
/* LOCKTAG_TUPLE or LOCKTAG_RELATION ok */
4149+
Form_pg_class classForm = (Form_pg_class) GETSTRUCT(newtup);
4150+
Oid relid = classForm->oid;
4151+
Oid dbid;
4152+
LOCKTAG tag;
4153+
4154+
if (IsSharedRelation(relid))
4155+
dbid = InvalidOid;
4156+
else
4157+
dbid = MyDatabaseId;
4158+
4159+
if (classForm->relkind == RELKIND_INDEX)
4160+
{
4161+
Relation irel = index_open(relid, AccessShareLock);
4162+
4163+
SET_LOCKTAG_RELATION(tag, dbid, irel->rd_index->indrelid);
4164+
index_close(irel, AccessShareLock);
4165+
}
4166+
else
4167+
SET_LOCKTAG_RELATION(tag, dbid, relid);
4168+
4169+
if (!LockHeldByMe(&tag, ShareUpdateExclusiveLock) &&
4170+
!LockOrStrongerHeldByMe(&tag, ShareRowExclusiveLock))
4171+
elog(WARNING,
4172+
"missing lock for relation \"%s\" (OID %u, relkind %c) @ TID (%u,%u)",
4173+
NameStr(classForm->relname),
4174+
relid,
4175+
classForm->relkind,
4176+
ItemPointerGetBlockNumber(otid),
4177+
ItemPointerGetOffsetNumber(otid));
4178+
}
4179+
break;
4180+
case DatabaseRelationId:
4181+
{
4182+
/* LOCKTAG_TUPLE required */
4183+
Form_pg_database dbForm = (Form_pg_database) GETSTRUCT(newtup);
4184+
4185+
elog(WARNING,
4186+
"missing lock on database \"%s\" (OID %u) @ TID (%u,%u)",
4187+
NameStr(dbForm->datname),
4188+
dbForm->oid,
4189+
ItemPointerGetBlockNumber(otid),
4190+
ItemPointerGetOffsetNumber(otid));
4191+
}
4192+
break;
4193+
}
4194+
}
4195+
4196+
/*
4197+
* Confirm adequate relation lock held, per rules from README.tuplock section
4198+
* "Locking to write inplace-updated tables".
4199+
*/
4200+
static void
4201+
check_inplace_rel_lock(HeapTuple oldtup)
4202+
{
4203+
Form_pg_class classForm = (Form_pg_class) GETSTRUCT(oldtup);
4204+
Oid relid = classForm->oid;
4205+
Oid dbid;
4206+
LOCKTAG tag;
4207+
4208+
if (IsSharedRelation(relid))
4209+
dbid = InvalidOid;
4210+
else
4211+
dbid = MyDatabaseId;
4212+
4213+
if (classForm->relkind == RELKIND_INDEX)
4214+
{
4215+
Relation irel = index_open(relid, AccessShareLock);
4216+
4217+
SET_LOCKTAG_RELATION(tag, dbid, irel->rd_index->indrelid);
4218+
index_close(irel, AccessShareLock);
4219+
}
4220+
else
4221+
SET_LOCKTAG_RELATION(tag, dbid, relid);
4222+
4223+
if (!LockOrStrongerHeldByMe(&tag, ShareUpdateExclusiveLock))
4224+
elog(WARNING,
4225+
"missing lock for relation \"%s\" (OID %u, relkind %c) @ TID (%u,%u)",
4226+
NameStr(classForm->relname),
4227+
relid,
4228+
classForm->relkind,
4229+
ItemPointerGetBlockNumber(&oldtup->t_self),
4230+
ItemPointerGetOffsetNumber(&oldtup->t_self));
4231+
}
4232+
#endif
4233+
40984234
/*
40994235
* Check if the specified attribute's values are the same. Subroutine for
41004236
* HeapDetermineColumnsInfo.
@@ -6120,15 +6256,21 @@ heap_inplace_lock(Relation relation,
61206256
TM_Result result;
61216257
bool ret;
61226258

6259+
#ifdef USE_ASSERT_CHECKING
6260+
if (RelationGetRelid(relation) == RelationRelationId)
6261+
check_inplace_rel_lock(oldtup_ptr);
6262+
#endif
6263+
61236264
Assert(BufferIsValid(buffer));
61246265

6266+
LockTuple(relation, &oldtup.t_self, InplaceUpdateTupleLock);
61256267
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
61266268

61276269
/*----------
61286270
* Interpret HeapTupleSatisfiesUpdate() like heap_update() does, except:
61296271
*
61306272
* - wait unconditionally
6131-
* - no tuple locks
6273+
* - already locked tuple above, since inplace needs that unconditionally
61326274
* - don't recheck header after wait: simpler to defer to next iteration
61336275
* - don't try to continue even if the updater aborts: likewise
61346276
* - no crosscheck
@@ -6212,7 +6354,10 @@ heap_inplace_lock(Relation relation,
62126354
* don't bother optimizing that.
62136355
*/
62146356
if (!ret)
6357+
{
6358+
UnlockTuple(relation, &oldtup.t_self, InplaceUpdateTupleLock);
62156359
InvalidateCatalogSnapshot();
6360+
}
62166361
return ret;
62176362
}
62186363

@@ -6221,6 +6366,8 @@ heap_inplace_lock(Relation relation,
62216366
*
62226367
* The tuple cannot change size, and therefore its header fields and null
62236368
* bitmap (if any) don't change either.
6369+
*
6370+
* Since we hold LOCKTAG_TUPLE, no updater has a local copy of this tuple.
62246371
*/
62256372
void
62266373
heap_inplace_update_and_unlock(Relation relation,
@@ -6304,6 +6451,7 @@ heap_inplace_unlock(Relation relation,
63046451
HeapTuple oldtup, Buffer buffer)
63056452
{
63066453
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
6454+
UnlockTuple(relation, &oldtup->t_self, InplaceUpdateTupleLock);
63076455
}
63086456

63096457
/*

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -752,7 +752,9 @@ systable_endscan_ordered(SysScanDesc sysscan)
752752
*
753753
* Overwriting violates both MVCC and transactional safety, so the uses of
754754
* this function in Postgres are extremely limited. Nonetheless we find some
755-
* places to use it. Standard flow:
755+
* places to use it. See README.tuplock section "Locking to write
756+
* inplace-updated tables" and later sections for expectations of readers and
757+
* writers of a table that gets inplace updates. Standard flow:
756758
*
757759
* ... [any slow preparation not requiring oldtup] ...
758760
* 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"
@@ -1779,7 +1780,7 @@ ExecGrant_Relation(InternalGrant *istmt)
17791780
HeapTuple tuple;
17801781
ListCell *cell_colprivs;
17811782

1782-
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relOid));
1783+
tuple = SearchSysCacheLocked1(RELOID, ObjectIdGetDatum(relOid));
17831784
if (!HeapTupleIsValid(tuple))
17841785
elog(ERROR, "cache lookup failed for relation %u", relOid);
17851786
pg_class_tuple = (Form_pg_class) GETSTRUCT(tuple);
@@ -1995,6 +1996,7 @@ ExecGrant_Relation(InternalGrant *istmt)
19951996
values, nulls, replaces);
19961997

19971998
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
1999+
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
19982000

19992001
/* Update initial privileges for extensions */
20002002
recordExtensionInitPriv(relOid, RelationRelationId, 0, new_acl);
@@ -2007,6 +2009,8 @@ ExecGrant_Relation(InternalGrant *istmt)
20072009

20082010
pfree(new_acl);
20092011
}
2012+
else
2013+
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
20102014

20112015
/*
20122016
* Handle column-level privileges, if any were specified or implied.
@@ -2116,7 +2120,7 @@ ExecGrant_Database(InternalGrant *istmt)
21162120
Oid *newmembers;
21172121
HeapTuple tuple;
21182122

2119-
tuple = SearchSysCache1(DATABASEOID, ObjectIdGetDatum(datId));
2123+
tuple = SearchSysCacheLocked1(DATABASEOID, ObjectIdGetDatum(datId));
21202124
if (!HeapTupleIsValid(tuple))
21212125
elog(ERROR, "cache lookup failed for database %u", datId);
21222126

@@ -2185,6 +2189,7 @@ ExecGrant_Database(InternalGrant *istmt)
21852189
nulls, replaces);
21862190

21872191
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
2192+
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
21882193

21892194
/* Update the shared dependency ACL info */
21902195
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
@@ -138,6 +138,15 @@ IsCatalogRelationOid(Oid relid)
138138
/*
139139
* IsInplaceUpdateRelation
140140
* True iff core code performs inplace updates on the relation.
141+
*
142+
* This is used for assertions and for making the executor follow the
143+
* locking protocol described at README.tuplock section "Locking to write
144+
* inplace-updated tables". Extensions may inplace-update other heap
145+
* tables, but concurrent SQL UPDATE on the same table may overwrite
146+
* those modifications.
147+
*
148+
* The executor can assume these are not partitions or partitioned and
149+
* have no triggers.
141150
*/
142151
bool
143152
IsInplaceUpdateRelation(Relation relation)

0 commit comments

Comments
 (0)