Skip to content

Commit 3b7a689

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 fd27b87 commit 3b7a689

File tree

20 files changed

+498
-57
lines changed

20 files changed

+498
-57
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 "commands/vacuum.h"
5557
#include "miscadmin.h"
5658
#include "pgstat.h"
@@ -75,6 +77,12 @@ static XLogRecPtr log_heap_update(Relation reln, Buffer oldbuf,
7577
Buffer newbuf, HeapTuple oldtup,
7678
HeapTuple newtup, HeapTuple old_key_tuple,
7779
bool all_visible_cleared, bool new_all_visible_cleared);
80+
#ifdef USE_ASSERT_CHECKING
81+
static void check_lock_if_inplace_updateable_rel(Relation relation,
82+
ItemPointer otid,
83+
HeapTuple newtup);
84+
static void check_inplace_rel_lock(HeapTuple oldtup);
85+
#endif
7886
static Bitmapset *HeapDetermineColumnsInfo(Relation relation,
7987
Bitmapset *interesting_cols,
8088
Bitmapset *external_cols,
@@ -121,6 +129,8 @@ static HeapTuple ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool ke
121129
* heavyweight lock mode and MultiXactStatus values to use for any particular
122130
* tuple lock strength.
123131
*
132+
* These interact with InplaceUpdateTupleLock, an alias for ExclusiveLock.
133+
*
124134
* Don't look at lockstatus/updstatus directly! Use get_mxact_status_for_lock
125135
* instead.
126136
*/
@@ -3207,6 +3217,10 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
32073217
(errcode(ERRCODE_INVALID_TRANSACTION_STATE),
32083218
errmsg("cannot update tuples during a parallel operation")));
32093219

3220+
#ifdef USE_ASSERT_CHECKING
3221+
check_lock_if_inplace_updateable_rel(relation, otid, newtup);
3222+
#endif
3223+
32103224
/*
32113225
* Fetch the list of attributes to be checked for various operations.
32123226
*
@@ -4071,6 +4085,128 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
40714085
return TM_Ok;
40724086
}
40734087

4088+
#ifdef USE_ASSERT_CHECKING
4089+
/*
4090+
* Confirm adequate lock held during heap_update(), per rules from
4091+
* README.tuplock section "Locking to write inplace-updated tables".
4092+
*/
4093+
static void
4094+
check_lock_if_inplace_updateable_rel(Relation relation,
4095+
ItemPointer otid,
4096+
HeapTuple newtup)
4097+
{
4098+
/* LOCKTAG_TUPLE acceptable for any catalog */
4099+
switch (RelationGetRelid(relation))
4100+
{
4101+
case RelationRelationId:
4102+
case DatabaseRelationId:
4103+
{
4104+
LOCKTAG tuptag;
4105+
4106+
SET_LOCKTAG_TUPLE(tuptag,
4107+
relation->rd_lockInfo.lockRelId.dbId,
4108+
relation->rd_lockInfo.lockRelId.relId,
4109+
ItemPointerGetBlockNumber(otid),
4110+
ItemPointerGetOffsetNumber(otid));
4111+
if (LockHeldByMe(&tuptag, InplaceUpdateTupleLock, false))
4112+
return;
4113+
}
4114+
break;
4115+
default:
4116+
Assert(!IsInplaceUpdateRelation(relation));
4117+
return;
4118+
}
4119+
4120+
switch (RelationGetRelid(relation))
4121+
{
4122+
case RelationRelationId:
4123+
{
4124+
/* LOCKTAG_TUPLE or LOCKTAG_RELATION ok */
4125+
Form_pg_class classForm = (Form_pg_class) GETSTRUCT(newtup);
4126+
Oid relid = classForm->oid;
4127+
Oid dbid;
4128+
LOCKTAG tag;
4129+
4130+
if (IsSharedRelation(relid))
4131+
dbid = InvalidOid;
4132+
else
4133+
dbid = MyDatabaseId;
4134+
4135+
if (classForm->relkind == RELKIND_INDEX)
4136+
{
4137+
Relation irel = index_open(relid, AccessShareLock);
4138+
4139+
SET_LOCKTAG_RELATION(tag, dbid, irel->rd_index->indrelid);
4140+
index_close(irel, AccessShareLock);
4141+
}
4142+
else
4143+
SET_LOCKTAG_RELATION(tag, dbid, relid);
4144+
4145+
if (!LockHeldByMe(&tag, ShareUpdateExclusiveLock, false) &&
4146+
!LockHeldByMe(&tag, ShareRowExclusiveLock, true))
4147+
elog(WARNING,
4148+
"missing lock for relation \"%s\" (OID %u, relkind %c) @ TID (%u,%u)",
4149+
NameStr(classForm->relname),
4150+
relid,
4151+
classForm->relkind,
4152+
ItemPointerGetBlockNumber(otid),
4153+
ItemPointerGetOffsetNumber(otid));
4154+
}
4155+
break;
4156+
case DatabaseRelationId:
4157+
{
4158+
/* LOCKTAG_TUPLE required */
4159+
Form_pg_database dbForm = (Form_pg_database) GETSTRUCT(newtup);
4160+
4161+
elog(WARNING,
4162+
"missing lock on database \"%s\" (OID %u) @ TID (%u,%u)",
4163+
NameStr(dbForm->datname),
4164+
dbForm->oid,
4165+
ItemPointerGetBlockNumber(otid),
4166+
ItemPointerGetOffsetNumber(otid));
4167+
}
4168+
break;
4169+
}
4170+
}
4171+
4172+
/*
4173+
* Confirm adequate relation lock held, per rules from README.tuplock section
4174+
* "Locking to write inplace-updated tables".
4175+
*/
4176+
static void
4177+
check_inplace_rel_lock(HeapTuple oldtup)
4178+
{
4179+
Form_pg_class classForm = (Form_pg_class) GETSTRUCT(oldtup);
4180+
Oid relid = classForm->oid;
4181+
Oid dbid;
4182+
LOCKTAG tag;
4183+
4184+
if (IsSharedRelation(relid))
4185+
dbid = InvalidOid;
4186+
else
4187+
dbid = MyDatabaseId;
4188+
4189+
if (classForm->relkind == RELKIND_INDEX)
4190+
{
4191+
Relation irel = index_open(relid, AccessShareLock);
4192+
4193+
SET_LOCKTAG_RELATION(tag, dbid, irel->rd_index->indrelid);
4194+
index_close(irel, AccessShareLock);
4195+
}
4196+
else
4197+
SET_LOCKTAG_RELATION(tag, dbid, relid);
4198+
4199+
if (!LockHeldByMe(&tag, ShareUpdateExclusiveLock, true))
4200+
elog(WARNING,
4201+
"missing lock for relation \"%s\" (OID %u, relkind %c) @ TID (%u,%u)",
4202+
NameStr(classForm->relname),
4203+
relid,
4204+
classForm->relkind,
4205+
ItemPointerGetBlockNumber(&oldtup->t_self),
4206+
ItemPointerGetOffsetNumber(&oldtup->t_self));
4207+
}
4208+
#endif
4209+
40744210
/*
40754211
* Check if the specified attribute's values are the same. Subroutine for
40764212
* HeapDetermineColumnsInfo.
@@ -6088,15 +6224,21 @@ heap_inplace_lock(Relation relation,
60886224
TM_Result result;
60896225
bool ret;
60906226

6227+
#ifdef USE_ASSERT_CHECKING
6228+
if (RelationGetRelid(relation) == RelationRelationId)
6229+
check_inplace_rel_lock(oldtup_ptr);
6230+
#endif
6231+
60916232
Assert(BufferIsValid(buffer));
60926233

6234+
LockTuple(relation, &oldtup.t_self, InplaceUpdateTupleLock);
60936235
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
60946236

60956237
/*----------
60966238
* Interpret HeapTupleSatisfiesUpdate() like heap_update() does, except:
60976239
*
60986240
* - wait unconditionally
6099-
* - no tuple locks
6241+
* - already locked tuple above, since inplace needs that unconditionally
61006242
* - don't recheck header after wait: simpler to defer to next iteration
61016243
* - don't try to continue even if the updater aborts: likewise
61026244
* - no crosscheck
@@ -6180,7 +6322,10 @@ heap_inplace_lock(Relation relation,
61806322
* don't bother optimizing that.
61816323
*/
61826324
if (!ret)
6325+
{
6326+
UnlockTuple(relation, &oldtup.t_self, InplaceUpdateTupleLock);
61836327
InvalidateCatalogSnapshot();
6328+
}
61846329
return ret;
61856330
}
61866331

@@ -6189,6 +6334,8 @@ heap_inplace_lock(Relation relation,
61896334
*
61906335
* The tuple cannot change size, and therefore its header fields and null
61916336
* bitmap (if any) don't change either.
6337+
*
6338+
* Since we hold LOCKTAG_TUPLE, no updater has a local copy of this tuple.
61926339
*/
61936340
void
61946341
heap_inplace_update_and_unlock(Relation relation,
@@ -6272,6 +6419,7 @@ heap_inplace_unlock(Relation relation,
62726419
HeapTuple oldtup, Buffer buffer)
62736420
{
62746421
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
6422+
UnlockTuple(relation, &oldtup->t_self, InplaceUpdateTupleLock);
62756423
}
62766424

62776425
/*

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -754,7 +754,9 @@ systable_endscan_ordered(SysScanDesc sysscan)
754754
*
755755
* Overwriting violates both MVCC and transactional safety, so the uses of
756756
* this function in Postgres are extremely limited. Nonetheless we find some
757-
* places to use it. Standard flow:
757+
* places to use it. See README.tuplock section "Locking to write
758+
* inplace-updated tables" and later sections for expectations of readers and
759+
* writers of a table that gets inplace updates. Standard flow:
758760
*
759761
* ... [any slow preparation not requiring oldtup] ...
760762
* 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
@@ -75,6 +75,7 @@
7575
#include "nodes/makefuncs.h"
7676
#include "parser/parse_func.h"
7777
#include "parser/parse_type.h"
78+
#include "storage/lmgr.h"
7879
#include "utils/acl.h"
7980
#include "utils/aclchk_internal.h"
8081
#include "utils/builtins.h"
@@ -1848,7 +1849,7 @@ ExecGrant_Relation(InternalGrant *istmt)
18481849
HeapTuple tuple;
18491850
ListCell *cell_colprivs;
18501851

1851-
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relOid));
1852+
tuple = SearchSysCacheLocked1(RELOID, ObjectIdGetDatum(relOid));
18521853
if (!HeapTupleIsValid(tuple))
18531854
elog(ERROR, "cache lookup failed for relation %u", relOid);
18541855
pg_class_tuple = (Form_pg_class) GETSTRUCT(tuple);
@@ -2060,6 +2061,7 @@ ExecGrant_Relation(InternalGrant *istmt)
20602061
values, nulls, replaces);
20612062

20622063
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
2064+
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
20632065

20642066
/* Update initial privileges for extensions */
20652067
recordExtensionInitPriv(relOid, RelationRelationId, 0, new_acl);
@@ -2072,6 +2074,8 @@ ExecGrant_Relation(InternalGrant *istmt)
20722074

20732075
pfree(new_acl);
20742076
}
2077+
else
2078+
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
20752079

20762080
/*
20772081
* Handle column-level privileges, if any were specified or implied.
@@ -2185,7 +2189,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs,
21852189
Oid *oldmembers;
21862190
Oid *newmembers;
21872191

2188-
tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objectid));
2192+
tuple = SearchSysCacheLocked1(cacheid, ObjectIdGetDatum(objectid));
21892193
if (!HeapTupleIsValid(tuple))
21902194
elog(ERROR, "cache lookup failed for %s %u", get_object_class_descr(classid), objectid);
21912195

@@ -2261,6 +2265,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs,
22612265
nulls, replaces);
22622266

22632267
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
2268+
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
22642269

22652270
/* Update initial privileges for extensions */
22662271
recordExtensionInitPriv(objectid, classid, 0, new_acl);

‎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)