From: Noah Misch <noah@leadboat.com>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: Melanie Plageman <melanieplageman@gmail.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Andres Freund <andres@anarazel.de>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17821: Assertion failed in heap_update() due to heap pruning
Date: Wed, 22 Jan 2025 18:13:49 -0800
Message-ID: <20250123021349.b0.nmisch@google.com> (raw)
In-Reply-To: <20250111214454.9a.nmisch@google.com>
References: <17821-dd8c334263399284@postgresql.org>
<13d30799-0107-6502-c81c-39ff5f1e7eb0@gmail.com>
<BEB9A06A-3B05-41DC-AEE4-AD3DC4AD67E5@anarazel.de>
<25c3399b-58a3-d727-7f97-93394fd1afa9@gmail.com>
<2a203fd8-e9c8-5f5d-40f8-707848905a1a@gmail.com>
<CAAKRu_bWz0KhTKW3SzetvhZL3gpsnyekB2WFCn-MO4aCx=37=g@mail.gmail.com>
<53a0f263-bece-bc96-e016-1690628a9980@gmail.com>
<20241113002657.ce@rfd.leadboat.com>
<20250111214454.9a.nmisch@google.com>
On Sat, Jan 11, 2025 at 01:44:54PM -0800, Noah Misch wrote:
> On Tue, Nov 12, 2024 at 04:26:57PM -0800, Noah Misch wrote:
> > 1. Make the ItemIdIsNormal() check a runtime check that causes heap_update()
> > to return TM_Deleted. It could Assert(RelationSupportsSysCache(rel)), too.
> A UNIQUE constraint violation is too late to avoid page header corruption.
> the page is next read from disk, it gets "ERROR: invalid page".
> I plan to write (1).
Attached. Outside of comments and tests, it's a simple 20-line patch. Since
only two weeks will remain before release freeze, in the absence of review, I
would push this after 2025-01-25T16:00+0000. Unfortunately, while this
prevents the page header corruption, there's a preexisting loss of DDL changes
that it doesn't fix. I'll start a new thread about that, but I've included a
preview in Appendix 1 below. Would anyone else like to work on the problem in
Appendix 1? If so, let me know and feel free to start that thread yourself.
I may not get to it in 2025.
The new code sets TM_FailureData.xmax to invalid, which TM_Deleted may have
never done before. I see no problem at sites using that xmax:
src/backend/executor/nodeModifyTable.c:3137: if (TransactionIdIsCurrentTransactionId(context->tmfd.xmax))
src/backend/executor/nodeModifyTable.c:3307: if (TransactionIdIsCurrentTransactionId(context->tmfd.xmax))
src/backend/access/heap/heapam_handler.c:395: priorXmax = tmfd->xmax;
src/backend/access/heap/heapam_handler.c:488: tmfd->xmax = priorXmax;
The first two will rightly compute
TransactionIdIsCurrentTransactionId()==false. The second two are dealing with
tmfd from heap_lock_tuple(), not from heap_update(). Those grep matches are
from v18, but v13 and PGXN don't have additional challenges.
The test was sensitive at first to debug_parallel_query, debug_discard_caches,
autovacuum, and -DCATCACHE_FORCE_RELEASE. It would have avoided an
alternative output to have a GUC controlling -DCATCACHE_FORCE_RELEASE, like we
have debug_discard_caches to control CLOBBER_CACHE_ALWAYS. I'll not be
shocked if the buildfarm finds another way to make the test unstable. We may
end up disabling autovacuum for src/test/modules/injection_points, since
autovacuum doesn't mix with non-local injection points that autovacuum can
reach. For now, I've aimed to preserve compatibility with autovacuum=on.
==== Appendix 1: the unfixed loss of DDL changes
Unfortunately, I found a related case the patch doesn't fix. The known-to-me
fixes for this other case are more invasive than the fix for $SUBJECT, so I
plan to move forward with the $SUBJECT fix and bring the following to a new
thread. The third & last permutation of the $SUBJECT patch test case covers
the unfixed case. In brief, suppose the oldtup for a catalog update is a
syscache entry with xmin=1234 ctid=(11,38). We call CatalogTupleUpdate(rel,
'(11,38)', newtup). Meanwhile, the tuple has been updated twice with a VACUUM
between the two updates, so (11,38) now contains an xmin=5678 tuple for the
same primary key. CatalogTupleUpdate() has no way to know that its caller
used a stale oldtup, so the effect is to discard the two intervening updates.
To fix that, I lean toward the following:
- Make heap_modify_tuple() copy xmin from the oldtup.
- Make simple_heap_update() assert that newtup contains a valid xmin.
- In heap_update(), if newtup contains a valid xmin and otid now points to
tuple with a different xmin, fail with TM_Deleted. (Invalid xmin is also
okay for heap_update() callers that don't route through
simple_heap_update(), e.g. ExecUpdate(). ExecUpdate() has a snapshot
preventing the loss.)
Catalog updates not based on heap_copytuple() or heap_modify_tuple() would
need changes. They'll keep working in non-assert builds if they pass invalid
xmin. They'll break if they pass garbage bytes in xmin.
Better ideas? Other ways our lack of snapshot protecting oldtup might break?
Two of my upthread alternatives would also fix this, but they both entail the
churn of changing every CatalogTupleUpdate() caller (6 PGXN modules and ~200
postgresql.git calls):
> > 3. Before using a syscache tuple for a heap_update(), pin its buffer and
> > compare the syscache xmin to the buffer tuple xmin. Normally, they'll
> > match. If they don't match, ERROR or re-fetch the latest tuple as though
> > the syscache lookup had experienced a cache miss.
> > 4. Take a self-exclusive heavyweight lock before we use the syscache to fetch
> > the old tuple for a heap_update(). That's how we avoid this if the only
> > catalog-modifying commands are ALTER TABLE.
==== Appendix 2: UNIQUE violation helps less than theorized
The repro-ItemIdIsNormal-v2.patch test case (see previous message of thread)
didn't get the UNIQUE violation I expected. I opted to find out why not,
because I thought at the time we'll be relying long-term on the UNIQUE
violation when a different live tuple has replaced the ctid we're updating.
The problem update makes index entries for its new tuple, but this code stops
us from reporting the violation:
* Otherwise we have a definite conflict. But before
* complaining, look to see if the tuple we want to insert
* is itself now committed dead --- if so, don't complain.
* This is a waste of time in normal scenarios but we must
* do it to support CREATE INDEX CONCURRENTLY.
...
if (table_index_fetch_tuple_check(heapRel, &htid,
SnapshotSelf, NULL))
Since the page header corruption sets pd_lower=0, PageGetMaxOffsetNumber()
concludes the page has no tuples, and that table_index_fetch_tuple_check()
finds nothing.
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
Disable runningcheck for src/test/modules/injection_points/specs.
Directory "injection_points" has specified NO_INSTALLCHECK since before
commit c35f419d6efbdf1a050250d84b687e6705917711 added the specs, but
that commit neglected to disable the corresponding meson runningcheck.
The alternative would be to enable "make installcheck" for ISOLATION,
but the GNU make build system lacks a concept of setting NO_INSTALLCHECK
for REGRESS without also setting it for ISOLATION. Back-patch to v17,
where that commit first appeared, to avoid surprises when back-patching
additional specs.
Reviewed by FIXME.
Discussion: https://postgr.es/m/17821-dd8c334263399284@postgresql.orgdiff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.buildindex 989b4db..ebe79fe 100644--- a/src/test/modules/injection_points/meson.build+++ b/src/test/modules/injection_points/meson.build@@ -45,6 +45,7 @@ tests += {
'basic',
'inplace',
],
+ 'runningcheck': false, # align with GNU make build system
},
'tap': {
'env': {
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
Merge copies of converting an XID to a FullTransactionId.
Assume twophase.c is the performance-sensitive caller, and preserve its
choice of unlikely() branch hint. Add some retrospective rationale for
that choice. Back-patch to v17, for the next commit to use it.
Reviewed (in earlier versions) by Michael Paquier.
Discussion: https://postgr.es/m/17821-dd8c334263399284@postgresql.org
Discussion: https://postgr.es/m/20250116010051.f3.nmisch@google.comdiff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.cindex 8a8e36d..8273123 100644--- a/contrib/amcheck/verify_heapam.c+++ b/contrib/amcheck/verify_heapam.c@@ -1877,7 +1877,9 @@ check_tuple(HeapCheckContext *ctx, bool *xmin_commit_status_ok,
/*
* Convert a TransactionId into a FullTransactionId using our cached values of
* the valid transaction ID range. It is the caller's responsibility to have
- * already updated the cached values, if necessary.+ * already updated the cached values, if necessary. This is akin to+ * FullTransactionIdFromAllowableAt(), but it tolerates corruption in the form+ * of an xid before epoch 0.
*/
static FullTransactionId
FullTransactionIdFromXidAndCtx(TransactionId xid, const HeapCheckContext *ctx)
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.cindex ab2f4a8..73a8055 100644--- a/src/backend/access/transam/twophase.c+++ b/src/backend/access/transam/twophase.c@@ -929,32 +929,16 @@ TwoPhaseGetDummyProc(TransactionId xid, bool lock_held)
/*
* Compute the FullTransactionId for the given TransactionId.
*
- * The wrap logic is safe here because the span of active xids cannot exceed one- * epoch at any given time.+ * This is safe if the xid has not yet reached COMMIT PREPARED or ROLLBACK+ * PREPARED. After those commands, concurrent vac_truncate_clog() may make+ * the xid cease to qualify as allowable. XXX Not all callers limit their+ * calls accordingly.
*/
static inline FullTransactionId
AdjustToFullTransactionId(TransactionId xid)
{
- FullTransactionId nextFullXid;- TransactionId nextXid;- uint32 epoch;-
Assert(TransactionIdIsValid(xid));
-- LWLockAcquire(XidGenLock, LW_SHARED);- nextFullXid = TransamVariables->nextXid;- LWLockRelease(XidGenLock);-- nextXid = XidFromFullTransactionId(nextFullXid);- epoch = EpochFromFullTransactionId(nextFullXid);- if (unlikely(xid > nextXid))- {- /* Wraparound occurred, must be from a prev epoch. */- Assert(epoch > 0);- epoch--;- }-- return FullTransactionIdFromEpochAndXid(epoch, xid);+ return FullTransactionIdFromAllowableAt(ReadNextFullTransactionId(), xid);
}
static inline int
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.cindex 3596af0..91b6a91 100644--- a/src/backend/access/transam/xlogreader.c+++ b/src/backend/access/transam/xlogreader.c@@ -2166,28 +2166,14 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page)
FullTransactionId
XLogRecGetFullXid(XLogReaderState *record)
{
- TransactionId xid,- next_xid;- uint32 epoch;-
/*
* This function is only safe during replay, because it depends on the
* replay state. See AdvanceNextFullTransactionIdPastXid() for more.
*/
Assert(AmStartupProcess() || !IsUnderPostmaster);
- xid = XLogRecGetXid(record);- next_xid = XidFromFullTransactionId(TransamVariables->nextXid);- epoch = EpochFromFullTransactionId(TransamVariables->nextXid);-- /*- * If xid is numerically greater than next_xid, it has to be from the last- * epoch.- */- if (unlikely(xid > next_xid))- --epoch;-- return FullTransactionIdFromEpochAndXid(epoch, xid);+ return FullTransactionIdFromAllowableAt(TransamVariables->nextXid,+ XLogRecGetXid(record));
}
#endif
diff --git a/src/backend/utils/adt/xid8funcs.c b/src/backend/utils/adt/xid8funcs.cindex 4736755..20b28b2 100644--- a/src/backend/utils/adt/xid8funcs.c+++ b/src/backend/utils/adt/xid8funcs.c@@ -97,15 +97,11 @@ static bool
TransactionIdInRecentPast(FullTransactionId fxid, TransactionId *extracted_xid)
{
TransactionId xid = XidFromFullTransactionId(fxid);
- uint32 now_epoch;- TransactionId now_epoch_next_xid;
FullTransactionId now_fullxid;
- TransactionId oldest_xid;- FullTransactionId oldest_fxid;+ TransactionId oldest_clog_xid;+ FullTransactionId oldest_clog_fxid;
now_fullxid = ReadNextFullTransactionId();
- now_epoch_next_xid = XidFromFullTransactionId(now_fullxid);- now_epoch = EpochFromFullTransactionId(now_fullxid);
if (extracted_xid != NULL)
*extracted_xid = xid;
@@ -135,52 +131,19 @@ TransactionIdInRecentPast(FullTransactionId fxid, TransactionId *extracted_xid)
/*
* If fxid is not older than TransamVariables->oldestClogXid, the relevant
- * CLOG entry is guaranteed to still exist. Convert- * TransamVariables->oldestClogXid into a FullTransactionId to compare it- * with fxid. Determine the right epoch knowing that oldest_fxid- * shouldn't be more than 2^31 older than now_fullxid.+ * CLOG entry is guaranteed to still exist.+ *+ * TransamVariables->oldestXid governs allowable XIDs. Usually,+ * oldestClogXid==oldestXid. It's also possible for oldestClogXid to+ * follow oldestXid, in which case oldestXid might advance after our+ * ReadNextFullTransactionId() call. If oldestXid has advanced, that+ * advancement reinstated the usual oldestClogXid==oldestXid. Whether or+ * not that happened, oldestClogXid is allowable relative to now_fullxid.
*/
- oldest_xid = TransamVariables->oldestClogXid;- Assert(TransactionIdPrecedesOrEquals(oldest_xid, now_epoch_next_xid));- if (oldest_xid <= now_epoch_next_xid)- {- oldest_fxid = FullTransactionIdFromEpochAndXid(now_epoch, oldest_xid);- }- else- {- Assert(now_epoch > 0);- oldest_fxid = FullTransactionIdFromEpochAndXid(now_epoch - 1, oldest_xid);- }- return !FullTransactionIdPrecedes(fxid, oldest_fxid);-}--/*- * Convert a TransactionId obtained from a snapshot held by the caller to a- * FullTransactionId. Use next_fxid as a reference FullTransactionId, so that- * we can compute the high order bits. It must have been obtained by the- * caller with ReadNextFullTransactionId() after the snapshot was created.- */-static FullTransactionId-widen_snapshot_xid(TransactionId xid, FullTransactionId next_fxid)-{- TransactionId next_xid = XidFromFullTransactionId(next_fxid);- uint32 epoch = EpochFromFullTransactionId(next_fxid);-- /* Special transaction ID. */- if (!TransactionIdIsNormal(xid))- return FullTransactionIdFromEpochAndXid(0, xid);-- /*- * The 64 bit result must be <= next_fxid, since next_fxid hadn't been- * issued yet when the snapshot was created. Every TransactionId in the- * snapshot must therefore be from the same epoch as next_fxid, or the- * epoch before. We know this because next_fxid is never allow to get- * more than one epoch ahead of the TransactionIds in any snapshot.- */- if (xid > next_xid)- epoch--;-- return FullTransactionIdFromEpochAndXid(epoch, xid);+ oldest_clog_xid = TransamVariables->oldestClogXid;+ oldest_clog_fxid =+ FullTransactionIdFromAllowableAt(now_fullxid, oldest_clog_xid);+ return !FullTransactionIdPrecedes(fxid, oldest_clog_fxid);
}
/*
@@ -420,12 +383,18 @@ pg_current_snapshot(PG_FUNCTION_ARGS)
nxip = cur->xcnt;
snap = palloc(PG_SNAPSHOT_SIZE(nxip));
- /* fill */- snap->xmin = widen_snapshot_xid(cur->xmin, next_fxid);- snap->xmax = widen_snapshot_xid(cur->xmax, next_fxid);+ /*+ * Fill. This is the current backend's active snapshot, so MyProc->xmin+ * is <= all these XIDs. As long as that remains so, oldestXid can't+ * advance past any of these XIDs. Hence, these XIDs remain allowable+ * relative to next_fxid.+ */+ snap->xmin = FullTransactionIdFromAllowableAt(next_fxid, cur->xmin);+ snap->xmax = FullTransactionIdFromAllowableAt(next_fxid, cur->xmax);
snap->nxip = nxip;
for (i = 0; i < nxip; i++)
- snap->xip[i] = widen_snapshot_xid(cur->xip[i], next_fxid);+ snap->xip[i] =+ FullTransactionIdFromAllowableAt(next_fxid, cur->xip[i]);
/*
* We want them guaranteed to be in ascending order. This also removes
diff --git a/src/include/access/transam.h b/src/include/access/transam.hindex 0cab865..7d82cd2 100644--- a/src/include/access/transam.h+++ b/src/include/access/transam.h@@ -370,6 +370,49 @@ FullTransactionIdNewer(FullTransactionId a, FullTransactionId b)
return b;
}
+/*+ * Compute FullTransactionId for the given TransactionId, assuming xid was+ * between [oldestXid, nextXid] at the time when TransamVariables->nextXid was+ * nextFullXid. When adding calls, evaluate what prevents xid from preceding+ * oldestXid if SetTransactionIdLimit() runs between the collection of xid and+ * the collection of nextFullXid.+ */+static inline FullTransactionId+FullTransactionIdFromAllowableAt(FullTransactionId nextFullXid,+ TransactionId xid)+{+ uint32 epoch;++ /* Special transaction ID. */+ if (!TransactionIdIsNormal(xid))+ return FullTransactionIdFromEpochAndXid(0, xid);++ Assert(TransactionIdPrecedesOrEquals(xid,+ XidFromFullTransactionId(nextFullXid)));++ /*+ * The 64 bit result must be <= nextFullXid, since nextFullXid hadn't been+ * issued yet when xid was in the past. The xid must therefore be from+ * the epoch of nextFullXid or the epoch before. We know this because we+ * must remove (by freezing) an XID before assigning the XID half an epoch+ * ahead of it.+ *+ * The unlikely() branch hint is dubious. It's perfect for the first 2^32+ * XIDs of a cluster's life. Right at 2^32 XIDs, misprediction shoots to+ * 100%, then improves until perfection returns 2^31 XIDs later. Since+ * current callers pass relatively-recent XIDs, expect >90% prediction+ * accuracy overall. This favors average latency over tail latency.+ */+ epoch = EpochFromFullTransactionId(nextFullXid);+ if (unlikely(xid > XidFromFullTransactionId(nextFullXid)))+ {+ Assert(epoch != 0);+ epoch--;+ }++ return FullTransactionIdFromEpochAndXid(epoch, xid);+}+
#endif /* FRONTEND */
#endif /* TRANSAM_H */
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
At update of non-LP_NORMAL TID, fail instead of corrupting page header.
The right mix of DDL and VACUUM could corrupt a page header such that
PageIsVerified() durably fails, requiring a restore from backup. One of
the test permutations shows a variant not yet fixed.
This makes !TransactionIdIsValid(TM_FailureData.xmax) possible with
TM_Deleted. Core and PGXN appear indifferent to that.
Per bug #17821 from Alexander Lakhin. Back-patch to v13 (all supported
versions). The test case is v17+, since it uses INJECTION_POINT.
Reviewed by FIXME.
Discussion: https://postgr.es/m/17821-dd8c334263399284@postgresql.orgdiff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.cindex b634995..9d00808 100644--- a/src/backend/access/heap/heapam.c+++ b/src/backend/access/heap/heapam.c@@ -49,8 +49,10 @@
#include "storage/predicate.h"
#include "storage/procarray.h"
#include "utils/datum.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/spccache.h"
+#include "utils/syscache.h"
static HeapTuple heap_prepare_insert(Relation relation, HeapTuple tup,
@@ -3254,6 +3256,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
interesting_attrs = bms_add_members(interesting_attrs, id_attrs);
block = ItemPointerGetBlockNumber(otid);
+ INJECTION_POINT("heap_update-before-pin");
buffer = ReadBuffer(relation, block);
page = BufferGetPage(buffer);
@@ -3269,7 +3272,51 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
lp = PageGetItemId(page, ItemPointerGetOffsetNumber(otid));
- Assert(ItemIdIsNormal(lp));++ /*+ * Usually, a buffer pin and/or snapshot blocks pruning of otid, ensuring+ * we see LP_NORMAL here. When the otid origin is a syscache, we may have+ * neither a pin nor a snapshot. Hence, we may see other LP_ states, each+ * of which indicates concurrent pruning.+ *+ * Failing with TM_Updated would be most accurate. However, unlike other+ * TM_Updated scenarios, we don't know the successor ctid in LP_UNUSED and+ * LP_DEAD cases. While the distinction between TM_Updated and TM_Deleted+ * does matter to SQL statements UPDATE and MERGE, those SQL statements+ * hold a snapshot that ensures LP_NORMAL. Hence, the choice between+ * TM_Updated and TM_Deleted affects only the wording of error messages.+ * Settle on TM_Deleted, for two reasons. First, it avoids complicating+ * the specification of when tmfd->ctid is valid. Second, it creates+ * error log evidence that we took this branch.+ *+ * Since it's possible to see LP_UNUSED at otid, it's also possible to see+ * LP_NORMAL for a tuple that replaced LP_UNUSED. If it's a tuple for an+ * unrelated row, we'll fail with "duplicate key value violates unique".+ * XXX if otid is the live, newer version of the newtup row, we'll discard+ * changes originating in versions of this catalog row after the version+ * the caller got from syscache. See syscache-update-pruned.spec.+ */+ if (!ItemIdIsNormal(lp))+ {+ Assert(RelationSupportsSysCache(RelationGetRelid(relation)));++ UnlockReleaseBuffer(buffer);+ Assert(!have_tuple_lock);+ if (vmbuffer != InvalidBuffer)+ ReleaseBuffer(vmbuffer);+ tmfd->ctid = *otid;+ tmfd->xmax = InvalidTransactionId;+ tmfd->cmax = InvalidCommandId;+ *update_indexes = TU_None;++ bms_free(hot_attrs);+ bms_free(sum_attrs);+ bms_free(key_attrs);+ bms_free(id_attrs);+ /* modified_attrs not yet initialized */+ bms_free(interesting_attrs);+ return TM_Deleted;+ }
/*
* Fill in enough data in oldtup for HeapDetermineColumnsInfo to work
diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.cindex f41d314..32cf28b 100644--- a/src/backend/utils/cache/inval.c+++ b/src/backend/utils/cache/inval.c@@ -123,6 +123,7 @@
#include "storage/sinval.h"
#include "storage/smgr.h"
#include "utils/catcache.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/memdebug.h"
#include "utils/memutils.h"
@@ -1134,6 +1135,8 @@ AtEOXact_Inval(bool isCommit)
/* Must be at top of stack */
Assert(transInvalInfo->my_level == 1 && transInvalInfo->parent == NULL);
+ INJECTION_POINT("AtEOXact_Inval-with-transInvalInfo");+
if (isCommit)
{
/*
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.hindex 09b9b39..131c050 100644--- a/src/include/access/tableam.h+++ b/src/include/access/tableam.h@@ -136,7 +136,8 @@ typedef enum TU_UpdateIndexes
*
* xmax is the outdating transaction's XID. If the caller wants to visit the
* replacement tuple, it must check that this matches before believing the
- * replacement is really a match.+ * replacement is really a match. This is InvalidTransactionId if the target+ * was !LP_NORMAL (expected only for a TID retrieved from syscache).
*
* cmax is the outdating command's CID, but only when the failure code is
* TM_SelfModified (i.e., something in the current transaction outdated the
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefileindex 0753a9d..4f0161f 100644--- a/src/test/modules/injection_points/Makefile+++ b/src/test/modules/injection_points/Makefile@@ -5,7 +5,8 @@ OBJS = \
$(WIN32RES) \
injection_points.o \
injection_stats.o \
- injection_stats_fixed.o+ injection_stats_fixed.o \+ regress_injection.o
EXTENSION = injection_points
DATA = injection_points--1.0.sql
PGFILEDESC = "injection_points - facility for injection points"
@@ -13,7 +14,7 @@ PGFILEDESC = "injection_points - facility for injection points"
REGRESS = injection_points reindex_conc
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
-ISOLATION = basic inplace+ISOLATION = basic inplace syscache-update-pruned
TAP_TESTS = 1
diff --git a/src/test/modules/injection_points/expected/syscache-update-pruned.out b/src/test/modules/injection_points/expected/syscache-update-pruned.outnew file mode 100644index 0000000..5dc5a1d--- /dev/null+++ b/src/test/modules/injection_points/expected/syscache-update-pruned.out@@ -0,0 +1,87 @@+Parsed test spec with 4 sessions++starting permutation: cachefill1 at2 waitprunable4 vac4 grant1 wakeinval4 wakegrant4+step cachefill1: SELECT FROM vactest.reloid_catcache_set('vactest.orig50');+step at2: + CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+ <waiting ...>+step waitprunable4: CALL vactest.wait_prunable();+step vac4: VACUUM pg_class;+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>+step wakeinval4: + SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+ <waiting ...>+step at2: <... completed>+step wakeinval4: <... completed>+step wakegrant4: + SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+ <waiting ...>+step grant1: <... completed>+ERROR: tuple concurrently deleted+step wakegrant4: <... completed>++starting permutation: cachefill1 at2 waitprunable4 vac4 grant1 wakeinval4 mkrels4 wakegrant4+step cachefill1: SELECT FROM vactest.reloid_catcache_set('vactest.orig50');+step at2: + CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+ <waiting ...>+step waitprunable4: CALL vactest.wait_prunable();+step vac4: VACUUM pg_class;+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>+step wakeinval4: + SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+ <waiting ...>+step at2: <... completed>+step wakeinval4: <... completed>+step mkrels4: + SELECT FROM vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED++step wakegrant4: + SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+ <waiting ...>+step grant1: <... completed>+ERROR: duplicate key value violates unique constraint "pg_class_oid_index"+step wakegrant4: <... completed>++starting permutation: snap3 cachefill1 at2 mkrels4 r3 waitprunable4 vac4 grant1 wakeinval4 at4 wakegrant4 inspect4+step snap3: BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT;+step cachefill1: SELECT FROM vactest.reloid_catcache_set('vactest.orig50');+step at2: + CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+ <waiting ...>+step mkrels4: + SELECT FROM vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED++step r3: ROLLBACK;+step waitprunable4: CALL vactest.wait_prunable();+step vac4: VACUUM pg_class;+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>+step wakeinval4: + SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+ <waiting ...>+step at2: <... completed>+step wakeinval4: <... completed>+step at4: ALTER TABLE vactest.child50 INHERIT vactest.orig50;+step wakegrant4: + SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+ <waiting ...>+step grant1: <... completed>+step wakegrant4: <... completed>+step inspect4: + SELECT relhastriggers, relhassubclass FROM pg_class+ WHERE oid = 'vactest.orig50'::regclass;++relhastriggers|relhassubclass+--------------+--------------+f |f +(1 row)+diff --git a/src/test/modules/injection_points/expected/syscache-update-pruned_1.out b/src/test/modules/injection_points/expected/syscache-update-pruned_1.outnew file mode 100644index 0000000..b18857c--- /dev/null+++ b/src/test/modules/injection_points/expected/syscache-update-pruned_1.out@@ -0,0 +1,86 @@+Parsed test spec with 4 sessions++starting permutation: cachefill1 at2 waitprunable4 vac4 grant1 wakeinval4 wakegrant4+step cachefill1: SELECT FROM vactest.reloid_catcache_set('vactest.orig50');+step at2: + CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+ <waiting ...>+step waitprunable4: CALL vactest.wait_prunable();+step vac4: VACUUM pg_class;+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>+step wakeinval4: + SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+ <waiting ...>+step at2: <... completed>+step wakeinval4: <... completed>+step wakegrant4: + SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+ <waiting ...>+step grant1: <... completed>+step wakegrant4: <... completed>++starting permutation: cachefill1 at2 waitprunable4 vac4 grant1 wakeinval4 mkrels4 wakegrant4+step cachefill1: SELECT FROM vactest.reloid_catcache_set('vactest.orig50');+step at2: + CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+ <waiting ...>+step waitprunable4: CALL vactest.wait_prunable();+step vac4: VACUUM pg_class;+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>+step wakeinval4: + SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+ <waiting ...>+step at2: <... completed>+step wakeinval4: <... completed>+step mkrels4: + SELECT FROM vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED++step wakegrant4: + SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+ <waiting ...>+step grant1: <... completed>+step wakegrant4: <... completed>++starting permutation: snap3 cachefill1 at2 mkrels4 r3 waitprunable4 vac4 grant1 wakeinval4 at4 wakegrant4 inspect4+step snap3: BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT;+step cachefill1: SELECT FROM vactest.reloid_catcache_set('vactest.orig50');+step at2: + CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+ <waiting ...>+step mkrels4: + SELECT FROM vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED++step r3: ROLLBACK;+step waitprunable4: CALL vactest.wait_prunable();+step vac4: VACUUM pg_class;+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>+step wakeinval4: + SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+ <waiting ...>+step at2: <... completed>+step wakeinval4: <... completed>+step at4: ALTER TABLE vactest.child50 INHERIT vactest.orig50;+step wakegrant4: + SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+ <waiting ...>+step grant1: <... completed>+ERROR: tuple concurrently updated+step wakegrant4: <... completed>+step inspect4: + SELECT relhastriggers, relhassubclass FROM pg_class+ WHERE oid = 'vactest.orig50'::regclass;++relhastriggers|relhassubclass+--------------+--------------+t |t +(1 row)+diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sqlindex 6c81d55..c445bf6 100644--- a/src/test/modules/injection_points/injection_points--1.0.sql+++ b/src/test/modules/injection_points/injection_points--1.0.sql@@ -97,3 +97,11 @@ CREATE FUNCTION injection_points_stats_fixed(OUT numattach int8,
RETURNS record
AS 'MODULE_PATHNAME', 'injection_points_stats_fixed'
LANGUAGE C STRICT;
++--+-- regress_injection.c functions+--+CREATE FUNCTION removable_cutoff(rel regclass)+RETURNS xid8+AS 'MODULE_PATHNAME'+LANGUAGE C CALLED ON NULL INPUT;diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.buildindex ebe79fe..259045e 100644--- a/src/test/modules/injection_points/meson.build+++ b/src/test/modules/injection_points/meson.build@@ -8,6 +8,7 @@ injection_points_sources = files(
'injection_points.c',
'injection_stats.c',
'injection_stats_fixed.c',
+ 'regress_injection.c',
)
if host_system == 'windows'
@@ -44,8 +45,9 @@ tests += {
'specs': [
'basic',
'inplace',
+ 'syscache-update-pruned',
],
- 'runningcheck': false, # align with GNU make build system+ 'runningcheck': false, # see syscache-update-pruned
},
'tap': {
'env': {
diff --git a/src/test/modules/injection_points/regress_injection.c b/src/test/modules/injection_points/regress_injection.cnew file mode 100644index 0000000..422f416--- /dev/null+++ b/src/test/modules/injection_points/regress_injection.c@@ -0,0 +1,71 @@+/*--------------------------------------------------------------------------+ *+ * regress_injection.c+ * Functions supporting test-specific subject matter.+ *+ * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group+ * Portions Copyright (c) 1994, Regents of the University of California+ *+ * IDENTIFICATION+ * src/test/modules/injection_points/regress_injection.c+ *+ * -------------------------------------------------------------------------+ */++#include "postgres.h"++#include "access/table.h"+#include "fmgr.h"+#include "miscadmin.h"+#include "storage/procarray.h"+#include "utils/xid8.h"++/*+ * removable_cutoff - for syscache-update-pruned.spec+ *+ * Wrapper around GetOldestNonRemovableTransactionId(). In general, this can+ * move backward. runningcheck=false isolation tests can reasonably prevent+ * that. For the causes of backward movement, see+ * postgr.es/m/CAEze2Wj%2BV0kTx86xB_YbyaqTr5hnE_igdWAwuhSyjXBYscf5-Q%40mail.gmail.com+ * and the header comment for ComputeXidHorizons(). One can assume this+ * doesn't move backward if one arranges for concurrent activity not to reach+ * AbortTransaction() and not to allocate an XID while connected to another+ * database. Non-runningcheck tests can control most concurrent activity,+ * except autovacuum and the isolationtester control connection. Neither+ * allocates XIDs, and AbortTransaction() in those would justify test failure.+ */+PG_FUNCTION_INFO_V1(removable_cutoff);+Datum+removable_cutoff(PG_FUNCTION_ARGS)+{+ Relation rel = NULL;+ TransactionId xid;+ FullTransactionId next_fxid_before,+ next_fxid;++ /* could take other relkinds callee takes, but we've not yet needed it */+ if (!PG_ARGISNULL(0))+ rel = table_open(PG_GETARG_OID(0), AccessShareLock);++ /*+ * No lock or snapshot necessarily prevents oldestXid from advancing past+ * "xid" while this function runs. That concerns us only in that we must+ * not ascribe "xid" to the wrong epoch. (That may never arise in+ * isolation testing, but let's set a good example.) As a crude solution,+ * retry until nextXid doesn't change.+ */+ next_fxid = ReadNextFullTransactionId();+ do+ {+ CHECK_FOR_INTERRUPTS();+ next_fxid_before = next_fxid;+ xid = GetOldestNonRemovableTransactionId(rel);+ next_fxid = ReadNextFullTransactionId();+ } while (!FullTransactionIdEquals(next_fxid, next_fxid_before));++ if (rel)+ table_close(rel, AccessShareLock);++ PG_RETURN_FULLTRANSACTIONID(FullTransactionIdFromAllowableAt(next_fxid,+ xid));+}diff --git a/src/test/modules/injection_points/specs/syscache-update-pruned.spec b/src/test/modules/injection_points/specs/syscache-update-pruned.specnew file mode 100644index 0000000..b48e897--- /dev/null+++ b/src/test/modules/injection_points/specs/syscache-update-pruned.spec@@ -0,0 +1,179 @@+# Test race conditions involving:+# - s1: heap_update($FROM_SYSCACHE), without a snapshot or pin+# - s2: ALTER TABLE making $FROM_SYSCACHE a dead tuple+# - s3: "VACUUM pg_class" making $FROM_SYSCACHE become LP_UNUSED++# This is a derivative work of inplace.spec, which exercises the corresponding+# race condition for inplace updates.++# Despite local injection points, this is incompatible with runningcheck.+# First, removable_cutoff() could move backward, per its header comment.+# Second, other activity could trigger sinval queue overflow, negating our+# efforts to delay inval. Third, this deadlock emerges:+#+# - step at2 waits at an injection point, with interrupts held+# - an unrelated backend waits for at2 to do PROCSIGNAL_BARRIER_SMGRRELEASE+# - step waitprunable4 waits for the unrelated backend to release its xmin++# The alternative expected output is for -DCATCACHE_FORCE_RELEASE, a setting+# that thwarts testing the race conditions this spec seeks.+++# Need s2 to make a non-HOT update. Otherwise, "VACUUM pg_class" would leave+# an LP_REDIRECT that persists. To get non-HOT, make rels so the pg_class row+# for vactest.orig50 is on a filled page (assuming BLCKSZ=8192). Just to save+# on filesystem syscalls, use relkind=c for every other rel.+setup+{+ CREATE EXTENSION injection_points;+ CREATE SCHEMA vactest;+ -- Ensure a leader RELOID catcache entry. PARALLEL RESTRICTED since a+ -- parallel worker running pg_relation_filenode() would lack that effect.+ CREATE FUNCTION vactest.reloid_catcache_set(regclass) RETURNS int+ LANGUAGE sql PARALLEL RESTRICTED+ AS 'SELECT 0 FROM pg_relation_filenode($1)';+ CREATE FUNCTION vactest.mkrels(text, int, int) RETURNS void+ LANGUAGE plpgsql SET search_path = vactest AS $$+ DECLARE+ tname text;+ BEGIN+ FOR i in $2 .. $3 LOOP+ tname := $1 || i;+ EXECUTE FORMAT('CREATE TYPE ' || tname || ' AS ()');+ RAISE DEBUG '% at %', tname, ctid+ FROM pg_class WHERE oid = tname::regclass;+ END LOOP;+ END+ $$;+ CREATE PROCEDURE vactest.wait_prunable() LANGUAGE plpgsql AS $$+ DECLARE+ barrier xid8;+ cutoff xid8;+ BEGIN+ barrier := pg_current_xact_id();+ -- autovacuum worker RelationCacheInitializePhase3() or the+ -- isolationtester control connection might hold a snapshot that+ -- limits pruning. Sleep until that clears.+ LOOP+ ROLLBACK; -- release MyProc->xmin, which could be the oldest+ cutoff := removable_cutoff('pg_class');+ EXIT WHEN cutoff >= barrier;+ RAISE LOG 'removable cutoff %; waiting for %', cutoff, barrier;+ PERFORM pg_sleep(.1);+ END LOOP;+ END+ $$;+}+setup { CALL vactest.wait_prunable(); -- maximize next two VACUUMs }+setup { VACUUM FULL pg_class; -- reduce free space }+setup { VACUUM FREEZE pg_class; -- populate fsm etc. }+setup+{+ SELECT FROM vactest.mkrels('orig', 1, 49);+ CREATE TABLE vactest.orig50 (c int) WITH (autovacuum_enabled = off);+ CREATE TABLE vactest.child50 (c int) WITH (autovacuum_enabled = off);+ SELECT FROM vactest.mkrels('orig', 51, 100);+}+teardown+{+ DROP SCHEMA vactest CASCADE;+ DROP EXTENSION injection_points;+}++# Wait during GRANT. Disable debug_discard_caches, since we're here to+# exercise an outcome that happens under permissible cache staleness.+session s1+setup {+ SET debug_discard_caches = 0;+ SELECT FROM injection_points_set_local();+ SELECT FROM injection_points_attach('heap_update-before-pin', 'wait');+}+step cachefill1 { SELECT FROM vactest.reloid_catcache_set('vactest.orig50'); }+step grant1 { GRANT SELECT ON vactest.orig50 TO PUBLIC; }++# Update of the tuple that grant1 will update. Wait before sending invals, so+# s1 will not get a cache miss. Choose the commands for making such updates+# from among those whose heavyweight locking does not conflict with GRANT's+# heavyweight locking. (GRANT will see our XID as committed, so observing+# that XID in the tuple xmax also won't block GRANT.)+session s2+setup {+ SELECT FROM injection_points_set_local();+ SELECT FROM+ injection_points_attach('AtEOXact_Inval-with-transInvalInfo', 'wait');+}+step at2 {+ CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+}++# Hold snapshot to block pruning.+session s3+step snap3 { BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT; }+step r3 { ROLLBACK; }++# Non-blocking actions.+session s4+step waitprunable4 { CALL vactest.wait_prunable(); }+step vac4 { VACUUM pg_class; }+# Reuse the lp that s1 is waiting to change. I've observed reuse at the 1st+# or 18th CREATE, so create excess.+step mkrels4 {+ SELECT FROM vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED+}+step wakegrant4 {+ SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+}+step at4 { ALTER TABLE vactest.child50 INHERIT vactest.orig50; }+step wakeinval4 {+ SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+}+# Witness effects of steps at2 and/or at4.+step inspect4 {+ SELECT relhastriggers, relhassubclass FROM pg_class+ WHERE oid = 'vactest.orig50'::regclass;+}++# TID from syscache becomes LP_UNUSED. Before the bug fix, this permutation+# made s1 fail with "attempted to update invisible tuple" or an assert.+# However, suppose a pd_lsn value such that (pd_lsn.xlogid, pd_lsn.xrecoff)+# passed for (xmin, xmax) with xmin known-committed and xmax known-aborted.+# Persistent page header corruption ensued. For example, s1 overwrote+# pd_lower, pd_upper, and pd_special as though they were t_ctid.+permutation+ cachefill1 # reads pg_class tuple T0, xmax invalid+ at2 # T0 dead, T1 live+ waitprunable4 # T0 prunable+ vac4 # T0 becomes LP_UNUSED+ grant1 # pauses at heap_update(T0)+ wakeinval4(at2) # at2 sends inval message+ wakegrant4(grant1) # s1 wakes: "tuple concurrently deleted"++# add mkrels4: LP_UNUSED becomes a different rel's row+permutation+ cachefill1 # reads pg_class tuple T0, xmax invalid+ at2 # T0 dead, T1 live+ waitprunable4 # T0 prunable+ vac4 # T0 becomes LP_UNUSED+ grant1 # pauses at heap_update(T0)+ wakeinval4(at2) # at2 sends inval message+ mkrels4 # T0 becomes a new rel+ wakegrant4(grant1) # s1 wakes: "duplicate key value violates unique"++# TID from syscache becomes LP_UNUSED, then becomes a newer version of the+# original rel's row.+permutation+ snap3 # sets MyProc->xmin+ cachefill1 # reads pg_class tuple T0, xmax invalid+ at2 # T0 dead, T1 live+ mkrels4 # T1's page becomes full+ r3 # clears MyProc->xmin+ waitprunable4 # T0 prunable+ vac4 # T0 becomes LP_UNUSED+ grant1 # pauses at heap_update(T0)+ wakeinval4(at2) # at2 sends inval message+ at4 # T1 dead, T0 live+ wakegrant4(grant1) # s1 wakes: T0 dead, T2 live+ inspect4 # observe loss of at2+at4 changes XXX is an extant bugAttachments:update-unused-lp-10-no-installcheck-v1.patch (text/plain, 1.2K) download
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
Disable runningcheck for src/test/modules/injection_points/specs.
Directory "injection_points" has specified NO_INSTALLCHECK since before
commit c35f419d6efbdf1a050250d84b687e6705917711 added the specs, but
that commit neglected to disable the corresponding meson runningcheck.
The alternative would be to enable "make installcheck" for ISOLATION,
but the GNU make build system lacks a concept of setting NO_INSTALLCHECK
for REGRESS without also setting it for ISOLATION. Back-patch to v17,
where that commit first appeared, to avoid surprises when back-patching
additional specs.
Reviewed by FIXME.
Discussion: https://postgr.es/m/17821-dd8c334263399284@postgresql.org
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.buildindex 989b4db..ebe79fe 100644--- a/src/test/modules/injection_points/meson.build+++ b/src/test/modules/injection_points/meson.build@@ -45,6 +45,7 @@ tests += {
'basic',
'inplace',
],
+ 'runningcheck': false, # align with GNU make build system
},
'tap': {
'env': {
update-unused-lp-40-xid2fxid-v1.patch (text/plain, 10.1K) download
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
Merge copies of converting an XID to a FullTransactionId.
Assume twophase.c is the performance-sensitive caller, and preserve its
choice of unlikely() branch hint. Add some retrospective rationale for
that choice. Back-patch to v17, for the next commit to use it.
Reviewed (in earlier versions) by Michael Paquier.
Discussion: https://postgr.es/m/17821-dd8c334263399284@postgresql.org
Discussion: https://postgr.es/m/20250116010051.f3.nmisch@google.com
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.cindex 8a8e36d..8273123 100644--- a/contrib/amcheck/verify_heapam.c+++ b/contrib/amcheck/verify_heapam.c@@ -1877,7 +1877,9 @@ check_tuple(HeapCheckContext *ctx, bool *xmin_commit_status_ok,
/*
* Convert a TransactionId into a FullTransactionId using our cached values of
* the valid transaction ID range. It is the caller's responsibility to have
- * already updated the cached values, if necessary.+ * already updated the cached values, if necessary. This is akin to+ * FullTransactionIdFromAllowableAt(), but it tolerates corruption in the form+ * of an xid before epoch 0.
*/
static FullTransactionId
FullTransactionIdFromXidAndCtx(TransactionId xid, const HeapCheckContext *ctx)
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.cindex ab2f4a8..73a8055 100644--- a/src/backend/access/transam/twophase.c+++ b/src/backend/access/transam/twophase.c@@ -929,32 +929,16 @@ TwoPhaseGetDummyProc(TransactionId xid, bool lock_held)
/*
* Compute the FullTransactionId for the given TransactionId.
*
- * The wrap logic is safe here because the span of active xids cannot exceed one- * epoch at any given time.+ * This is safe if the xid has not yet reached COMMIT PREPARED or ROLLBACK+ * PREPARED. After those commands, concurrent vac_truncate_clog() may make+ * the xid cease to qualify as allowable. XXX Not all callers limit their+ * calls accordingly.
*/
static inline FullTransactionId
AdjustToFullTransactionId(TransactionId xid)
{
- FullTransactionId nextFullXid;- TransactionId nextXid;- uint32 epoch;-
Assert(TransactionIdIsValid(xid));
-- LWLockAcquire(XidGenLock, LW_SHARED);- nextFullXid = TransamVariables->nextXid;- LWLockRelease(XidGenLock);-- nextXid = XidFromFullTransactionId(nextFullXid);- epoch = EpochFromFullTransactionId(nextFullXid);- if (unlikely(xid > nextXid))- {- /* Wraparound occurred, must be from a prev epoch. */- Assert(epoch > 0);- epoch--;- }-- return FullTransactionIdFromEpochAndXid(epoch, xid);+ return FullTransactionIdFromAllowableAt(ReadNextFullTransactionId(), xid);
}
static inline int
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.cindex 3596af0..91b6a91 100644--- a/src/backend/access/transam/xlogreader.c+++ b/src/backend/access/transam/xlogreader.c@@ -2166,28 +2166,14 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page)
FullTransactionId
XLogRecGetFullXid(XLogReaderState *record)
{
- TransactionId xid,- next_xid;- uint32 epoch;-
/*
* This function is only safe during replay, because it depends on the
* replay state. See AdvanceNextFullTransactionIdPastXid() for more.
*/
Assert(AmStartupProcess() || !IsUnderPostmaster);
- xid = XLogRecGetXid(record);- next_xid = XidFromFullTransactionId(TransamVariables->nextXid);- epoch = EpochFromFullTransactionId(TransamVariables->nextXid);-- /*- * If xid is numerically greater than next_xid, it has to be from the last- * epoch.- */- if (unlikely(xid > next_xid))- --epoch;-- return FullTransactionIdFromEpochAndXid(epoch, xid);+ return FullTransactionIdFromAllowableAt(TransamVariables->nextXid,+ XLogRecGetXid(record));
}
#endif
diff --git a/src/backend/utils/adt/xid8funcs.c b/src/backend/utils/adt/xid8funcs.cindex 4736755..20b28b2 100644--- a/src/backend/utils/adt/xid8funcs.c+++ b/src/backend/utils/adt/xid8funcs.c@@ -97,15 +97,11 @@ static bool
TransactionIdInRecentPast(FullTransactionId fxid, TransactionId *extracted_xid)
{
TransactionId xid = XidFromFullTransactionId(fxid);
- uint32 now_epoch;- TransactionId now_epoch_next_xid;
FullTransactionId now_fullxid;
- TransactionId oldest_xid;- FullTransactionId oldest_fxid;+ TransactionId oldest_clog_xid;+ FullTransactionId oldest_clog_fxid;
now_fullxid = ReadNextFullTransactionId();
- now_epoch_next_xid = XidFromFullTransactionId(now_fullxid);- now_epoch = EpochFromFullTransactionId(now_fullxid);
if (extracted_xid != NULL)
*extracted_xid = xid;
@@ -135,52 +131,19 @@ TransactionIdInRecentPast(FullTransactionId fxid, TransactionId *extracted_xid)
/*
* If fxid is not older than TransamVariables->oldestClogXid, the relevant
- * CLOG entry is guaranteed to still exist. Convert- * TransamVariables->oldestClogXid into a FullTransactionId to compare it- * with fxid. Determine the right epoch knowing that oldest_fxid- * shouldn't be more than 2^31 older than now_fullxid.+ * CLOG entry is guaranteed to still exist.+ *+ * TransamVariables->oldestXid governs allowable XIDs. Usually,+ * oldestClogXid==oldestXid. It's also possible for oldestClogXid to+ * follow oldestXid, in which case oldestXid might advance after our+ * ReadNextFullTransactionId() call. If oldestXid has advanced, that+ * advancement reinstated the usual oldestClogXid==oldestXid. Whether or+ * not that happened, oldestClogXid is allowable relative to now_fullxid.
*/
- oldest_xid = TransamVariables->oldestClogXid;- Assert(TransactionIdPrecedesOrEquals(oldest_xid, now_epoch_next_xid));- if (oldest_xid <= now_epoch_next_xid)- {- oldest_fxid = FullTransactionIdFromEpochAndXid(now_epoch, oldest_xid);- }- else- {- Assert(now_epoch > 0);- oldest_fxid = FullTransactionIdFromEpochAndXid(now_epoch - 1, oldest_xid);- }- return !FullTransactionIdPrecedes(fxid, oldest_fxid);-}--/*- * Convert a TransactionId obtained from a snapshot held by the caller to a- * FullTransactionId. Use next_fxid as a reference FullTransactionId, so that- * we can compute the high order bits. It must have been obtained by the- * caller with ReadNextFullTransactionId() after the snapshot was created.- */-static FullTransactionId-widen_snapshot_xid(TransactionId xid, FullTransactionId next_fxid)-{- TransactionId next_xid = XidFromFullTransactionId(next_fxid);- uint32 epoch = EpochFromFullTransactionId(next_fxid);-- /* Special transaction ID. */- if (!TransactionIdIsNormal(xid))- return FullTransactionIdFromEpochAndXid(0, xid);-- /*- * The 64 bit result must be <= next_fxid, since next_fxid hadn't been- * issued yet when the snapshot was created. Every TransactionId in the- * snapshot must therefore be from the same epoch as next_fxid, or the- * epoch before. We know this because next_fxid is never allow to get- * more than one epoch ahead of the TransactionIds in any snapshot.- */- if (xid > next_xid)- epoch--;-- return FullTransactionIdFromEpochAndXid(epoch, xid);+ oldest_clog_xid = TransamVariables->oldestClogXid;+ oldest_clog_fxid =+ FullTransactionIdFromAllowableAt(now_fullxid, oldest_clog_xid);+ return !FullTransactionIdPrecedes(fxid, oldest_clog_fxid);
}
/*
@@ -420,12 +383,18 @@ pg_current_snapshot(PG_FUNCTION_ARGS)
nxip = cur->xcnt;
snap = palloc(PG_SNAPSHOT_SIZE(nxip));
- /* fill */- snap->xmin = widen_snapshot_xid(cur->xmin, next_fxid);- snap->xmax = widen_snapshot_xid(cur->xmax, next_fxid);+ /*+ * Fill. This is the current backend's active snapshot, so MyProc->xmin+ * is <= all these XIDs. As long as that remains so, oldestXid can't+ * advance past any of these XIDs. Hence, these XIDs remain allowable+ * relative to next_fxid.+ */+ snap->xmin = FullTransactionIdFromAllowableAt(next_fxid, cur->xmin);+ snap->xmax = FullTransactionIdFromAllowableAt(next_fxid, cur->xmax);
snap->nxip = nxip;
for (i = 0; i < nxip; i++)
- snap->xip[i] = widen_snapshot_xid(cur->xip[i], next_fxid);+ snap->xip[i] =+ FullTransactionIdFromAllowableAt(next_fxid, cur->xip[i]);
/*
* We want them guaranteed to be in ascending order. This also removes
diff --git a/src/include/access/transam.h b/src/include/access/transam.hindex 0cab865..7d82cd2 100644--- a/src/include/access/transam.h+++ b/src/include/access/transam.h@@ -370,6 +370,49 @@ FullTransactionIdNewer(FullTransactionId a, FullTransactionId b)
return b;
}
+/*+ * Compute FullTransactionId for the given TransactionId, assuming xid was+ * between [oldestXid, nextXid] at the time when TransamVariables->nextXid was+ * nextFullXid. When adding calls, evaluate what prevents xid from preceding+ * oldestXid if SetTransactionIdLimit() runs between the collection of xid and+ * the collection of nextFullXid.+ */+static inline FullTransactionId+FullTransactionIdFromAllowableAt(FullTransactionId nextFullXid,+ TransactionId xid)+{+ uint32 epoch;++ /* Special transaction ID. */+ if (!TransactionIdIsNormal(xid))+ return FullTransactionIdFromEpochAndXid(0, xid);++ Assert(TransactionIdPrecedesOrEquals(xid,+ XidFromFullTransactionId(nextFullXid)));++ /*+ * The 64 bit result must be <= nextFullXid, since nextFullXid hadn't been+ * issued yet when xid was in the past. The xid must therefore be from+ * the epoch of nextFullXid or the epoch before. We know this because we+ * must remove (by freezing) an XID before assigning the XID half an epoch+ * ahead of it.+ *+ * The unlikely() branch hint is dubious. It's perfect for the first 2^32+ * XIDs of a cluster's life. Right at 2^32 XIDs, misprediction shoots to+ * 100%, then improves until perfection returns 2^31 XIDs later. Since+ * current callers pass relatively-recent XIDs, expect >90% prediction+ * accuracy overall. This favors average latency over tail latency.+ */+ epoch = EpochFromFullTransactionId(nextFullXid);+ if (unlikely(xid > XidFromFullTransactionId(nextFullXid)))+ {+ Assert(epoch != 0);+ epoch--;+ }++ return FullTransactionIdFromEpochAndXid(epoch, xid);+}+
#endif /* FRONTEND */
#endif /* TRANSAM_H */
update-unused-lp-50-fix-v1.patch (text/plain, 24.8K) download
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
At update of non-LP_NORMAL TID, fail instead of corrupting page header.
The right mix of DDL and VACUUM could corrupt a page header such that
PageIsVerified() durably fails, requiring a restore from backup. One of
the test permutations shows a variant not yet fixed.
This makes !TransactionIdIsValid(TM_FailureData.xmax) possible with
TM_Deleted. Core and PGXN appear indifferent to that.
Per bug #17821 from Alexander Lakhin. Back-patch to v13 (all supported
versions). The test case is v17+, since it uses INJECTION_POINT.
Reviewed by FIXME.
Discussion: https://postgr.es/m/17821-dd8c334263399284@postgresql.org
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.cindex b634995..9d00808 100644--- a/src/backend/access/heap/heapam.c+++ b/src/backend/access/heap/heapam.c@@ -49,8 +49,10 @@
#include "storage/predicate.h"
#include "storage/procarray.h"
#include "utils/datum.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/spccache.h"
+#include "utils/syscache.h"
static HeapTuple heap_prepare_insert(Relation relation, HeapTuple tup,
@@ -3254,6 +3256,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
interesting_attrs = bms_add_members(interesting_attrs, id_attrs);
block = ItemPointerGetBlockNumber(otid);
+ INJECTION_POINT("heap_update-before-pin");
buffer = ReadBuffer(relation, block);
page = BufferGetPage(buffer);
@@ -3269,7 +3272,51 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
lp = PageGetItemId(page, ItemPointerGetOffsetNumber(otid));
- Assert(ItemIdIsNormal(lp));++ /*+ * Usually, a buffer pin and/or snapshot blocks pruning of otid, ensuring+ * we see LP_NORMAL here. When the otid origin is a syscache, we may have+ * neither a pin nor a snapshot. Hence, we may see other LP_ states, each+ * of which indicates concurrent pruning.+ *+ * Failing with TM_Updated would be most accurate. However, unlike other+ * TM_Updated scenarios, we don't know the successor ctid in LP_UNUSED and+ * LP_DEAD cases. While the distinction between TM_Updated and TM_Deleted+ * does matter to SQL statements UPDATE and MERGE, those SQL statements+ * hold a snapshot that ensures LP_NORMAL. Hence, the choice between+ * TM_Updated and TM_Deleted affects only the wording of error messages.+ * Settle on TM_Deleted, for two reasons. First, it avoids complicating+ * the specification of when tmfd->ctid is valid. Second, it creates+ * error log evidence that we took this branch.+ *+ * Since it's possible to see LP_UNUSED at otid, it's also possible to see+ * LP_NORMAL for a tuple that replaced LP_UNUSED. If it's a tuple for an+ * unrelated row, we'll fail with "duplicate key value violates unique".+ * XXX if otid is the live, newer version of the newtup row, we'll discard+ * changes originating in versions of this catalog row after the version+ * the caller got from syscache. See syscache-update-pruned.spec.+ */+ if (!ItemIdIsNormal(lp))+ {+ Assert(RelationSupportsSysCache(RelationGetRelid(relation)));++ UnlockReleaseBuffer(buffer);+ Assert(!have_tuple_lock);+ if (vmbuffer != InvalidBuffer)+ ReleaseBuffer(vmbuffer);+ tmfd->ctid = *otid;+ tmfd->xmax = InvalidTransactionId;+ tmfd->cmax = InvalidCommandId;+ *update_indexes = TU_None;++ bms_free(hot_attrs);+ bms_free(sum_attrs);+ bms_free(key_attrs);+ bms_free(id_attrs);+ /* modified_attrs not yet initialized */+ bms_free(interesting_attrs);+ return TM_Deleted;+ }
/*
* Fill in enough data in oldtup for HeapDetermineColumnsInfo to work
diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.cindex f41d314..32cf28b 100644--- a/src/backend/utils/cache/inval.c+++ b/src/backend/utils/cache/inval.c@@ -123,6 +123,7 @@
#include "storage/sinval.h"
#include "storage/smgr.h"
#include "utils/catcache.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/memdebug.h"
#include "utils/memutils.h"
@@ -1134,6 +1135,8 @@ AtEOXact_Inval(bool isCommit)
/* Must be at top of stack */
Assert(transInvalInfo->my_level == 1 && transInvalInfo->parent == NULL);
+ INJECTION_POINT("AtEOXact_Inval-with-transInvalInfo");+
if (isCommit)
{
/*
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.hindex 09b9b39..131c050 100644--- a/src/include/access/tableam.h+++ b/src/include/access/tableam.h@@ -136,7 +136,8 @@ typedef enum TU_UpdateIndexes
*
* xmax is the outdating transaction's XID. If the caller wants to visit the
* replacement tuple, it must check that this matches before believing the
- * replacement is really a match.+ * replacement is really a match. This is InvalidTransactionId if the target+ * was !LP_NORMAL (expected only for a TID retrieved from syscache).
*
* cmax is the outdating command's CID, but only when the failure code is
* TM_SelfModified (i.e., something in the current transaction outdated the
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefileindex 0753a9d..4f0161f 100644--- a/src/test/modules/injection_points/Makefile+++ b/src/test/modules/injection_points/Makefile@@ -5,7 +5,8 @@ OBJS = \
$(WIN32RES) \
injection_points.o \
injection_stats.o \
- injection_stats_fixed.o+ injection_stats_fixed.o \+ regress_injection.o
EXTENSION = injection_points
DATA = injection_points--1.0.sql
PGFILEDESC = "injection_points - facility for injection points"
@@ -13,7 +14,7 @@ PGFILEDESC = "injection_points - facility for injection points"
REGRESS = injection_points reindex_conc
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
-ISOLATION = basic inplace+ISOLATION = basic inplace syscache-update-pruned
TAP_TESTS = 1
diff --git a/src/test/modules/injection_points/expected/syscache-update-pruned.out b/src/test/modules/injection_points/expected/syscache-update-pruned.outnew file mode 100644index 0000000..5dc5a1d--- /dev/null+++ b/src/test/modules/injection_points/expected/syscache-update-pruned.out@@ -0,0 +1,87 @@+Parsed test spec with 4 sessions++starting permutation: cachefill1 at2 waitprunable4 vac4 grant1 wakeinval4 wakegrant4+step cachefill1: SELECT FROM vactest.reloid_catcache_set('vactest.orig50');+step at2: + CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+ <waiting ...>+step waitprunable4: CALL vactest.wait_prunable();+step vac4: VACUUM pg_class;+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>+step wakeinval4: + SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+ <waiting ...>+step at2: <... completed>+step wakeinval4: <... completed>+step wakegrant4: + SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+ <waiting ...>+step grant1: <... completed>+ERROR: tuple concurrently deleted+step wakegrant4: <... completed>++starting permutation: cachefill1 at2 waitprunable4 vac4 grant1 wakeinval4 mkrels4 wakegrant4+step cachefill1: SELECT FROM vactest.reloid_catcache_set('vactest.orig50');+step at2: + CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+ <waiting ...>+step waitprunable4: CALL vactest.wait_prunable();+step vac4: VACUUM pg_class;+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>+step wakeinval4: + SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+ <waiting ...>+step at2: <... completed>+step wakeinval4: <... completed>+step mkrels4: + SELECT FROM vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED++step wakegrant4: + SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+ <waiting ...>+step grant1: <... completed>+ERROR: duplicate key value violates unique constraint "pg_class_oid_index"+step wakegrant4: <... completed>++starting permutation: snap3 cachefill1 at2 mkrels4 r3 waitprunable4 vac4 grant1 wakeinval4 at4 wakegrant4 inspect4+step snap3: BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT;+step cachefill1: SELECT FROM vactest.reloid_catcache_set('vactest.orig50');+step at2: + CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+ <waiting ...>+step mkrels4: + SELECT FROM vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED++step r3: ROLLBACK;+step waitprunable4: CALL vactest.wait_prunable();+step vac4: VACUUM pg_class;+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>+step wakeinval4: + SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+ <waiting ...>+step at2: <... completed>+step wakeinval4: <... completed>+step at4: ALTER TABLE vactest.child50 INHERIT vactest.orig50;+step wakegrant4: + SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+ <waiting ...>+step grant1: <... completed>+step wakegrant4: <... completed>+step inspect4: + SELECT relhastriggers, relhassubclass FROM pg_class+ WHERE oid = 'vactest.orig50'::regclass;++relhastriggers|relhassubclass+--------------+--------------+f |f +(1 row)+diff --git a/src/test/modules/injection_points/expected/syscache-update-pruned_1.out b/src/test/modules/injection_points/expected/syscache-update-pruned_1.outnew file mode 100644index 0000000..b18857c--- /dev/null+++ b/src/test/modules/injection_points/expected/syscache-update-pruned_1.out@@ -0,0 +1,86 @@+Parsed test spec with 4 sessions++starting permutation: cachefill1 at2 waitprunable4 vac4 grant1 wakeinval4 wakegrant4+step cachefill1: SELECT FROM vactest.reloid_catcache_set('vactest.orig50');+step at2: + CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+ <waiting ...>+step waitprunable4: CALL vactest.wait_prunable();+step vac4: VACUUM pg_class;+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>+step wakeinval4: + SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+ <waiting ...>+step at2: <... completed>+step wakeinval4: <... completed>+step wakegrant4: + SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+ <waiting ...>+step grant1: <... completed>+step wakegrant4: <... completed>++starting permutation: cachefill1 at2 waitprunable4 vac4 grant1 wakeinval4 mkrels4 wakegrant4+step cachefill1: SELECT FROM vactest.reloid_catcache_set('vactest.orig50');+step at2: + CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+ <waiting ...>+step waitprunable4: CALL vactest.wait_prunable();+step vac4: VACUUM pg_class;+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>+step wakeinval4: + SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+ <waiting ...>+step at2: <... completed>+step wakeinval4: <... completed>+step mkrels4: + SELECT FROM vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED++step wakegrant4: + SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+ <waiting ...>+step grant1: <... completed>+step wakegrant4: <... completed>++starting permutation: snap3 cachefill1 at2 mkrels4 r3 waitprunable4 vac4 grant1 wakeinval4 at4 wakegrant4 inspect4+step snap3: BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT;+step cachefill1: SELECT FROM vactest.reloid_catcache_set('vactest.orig50');+step at2: + CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+ <waiting ...>+step mkrels4: + SELECT FROM vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED++step r3: ROLLBACK;+step waitprunable4: CALL vactest.wait_prunable();+step vac4: VACUUM pg_class;+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>+step wakeinval4: + SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+ <waiting ...>+step at2: <... completed>+step wakeinval4: <... completed>+step at4: ALTER TABLE vactest.child50 INHERIT vactest.orig50;+step wakegrant4: + SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+ <waiting ...>+step grant1: <... completed>+ERROR: tuple concurrently updated+step wakegrant4: <... completed>+step inspect4: + SELECT relhastriggers, relhassubclass FROM pg_class+ WHERE oid = 'vactest.orig50'::regclass;++relhastriggers|relhassubclass+--------------+--------------+t |t +(1 row)+diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sqlindex 6c81d55..c445bf6 100644--- a/src/test/modules/injection_points/injection_points--1.0.sql+++ b/src/test/modules/injection_points/injection_points--1.0.sql@@ -97,3 +97,11 @@ CREATE FUNCTION injection_points_stats_fixed(OUT numattach int8,
RETURNS record
AS 'MODULE_PATHNAME', 'injection_points_stats_fixed'
LANGUAGE C STRICT;
++--+-- regress_injection.c functions+--+CREATE FUNCTION removable_cutoff(rel regclass)+RETURNS xid8+AS 'MODULE_PATHNAME'+LANGUAGE C CALLED ON NULL INPUT;diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.buildindex ebe79fe..259045e 100644--- a/src/test/modules/injection_points/meson.build+++ b/src/test/modules/injection_points/meson.build@@ -8,6 +8,7 @@ injection_points_sources = files(
'injection_points.c',
'injection_stats.c',
'injection_stats_fixed.c',
+ 'regress_injection.c',
)
if host_system == 'windows'
@@ -44,8 +45,9 @@ tests += {
'specs': [
'basic',
'inplace',
+ 'syscache-update-pruned',
],
- 'runningcheck': false, # align with GNU make build system+ 'runningcheck': false, # see syscache-update-pruned
},
'tap': {
'env': {
diff --git a/src/test/modules/injection_points/regress_injection.c b/src/test/modules/injection_points/regress_injection.cnew file mode 100644index 0000000..422f416--- /dev/null+++ b/src/test/modules/injection_points/regress_injection.c@@ -0,0 +1,71 @@+/*--------------------------------------------------------------------------+ *+ * regress_injection.c+ * Functions supporting test-specific subject matter.+ *+ * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group+ * Portions Copyright (c) 1994, Regents of the University of California+ *+ * IDENTIFICATION+ * src/test/modules/injection_points/regress_injection.c+ *+ * -------------------------------------------------------------------------+ */++#include "postgres.h"++#include "access/table.h"+#include "fmgr.h"+#include "miscadmin.h"+#include "storage/procarray.h"+#include "utils/xid8.h"++/*+ * removable_cutoff - for syscache-update-pruned.spec+ *+ * Wrapper around GetOldestNonRemovableTransactionId(). In general, this can+ * move backward. runningcheck=false isolation tests can reasonably prevent+ * that. For the causes of backward movement, see+ * postgr.es/m/CAEze2Wj%2BV0kTx86xB_YbyaqTr5hnE_igdWAwuhSyjXBYscf5-Q%40mail.gmail.com+ * and the header comment for ComputeXidHorizons(). One can assume this+ * doesn't move backward if one arranges for concurrent activity not to reach+ * AbortTransaction() and not to allocate an XID while connected to another+ * database. Non-runningcheck tests can control most concurrent activity,+ * except autovacuum and the isolationtester control connection. Neither+ * allocates XIDs, and AbortTransaction() in those would justify test failure.+ */+PG_FUNCTION_INFO_V1(removable_cutoff);+Datum+removable_cutoff(PG_FUNCTION_ARGS)+{+ Relation rel = NULL;+ TransactionId xid;+ FullTransactionId next_fxid_before,+ next_fxid;++ /* could take other relkinds callee takes, but we've not yet needed it */+ if (!PG_ARGISNULL(0))+ rel = table_open(PG_GETARG_OID(0), AccessShareLock);++ /*+ * No lock or snapshot necessarily prevents oldestXid from advancing past+ * "xid" while this function runs. That concerns us only in that we must+ * not ascribe "xid" to the wrong epoch. (That may never arise in+ * isolation testing, but let's set a good example.) As a crude solution,+ * retry until nextXid doesn't change.+ */+ next_fxid = ReadNextFullTransactionId();+ do+ {+ CHECK_FOR_INTERRUPTS();+ next_fxid_before = next_fxid;+ xid = GetOldestNonRemovableTransactionId(rel);+ next_fxid = ReadNextFullTransactionId();+ } while (!FullTransactionIdEquals(next_fxid, next_fxid_before));++ if (rel)+ table_close(rel, AccessShareLock);++ PG_RETURN_FULLTRANSACTIONID(FullTransactionIdFromAllowableAt(next_fxid,+ xid));+}diff --git a/src/test/modules/injection_points/specs/syscache-update-pruned.spec b/src/test/modules/injection_points/specs/syscache-update-pruned.specnew file mode 100644index 0000000..b48e897--- /dev/null+++ b/src/test/modules/injection_points/specs/syscache-update-pruned.spec@@ -0,0 +1,179 @@+# Test race conditions involving:+# - s1: heap_update($FROM_SYSCACHE), without a snapshot or pin+# - s2: ALTER TABLE making $FROM_SYSCACHE a dead tuple+# - s3: "VACUUM pg_class" making $FROM_SYSCACHE become LP_UNUSED++# This is a derivative work of inplace.spec, which exercises the corresponding+# race condition for inplace updates.++# Despite local injection points, this is incompatible with runningcheck.+# First, removable_cutoff() could move backward, per its header comment.+# Second, other activity could trigger sinval queue overflow, negating our+# efforts to delay inval. Third, this deadlock emerges:+#+# - step at2 waits at an injection point, with interrupts held+# - an unrelated backend waits for at2 to do PROCSIGNAL_BARRIER_SMGRRELEASE+# - step waitprunable4 waits for the unrelated backend to release its xmin++# The alternative expected output is for -DCATCACHE_FORCE_RELEASE, a setting+# that thwarts testing the race conditions this spec seeks.+++# Need s2 to make a non-HOT update. Otherwise, "VACUUM pg_class" would leave+# an LP_REDIRECT that persists. To get non-HOT, make rels so the pg_class row+# for vactest.orig50 is on a filled page (assuming BLCKSZ=8192). Just to save+# on filesystem syscalls, use relkind=c for every other rel.+setup+{+ CREATE EXTENSION injection_points;+ CREATE SCHEMA vactest;+ -- Ensure a leader RELOID catcache entry. PARALLEL RESTRICTED since a+ -- parallel worker running pg_relation_filenode() would lack that effect.+ CREATE FUNCTION vactest.reloid_catcache_set(regclass) RETURNS int+ LANGUAGE sql PARALLEL RESTRICTED+ AS 'SELECT 0 FROM pg_relation_filenode($1)';+ CREATE FUNCTION vactest.mkrels(text, int, int) RETURNS void+ LANGUAGE plpgsql SET search_path = vactest AS $$+ DECLARE+ tname text;+ BEGIN+ FOR i in $2 .. $3 LOOP+ tname := $1 || i;+ EXECUTE FORMAT('CREATE TYPE ' || tname || ' AS ()');+ RAISE DEBUG '% at %', tname, ctid+ FROM pg_class WHERE oid = tname::regclass;+ END LOOP;+ END+ $$;+ CREATE PROCEDURE vactest.wait_prunable() LANGUAGE plpgsql AS $$+ DECLARE+ barrier xid8;+ cutoff xid8;+ BEGIN+ barrier := pg_current_xact_id();+ -- autovacuum worker RelationCacheInitializePhase3() or the+ -- isolationtester control connection might hold a snapshot that+ -- limits pruning. Sleep until that clears.+ LOOP+ ROLLBACK; -- release MyProc->xmin, which could be the oldest+ cutoff := removable_cutoff('pg_class');+ EXIT WHEN cutoff >= barrier;+ RAISE LOG 'removable cutoff %; waiting for %', cutoff, barrier;+ PERFORM pg_sleep(.1);+ END LOOP;+ END+ $$;+}+setup { CALL vactest.wait_prunable(); -- maximize next two VACUUMs }+setup { VACUUM FULL pg_class; -- reduce free space }+setup { VACUUM FREEZE pg_class; -- populate fsm etc. }+setup+{+ SELECT FROM vactest.mkrels('orig', 1, 49);+ CREATE TABLE vactest.orig50 (c int) WITH (autovacuum_enabled = off);+ CREATE TABLE vactest.child50 (c int) WITH (autovacuum_enabled = off);+ SELECT FROM vactest.mkrels('orig', 51, 100);+}+teardown+{+ DROP SCHEMA vactest CASCADE;+ DROP EXTENSION injection_points;+}++# Wait during GRANT. Disable debug_discard_caches, since we're here to+# exercise an outcome that happens under permissible cache staleness.+session s1+setup {+ SET debug_discard_caches = 0;+ SELECT FROM injection_points_set_local();+ SELECT FROM injection_points_attach('heap_update-before-pin', 'wait');+}+step cachefill1 { SELECT FROM vactest.reloid_catcache_set('vactest.orig50'); }+step grant1 { GRANT SELECT ON vactest.orig50 TO PUBLIC; }++# Update of the tuple that grant1 will update. Wait before sending invals, so+# s1 will not get a cache miss. Choose the commands for making such updates+# from among those whose heavyweight locking does not conflict with GRANT's+# heavyweight locking. (GRANT will see our XID as committed, so observing+# that XID in the tuple xmax also won't block GRANT.)+session s2+setup {+ SELECT FROM injection_points_set_local();+ SELECT FROM+ injection_points_attach('AtEOXact_Inval-with-transInvalInfo', 'wait');+}+step at2 {+ CREATE TRIGGER to_set_relhastriggers BEFORE UPDATE ON vactest.orig50+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();+}++# Hold snapshot to block pruning.+session s3+step snap3 { BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT; }+step r3 { ROLLBACK; }++# Non-blocking actions.+session s4+step waitprunable4 { CALL vactest.wait_prunable(); }+step vac4 { VACUUM pg_class; }+# Reuse the lp that s1 is waiting to change. I've observed reuse at the 1st+# or 18th CREATE, so create excess.+step mkrels4 {+ SELECT FROM vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED+}+step wakegrant4 {+ SELECT FROM injection_points_detach('heap_update-before-pin');+ SELECT FROM injection_points_wakeup('heap_update-before-pin');+}+step at4 { ALTER TABLE vactest.child50 INHERIT vactest.orig50; }+step wakeinval4 {+ SELECT FROM injection_points_detach('AtEOXact_Inval-with-transInvalInfo');+ SELECT FROM injection_points_wakeup('AtEOXact_Inval-with-transInvalInfo');+}+# Witness effects of steps at2 and/or at4.+step inspect4 {+ SELECT relhastriggers, relhassubclass FROM pg_class+ WHERE oid = 'vactest.orig50'::regclass;+}++# TID from syscache becomes LP_UNUSED. Before the bug fix, this permutation+# made s1 fail with "attempted to update invisible tuple" or an assert.+# However, suppose a pd_lsn value such that (pd_lsn.xlogid, pd_lsn.xrecoff)+# passed for (xmin, xmax) with xmin known-committed and xmax known-aborted.+# Persistent page header corruption ensued. For example, s1 overwrote+# pd_lower, pd_upper, and pd_special as though they were t_ctid.+permutation+ cachefill1 # reads pg_class tuple T0, xmax invalid+ at2 # T0 dead, T1 live+ waitprunable4 # T0 prunable+ vac4 # T0 becomes LP_UNUSED+ grant1 # pauses at heap_update(T0)+ wakeinval4(at2) # at2 sends inval message+ wakegrant4(grant1) # s1 wakes: "tuple concurrently deleted"++# add mkrels4: LP_UNUSED becomes a different rel's row+permutation+ cachefill1 # reads pg_class tuple T0, xmax invalid+ at2 # T0 dead, T1 live+ waitprunable4 # T0 prunable+ vac4 # T0 becomes LP_UNUSED+ grant1 # pauses at heap_update(T0)+ wakeinval4(at2) # at2 sends inval message+ mkrels4 # T0 becomes a new rel+ wakegrant4(grant1) # s1 wakes: "duplicate key value violates unique"++# TID from syscache becomes LP_UNUSED, then becomes a newer version of the+# original rel's row.+permutation+ snap3 # sets MyProc->xmin+ cachefill1 # reads pg_class tuple T0, xmax invalid+ at2 # T0 dead, T1 live+ mkrels4 # T1's page becomes full+ r3 # clears MyProc->xmin+ waitprunable4 # T0 prunable+ vac4 # T0 becomes LP_UNUSED+ grant1 # pauses at heap_update(T0)+ wakeinval4(at2) # at2 sends inval message+ at4 # T1 dead, T0 live+ wakegrant4(grant1) # s1 wakes: T0 dead, T2 live+ inspect4 # observe loss of at2+at4 changes XXX is an extant bug
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: noah@leadboat.com, exclusion@gmail.com, melanieplageman@gmail.com, hlinnaka@iki.fi, andres@anarazel.de, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17821: Assertion failed in heap_update() due to heap pruning
In-Reply-To: <20250123021349.b0.nmisch@google.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox