Skip to content

Commit 4cf948c

Browse files
nmischnmisch
committed
For inplace update, send nontransactional invalidations.
The inplace update survives ROLLBACK. The inval didn't, so another backend's DDL could then update the row without incorporating the inplace update. In the test this fixes, a mix of CREATE INDEX and ALTER TABLE resulted in a table with an index, yet relhasindex=f. That is a source of index corruption. Back-patch to v12 (all supported versions). The back branch versions don't change WAL, because those branches just added end-of-recovery SIResetAll(). All branches change the ABI of extern function PrepareToInvalidateCacheTuple(). No PGXN extension calls that, and there's no apparent use case in extensions. Reviewed by Nitin Motiani and (in earlier versions) Andres Freund. Discussion: https://postgr.es/m/20240523000548.58.nmisch@google.com
1 parent da99504 commit 4cf948c

File tree

11 files changed

+289
-117
lines changed

11 files changed

+289
-117
lines changed

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

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6142,6 +6142,24 @@ heap_inplace_update_and_unlock(Relation relation,
61426142
if (oldlen != newlen || htup->t_hoff != tuple->t_data->t_hoff)
61436143
elog(ERROR, "wrong tuple length");
61446144

6145+
/*
6146+
* Construct shared cache inval if necessary. Note that because we only
6147+
* pass the new version of the tuple, this mustn't be used for any
6148+
* operations that could change catcache lookup keys. But we aren't
6149+
* bothering with index updates either, so that's true a fortiori.
6150+
*/
6151+
CacheInvalidateHeapTupleInplace(relation, tuple, NULL);
6152+
6153+
/*
6154+
* Unlink relcache init files as needed. If unlinking, acquire
6155+
* RelCacheInitLock until after associated invalidations. By doing this
6156+
* in advance, if we checkpoint and then crash between inplace
6157+
* XLogInsert() and inval, we don't rely on StartupXLOG() ->
6158+
* RelationCacheInitFileRemove(). That uses elevel==LOG, so replay would
6159+
* neglect to PANIC on EIO.
6160+
*/
6161+
PreInplace_Inval();
6162+
61456163
/* NO EREPORT(ERROR) from here till changes are logged */
61466164
START_CRIT_SECTION();
61476165

@@ -6185,17 +6203,28 @@ heap_inplace_update_and_unlock(Relation relation,
61856203
PageSetLSN(BufferGetPage(buffer), recptr);
61866204
}
61876205

6206+
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
6207+
6208+
/*
6209+
* Send invalidations to shared queue. SearchSysCacheLocked1() assumes we
6210+
* do this before UnlockTuple().
6211+
*
6212+
* If we're mutating a tuple visible only to this transaction, there's an
6213+
* equivalent transactional inval from the action that created the tuple,
6214+
* and this inval is superfluous.
6215+
*/
6216+
AtInplace_Inval();
6217+
61886218
END_CRIT_SECTION();
6219+
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
61896220

6190-
heap_inplace_unlock(relation, oldtup, buffer);
6221+
AcceptInvalidationMessages(); /* local processing of just-sent inval */
61916222

61926223
/*
6193-
* Send out shared cache inval if necessary. Note that because we only
6194-
* pass the new version of the tuple, this mustn't be used for any
6195-
* operations that could change catcache lookup keys. But we aren't
6196-
* bothering with index updates either, so that's true a fortiori.
6197-
*
6198-
* XXX ROLLBACK discards the invalidation. See test inplace-inval.spec.
6224+
* Queue a transactional inval. The immediate invalidation we just sent
6225+
* is the only one known to be necessary. To reduce risk from the
6226+
* transition to immediate invalidation, continue sending a transactional
6227+
* invalidation like we've long done. Third-party code might rely on it.
61996228
*/
62006229
if (!IsBootstrapProcessingMode())
62016230
CacheInvalidateHeapTuple(relation, tuple, NULL);

‎src/backend/access/transam/xact.c

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1247,14 +1247,24 @@ RecordTransactionCommit(void)
12471247

12481248
/*
12491249
* Transactions without an assigned xid can contain invalidation
1250-
* messages (e.g. explicit relcache invalidations or catcache
1251-
* invalidations for inplace updates); standbys need to process those.
1252-
* We can't emit a commit record without an xid, and we don't want to
1253-
* force assigning an xid, because that'd be problematic for e.g.
1254-
* vacuum. Hence we emit a bespoke record for the invalidations. We
1255-
* don't want to use that in case a commit record is emitted, so they
1256-
* happen synchronously with commits (besides not wanting to emit more
1257-
* WAL records).
1250+
* messages. While inplace updates do this, this is not known to be
1251+
* necessary; see comment at inplace CacheInvalidateHeapTuple().
1252+
* Extensions might still rely on this capability, and standbys may
1253+
* need to process those invals. We can't emit a commit record
1254+
* without an xid, and we don't want to force assigning an xid,
1255+
* because that'd be problematic for e.g. vacuum. Hence we emit a
1256+
* bespoke record for the invalidations. We don't want to use that in
1257+
* case a commit record is emitted, so they happen synchronously with
1258+
* commits (besides not wanting to emit more WAL records).
1259+
*
1260+
* XXX Every known use of this capability is a defect. Since an XID
1261+
* isn't controlling visibility of the change that prompted invals,
1262+
* other sessions need the inval even if this transactions aborts.
1263+
*
1264+
* ON COMMIT DELETE ROWS does a nontransactional index_build(), which
1265+
* queues a relcache inval, including in transactions without an xid
1266+
* that had read the (empty) table. Standbys don't need any ON COMMIT
1267+
* DELETE ROWS invals, but we've not done the work to withhold them.
12581268
*/
12591269
if (nmsgs != 0)
12601270
{

‎src/backend/catalog/index.c

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2881,12 +2881,19 @@ index_update_stats(Relation rel,
28812881
if (dirty)
28822882
{
28832883
systable_inplace_update_finish(state, tuple);
2884-
/* the above sends a cache inval message */
2884+
/* the above sends transactional and immediate cache inval messages */
28852885
}
28862886
else
28872887
{
28882888
systable_inplace_update_cancel(state);
2889-
/* no need to change tuple, but force relcache inval anyway */
2889+
2890+
/*
2891+
* While we didn't change relhasindex, CREATE INDEX needs a
2892+
* transactional inval for when the new index's catalog rows become
2893+
* visible. Other CREATE INDEX and REINDEX code happens to also queue
2894+
* this inval, but keep this in case rare callers rely on this part of
2895+
* our API contract.
2896+
*/
28902897
CacheInvalidateRelcacheByTuple(tuple);
28912898
}
28922899

‎src/backend/replication/logical/decode.c

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -463,23 +463,19 @@ DecodeHeapOp(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
463463

464464
/*
465465
* Inplace updates are only ever performed on catalog tuples and
466-
* can, per definition, not change tuple visibility. Since we
467-
* don't decode catalog tuples, we're not interested in the
468-
* record's contents.
466+
* can, per definition, not change tuple visibility. Inplace
467+
* updates don't affect storage or interpretation of table rows,
468+
* so they don't affect logicalrep_write_tuple() outcomes. Hence,
469+
* we don't process invalidations from the original operation. If
470+
* inplace updates did affect those things, invalidations wouldn't
471+
* make it work, since there are no snapshot-specific versions of
472+
* inplace-updated values. Since we also don't decode catalog
473+
* tuples, we're not interested in the record's contents.
469474
*
470-
* In-place updates can be used either by XID-bearing transactions
471-
* (e.g. in CREATE INDEX CONCURRENTLY) or by XID-less
472-
* transactions (e.g. VACUUM). In the former case, the commit
473-
* record will include cache invalidations, so we mark the
474-
* transaction as catalog modifying here. Currently that's
475-
* redundant because the commit will do that as well, but once we
476-
* support decoding in-progress relations, this will be important.
475+
* WAL contains likely-unnecessary commit-time invals from the
476+
* CacheInvalidateHeapTuple() call in heap_inplace_update().
477+
* Excess invalidation is safe.
477478
*/
478-
if (!TransactionIdIsValid(xid))
479-
break;
480-
481-
SnapBuildProcessChange(builder, xid, buf->origptr);
482-
ReorderBufferXidSetCatalogChanges(ctx->reorder, xid, buf->origptr);
483479
break;
484480

485481
case XLOG_HEAP_CONFIRM:

‎src/backend/utils/cache/catcache.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2128,7 +2128,8 @@ void
21282128
PrepareToInvalidateCacheTuple(Relation relation,
21292129
HeapTuple tuple,
21302130
HeapTuple newtuple,
2131-
void (*function) (int, uint32, Oid))
2131+
void (*function) (int, uint32, Oid, void *),
2132+
void *context)
21322133
{
21332134
slist_iter iter;
21342135
Oid reloid;
@@ -2169,7 +2170,7 @@ PrepareToInvalidateCacheTuple(Relation relation,
21692170
hashvalue = CatalogCacheComputeTupleHashValue(ccp, ccp->cc_nkeys, tuple);
21702171
dbid = ccp->cc_relisshared ? (Oid) 0 : MyDatabaseId;
21712172

2172-
(*function) (ccp->id, hashvalue, dbid);
2173+
(*function) (ccp->id, hashvalue, dbid, context);
21732174

21742175
if (newtuple)
21752176
{
@@ -2178,7 +2179,7 @@ PrepareToInvalidateCacheTuple(Relation relation,
21782179
newhashvalue = CatalogCacheComputeTupleHashValue(ccp, ccp->cc_nkeys, newtuple);
21792180

21802181
if (newhashvalue != hashvalue)
2181-
(*function) (ccp->id, newhashvalue, dbid);
2182+
(*function) (ccp->id, newhashvalue, dbid, context);
21822183
}
21832184
}
21842185
}

0 commit comments

Comments
 (0)