Skip to content

Commit c1099dd

Browse files
nmischnmisch
committed
Revert "For inplace update, send nontransactional invalidations."
This reverts commit 95c5acb (v17) and counterparts in each other non-master branch. If released, that commit would have caused a worst-in-years minor release regression, via undetected LWLock self-deadlock. This commit and its self-deadlock fix warrant more bake time in the master branch. Reported by Alexander Lakhin. Discussion: https://postgr.es/m/10ec0bc3-5933-1189-6bb8-5dec4114558e@gmail.com
1 parent bc6bad8 commit c1099dd

File tree

13 files changed

+137
-320
lines changed

13 files changed

+137
-320
lines changed

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

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

6358-
/*
6359-
* Construct shared cache inval if necessary. Note that because we only
6360-
* pass the new version of the tuple, this mustn't be used for any
6361-
* operations that could change catcache lookup keys. But we aren't
6362-
* bothering with index updates either, so that's true a fortiori.
6363-
*/
6364-
CacheInvalidateHeapTupleInplace(relation, tuple, NULL);
6365-
6366-
/*
6367-
* Unlink relcache init files as needed. If unlinking, acquire
6368-
* RelCacheInitLock until after associated invalidations. By doing this
6369-
* in advance, if we checkpoint and then crash between inplace
6370-
* XLogInsert() and inval, we don't rely on StartupXLOG() ->
6371-
* RelationCacheInitFileRemove(). That uses elevel==LOG, so replay would
6372-
* neglect to PANIC on EIO.
6373-
*/
6374-
PreInplace_Inval();
6375-
63766358
/* NO EREPORT(ERROR) from here till changes are logged */
63776359
START_CRIT_SECTION();
63786360

@@ -6416,28 +6398,17 @@ heap_inplace_update_and_unlock(Relation relation,
64166398
PageSetLSN(BufferGetPage(buffer), recptr);
64176399
}
64186400

6419-
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
6420-
6421-
/*
6422-
* Send invalidations to shared queue. SearchSysCacheLocked1() assumes we
6423-
* do this before UnlockTuple().
6424-
*
6425-
* If we're mutating a tuple visible only to this transaction, there's an
6426-
* equivalent transactional inval from the action that created the tuple,
6427-
* and this inval is superfluous.
6428-
*/
6429-
AtInplace_Inval();
6430-
64316401
END_CRIT_SECTION();
6432-
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
64336402

6434-
AcceptInvalidationMessages(); /* local processing of just-sent inval */
6403+
heap_inplace_unlock(relation, oldtup, buffer);
64356404

64366405
/*
6437-
* Queue a transactional inval. The immediate invalidation we just sent
6438-
* is the only one known to be necessary. To reduce risk from the
6439-
* transition to immediate invalidation, continue sending a transactional
6440-
* invalidation like we've long done. Third-party code might rely on it.
6406+
* Send out shared cache inval if necessary. Note that because we only
6407+
* pass the new version of the tuple, this mustn't be used for any
6408+
* operations that could change catcache lookup keys. But we aren't
6409+
* bothering with index updates either, so that's true a fortiori.
6410+
*
6411+
* XXX ROLLBACK discards the invalidation. See test inplace-inval.spec.
64416412
*/
64426413
if (!IsBootstrapProcessingMode())
64436414
CacheInvalidateHeapTuple(relation, tuple, NULL);

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

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

13591359
/*
13601360
* Transactions without an assigned xid can contain invalidation
1361-
* messages. While inplace updates do this, this is not known to be
1362-
* necessary; see comment at inplace CacheInvalidateHeapTuple().
1363-
* Extensions might still rely on this capability, and standbys may
1364-
* need to process those invals. We can't emit a commit record
1365-
* without an xid, and we don't want to force assigning an xid,
1366-
* because that'd be problematic for e.g. vacuum. Hence we emit a
1367-
* bespoke record for the invalidations. We don't want to use that in
1368-
* case a commit record is emitted, so they happen synchronously with
1369-
* commits (besides not wanting to emit more WAL records).
1370-
*
1371-
* XXX Every known use of this capability is a defect. Since an XID
1372-
* isn't controlling visibility of the change that prompted invals,
1373-
* other sessions need the inval even if this transactions aborts.
1374-
*
1375-
* ON COMMIT DELETE ROWS does a nontransactional index_build(), which
1376-
* queues a relcache inval, including in transactions without an xid
1377-
* that had read the (empty) table. Standbys don't need any ON COMMIT
1378-
* DELETE ROWS invals, but we've not done the work to withhold them.
1361+
* messages (e.g. explicit relcache invalidations or catcache
1362+
* invalidations for inplace updates); standbys need to process those.
1363+
* We can't emit a commit record without an xid, and we don't want to
1364+
* force assigning an xid, because that'd be problematic for e.g.
1365+
* vacuum. Hence we emit a bespoke record for the invalidations. We
1366+
* don't want to use that in case a commit record is emitted, so they
1367+
* happen synchronously with commits (besides not wanting to emit more
1368+
* WAL records).
13791369
*/
13801370
if (nmsgs != 0)
13811371
{

‎src/backend/catalog/index.c

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2889,19 +2889,12 @@ index_update_stats(Relation rel,
28892889
if (dirty)
28902890
{
28912891
systable_inplace_update_finish(state, tuple);
2892-
/* the above sends transactional and immediate cache inval messages */
2892+
/* the above sends a cache inval message */
28932893
}
28942894
else
28952895
{
28962896
systable_inplace_update_cancel(state);
2897-
2898-
/*
2899-
* While we didn't change relhasindex, CREATE INDEX needs a
2900-
* transactional inval for when the new index's catalog rows become
2901-
* visible. Other CREATE INDEX and REINDEX code happens to also queue
2902-
* this inval, but keep this in case rare callers rely on this part of
2903-
* our API contract.
2904-
*/
2897+
/* no need to change tuple, but force relcache inval anyway */
29052898
CacheInvalidateRelcacheByTuple(tuple);
29062899
}
29072900

‎src/backend/commands/event_trigger.c

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -975,6 +975,11 @@ EventTriggerOnLogin(void)
975975
* this instead of regular updates serves two purposes. First,
976976
* that avoids possible waiting on the row-level lock. Second,
977977
* that avoids dealing with TOAST.
978+
*
979+
* Changes made by inplace update may be lost due to
980+
* concurrent normal updates; see inplace-inval.spec. However,
981+
* we are OK with that. The subsequent connections will still
982+
* have a chance to set "dathasloginevt" to false.
978983
*/
979984
systable_inplace_update_finish(state, tuple);
980985
}

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

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

509509
/*
510510
* Inplace updates are only ever performed on catalog tuples and
511-
* can, per definition, not change tuple visibility. Inplace
512-
* updates don't affect storage or interpretation of table rows,
513-
* so they don't affect logicalrep_write_tuple() outcomes. Hence,
514-
* we don't process invalidations from the original operation. If
515-
* inplace updates did affect those things, invalidations wouldn't
516-
* make it work, since there are no snapshot-specific versions of
517-
* inplace-updated values. Since we also don't decode catalog
518-
* tuples, we're not interested in the record's contents.
511+
* can, per definition, not change tuple visibility. Since we
512+
* don't decode catalog tuples, we're not interested in the
513+
* record's contents.
519514
*
520-
* WAL contains likely-unnecessary commit-time invals from the
521-
* CacheInvalidateHeapTuple() call in heap_inplace_update().
522-
* Excess invalidation is safe.
515+
* In-place updates can be used either by XID-bearing transactions
516+
* (e.g. in CREATE INDEX CONCURRENTLY) or by XID-less
517+
* transactions (e.g. VACUUM). In the former case, the commit
518+
* record will include cache invalidations, so we mark the
519+
* transaction as catalog modifying here. Currently that's
520+
* redundant because the commit will do that as well, but once we
521+
* support decoding in-progress relations, this will be important.
523522
*/
523+
if (!TransactionIdIsValid(xid))
524+
break;
525+
526+
(void) SnapBuildProcessChange(builder, xid, buf->origptr);
527+
ReorderBufferXidSetCatalogChanges(ctx->reorder, xid, buf->origptr);
524528
break;
525529

526530
case XLOG_HEAP_CONFIRM:

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2288,8 +2288,7 @@ void
22882288
PrepareToInvalidateCacheTuple(Relation relation,
22892289
HeapTuple tuple,
22902290
HeapTuple newtuple,
2291-
void (*function) (int, uint32, Oid, void *),
2292-
void *context)
2291+
void (*function) (int, uint32, Oid))
22932292
{
22942293
slist_iter iter;
22952294
Oid reloid;
@@ -2330,7 +2329,7 @@ PrepareToInvalidateCacheTuple(Relation relation,
23302329
hashvalue = CatalogCacheComputeTupleHashValue(ccp, ccp->cc_nkeys, tuple);
23312330
dbid = ccp->cc_relisshared ? (Oid) 0 : MyDatabaseId;
23322331

2333-
(*function) (ccp->id, hashvalue, dbid, context);
2332+
(*function) (ccp->id, hashvalue, dbid);
23342333

23352334
if (newtuple)
23362335
{
@@ -2339,7 +2338,7 @@ PrepareToInvalidateCacheTuple(Relation relation,
23392338
newhashvalue = CatalogCacheComputeTupleHashValue(ccp, ccp->cc_nkeys, newtuple);
23402339

23412340
if (newhashvalue != hashvalue)
2342-
(*function) (ccp->id, newhashvalue, dbid, context);
2341+
(*function) (ccp->id, newhashvalue, dbid);
23432342
}
23442343
}
23452344
}

0 commit comments

Comments
 (0)