Date: Tue, 12 Nov 2024 16:26:57 -0800
From: Noah Misch <noah@leadboat.com>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: Melanie Plageman <melanieplageman@gmail.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	Andres Freund <andres@anarazel.de>, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17821: Assertion failed in heap_update() due to heap pruning
Message-ID: <20241113002657.ce@rfd.leadboat.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>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="l8Qf8/O6KW2x3VTE"
Content-Disposition: inline
In-Reply-To: <53a0f263-bece-bc96-e016-1690628a9980@gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20241113002657.ce%40rfd.leadboat.com>
Precedence: bulk


--l8Qf8/O6KW2x3VTE
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Apr 09, 2024 at 10:00:00PM +0300, Alexander Lakhin wrote:
> 09.04.2024 20:41, Melanie Plageman wrote:
> > Just to confirm, the original bug was filed against 15, but I see
> > ad98fb142 was committed on master as well as backpatched. You are able
> > to reproduce  your pg_subscription example on master? I tried your
> > repro (including the modification to minfree) and wasn't able to
> > reproduce on master on my machine. This could totally be my fault, but
> > I just wanted to confirm you were able to repro on master.
> 
> Yes, I reproduced both cases on master (27074bce0).

This looked a lot like the former LP_UNUSED race conditions that inplace.spec
demonstrated.  I forked inplace.spec to use regular MVCC updates (attached).
That reproduces the symptoms you describe.  This happens because syscache is a
way to get a tuple without a pin on the tuple's page and without satisfying
any snapshot.  Either a pin or a snapshot would stop the pruning.  That's how
regular UPDATE statements avoid this.

Some options for fixing it:

1. Make the ItemIdIsNormal() check a runtime check that causes heap_update()
   to return TM_Deleted.  It could Assert(RelationSupportsSysCache(rel)), too.

2. Like the previous, but introduce a TM_Pruned.  If pruning hadn't happened,
   heap_update() would have returned TM_Updated or TM_Deleted.  We don't know
   which one heap_update() would have returned, because pruning erased the
   t_ctid that would have driven that decision.

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.

I think this defect can't corrupt catalogs or other data.  It's merely a
source of transient failure.  While one of the test permutations does update a
pg_class tuple of a different rel, the update then fails with a primary key
violation.  All misdirected updates will fail that way, because we don't
update the primary key columns of syscache relations.  With assertions
disabled, I think heap_update() looks for an old tuple at lp_off=0, hence
treating the page header as a tuple header.  Bytes of pd_lsn become the
putative xmin.  The attached test gets "ERROR:  attempted to update invisible
tuple" error from that, but other PageHeader bit patterns might reach "could
not access status of transaction", unique constraint violations, etc.

I recommend (2) or (1), since they're self-contained.  (3) or (4) potentially
give a better user experience for this rare failure, but that's code churn at
every DDL heap_update() site.  How do you see it?

Thanks,
nm

--l8Qf8/O6KW2x3VTE
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="repro-ItemIdIsNormal-v1.patch"

Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@leadboat.com>

    
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index d00300c..787a995 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -49,6 +49,7 @@
 #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"
 
@@ -3233,6 +3234,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);
 
@@ -6372,6 +6374,9 @@ heap_inplace_update_and_unlock(Relation relation,
 	 */
 	PreInplace_Inval();
 
+	INJECTION_POINT_LOAD("SIInsertDataEntries2");
+	INJECTION_POINT_LOAD("SIInsertDataEntries3");
+
 	/*----------
 	 * NO EREPORT(ERROR) from here till changes are complete
 	 *
diff --git a/src/backend/storage/ipc/sinvaladt.c b/src/backend/storage/ipc/sinvaladt.c
index ff81744..d837f41 100644
--- a/src/backend/storage/ipc/sinvaladt.c
+++ b/src/backend/storage/ipc/sinvaladt.c
@@ -25,6 +25,7 @@
 #include "storage/shmem.h"
 #include "storage/sinvaladt.h"
 #include "storage/spin.h"
+#include "utils/injection_point.h"
 
 /*
  * Conceptually, the shared cache invalidation messages are stored in an
@@ -386,6 +387,17 @@ SIInsertDataEntries(const SharedInvalidationMessage *data, int n)
 		int			max;
 		int			i;
 
+		/*
+		 * CacheInvalidateSmgr skips the injection point, since this
+		 * facilitates a test that needs this only for delaying pg_class
+		 * syscache invals and relcache invals.
+		 */
+		if (n != 1 || data->sm.id != SHAREDINVALSMGR_ID)
+		{
+			INJECTION_POINT("SIInsertDataEntries2");
+			INJECTION_POINT("SIInsertDataEntries3");
+		}
+
 		n -= nthistime;
 
 		LWLockAcquire(SInvalWriteLock, LW_EXCLUSIVE);
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 0753a9d..013c68e 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -13,7 +13,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 syscache-update-pruned inplace
 
 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.out
new file mode 100644
index 0000000..9caba9a
--- /dev/null
+++ b/src/test/modules/injection_points/expected/syscache-update-pruned.out
@@ -0,0 +1,133 @@
+Parsed test spec with 4 sessions
+
+starting permutation: cachefill1 at2 vac3 grant1 mkrels4 wakegrant4 wakeinval4
+mkrels
+------
+      
+(1 row)
+
+injection_points_attach
+-----------------------
+                       
+(1 row)
+
+injection_points_attach
+-----------------------
+                       
+(1 row)
+
+injection_points_attach
+-----------------------
+                       
+(1 row)
+
+step cachefill1: SELECT pg_relation_filenode('vactest.orig50') > 0;
+?column?
+--------
+t       
+(1 row)
+
+step at2: ALTER TABLE vactest.orig50 FORCE ROW LEVEL SECURITY; <waiting ...>
+step vac3: VACUUM pg_class; <waiting ...>
+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>
+step mkrels4: 
+	SELECT vactest.mkrels('intruder', 1, 100);  -- repopulate LP_UNUSED
+
+mkrels
+------
+      
+(1 row)
+
+step wakegrant4: 
+	SELECT injection_points_detach('heap_update-before-pin');
+	SELECT injection_points_wakeup('heap_update-before-pin');
+
+injection_points_detach
+-----------------------
+                       
+(1 row)
+
+injection_points_wakeup
+-----------------------
+                       
+(1 row)
+
+step grant1: <... completed>
+ERROR:  duplicate key value violates unique constraint "pg_class_oid_index"
+step wakeinval4: 
+	SELECT injection_points_detach('SIInsertDataEntries2');
+	SELECT injection_points_detach('SIInsertDataEntries3');
+	SELECT injection_points_wakeup('SIInsertDataEntries2');
+	SELECT injection_points_wakeup('SIInsertDataEntries3');
+
+injection_points_detach
+-----------------------
+                       
+(1 row)
+
+injection_points_detach
+-----------------------
+                       
+(1 row)
+
+injection_points_wakeup
+-----------------------
+                       
+(1 row)
+
+injection_points_wakeup
+-----------------------
+                       
+(1 row)
+
+step at2: <... completed>
+step vac3: <... completed>
+
+starting permutation: cachefill1 at2 vac3 grant1 wakegrant4 wakeinval4
+mkrels
+------
+      
+(1 row)
+
+injection_points_attach
+-----------------------
+                       
+(1 row)
+
+injection_points_attach
+-----------------------
+                       
+(1 row)
+
+injection_points_attach
+-----------------------
+                       
+(1 row)
+
+step cachefill1: SELECT pg_relation_filenode('vactest.orig50') > 0;
+?column?
+--------
+t       
+(1 row)
+
+step at2: ALTER TABLE vactest.orig50 FORCE ROW LEVEL SECURITY; <waiting ...>
+step vac3: VACUUM pg_class; <waiting ...>
+step grant1: GRANT SELECT ON vactest.orig50 TO PUBLIC; <waiting ...>
+step wakegrant4: 
+	SELECT injection_points_detach('heap_update-before-pin');
+	SELECT injection_points_wakeup('heap_update-before-pin');
+
+injection_points_detach
+-----------------------
+                       
+(1 row)
+
+injection_points_wakeup
+-----------------------
+                       
+(1 row)
+
+PQconsumeInput failed: server closed the connection unexpectedly
+	This probably means the server terminated abnormally
+	before or while processing the request.
+
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 58f1900..626039e 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -43,6 +43,7 @@ tests += {
   'isolation': {
     'specs': [
       'basic',
+      'syscache-update-pruned',
       'inplace',
     ],
   },
diff --git a/src/test/modules/injection_points/specs/syscache-update-pruned.spec b/src/test/modules/injection_points/specs/syscache-update-pruned.spec
new file mode 100644
index 0000000..50dde9a
--- /dev/null
+++ b/src/test/modules/injection_points/specs/syscache-update-pruned.spec
@@ -0,0 +1,107 @@
+# 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.  When introduced, that spec
+# reached basically the same race condition, for inplace updates.  Commit
+# a07e03f fixed the inplace update case.
+
+# 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;
+	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
+	$$;
+}
+setup	{ VACUUM FULL pg_class;  -- reduce free space }
+setup
+{
+	SELECT vactest.mkrels('orig', 1, 49);
+	CREATE TABLE vactest.orig50 ();
+	SELECT vactest.mkrels('orig', 51, 100);
+}
+teardown
+{
+	DROP SCHEMA vactest CASCADE;
+	DROP EXTENSION injection_points;
+}
+
+# Wait during GRANT.
+session s1
+setup	{
+	SELECT injection_points_set_local();
+	SELECT injection_points_attach('heap_update-before-pin', 'wait');
+}
+step cachefill1	{ SELECT pg_relation_filenode('vactest.orig50') > 0; }
+step grant1	{ GRANT SELECT ON vactest.orig50 TO PUBLIC; }
+
+# Update of the tuple grant1 will update.  It has to be an update that uses a
+# relation lock instead of LockTuple(), else GRANT would block it.  Wait
+# before sending invals, so s1 will not get a cache miss.
+session s2
+setup	{
+	SELECT injection_points_set_local();
+	SELECT injection_points_attach('SIInsertDataEntries2', 'wait');
+}
+step at2		{ ALTER TABLE vactest.orig50 FORCE ROW LEVEL SECURITY; }
+
+# VACUUM to achieve LP_UNUSED.  Again delay any s1 cache miss.
+session s3
+setup	{
+	SELECT injection_points_set_local();
+	SELECT injection_points_attach('SIInsertDataEntries3', 'wait');
+}
+step vac3		{ VACUUM pg_class; }
+
+# Non-blocking actions.
+session s4
+# 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 vactest.mkrels('intruder', 1, 100);  -- repopulate LP_UNUSED
+}
+step wakegrant4	{
+	SELECT injection_points_detach('heap_update-before-pin');
+	SELECT injection_points_wakeup('heap_update-before-pin');
+}
+step wakeinval4	{
+	SELECT injection_points_detach('SIInsertDataEntries2');
+	SELECT injection_points_detach('SIInsertDataEntries3');
+	SELECT injection_points_wakeup('SIInsertDataEntries2');
+	SELECT injection_points_wakeup('SIInsertDataEntries3');
+}
+
+# tuple $FROM_SYSCACHE changes from LP_UNUSED to a successor
+permutation
+	cachefill1		# reads pg_class tuple T0 for vactest.orig50, xmax invalid
+	at2(wakeinval4)	# T0 becomes eligible for pruning, T1 is successor
+	vac3(at2)		# T0 becomes LP_UNUSED
+	grant1(wakegrant4)	# pauses at heap_update(T0)
+	mkrels4			# T0 becomes a new rel
+	wakegrant4		# s1 wakes, "duplicate key value violates unique"
+	wakeinval4
+
+# subtract mkrels4: tuple $FROM_SYSCACHE becomes LP_UNUSED
+permutation
+	cachefill1		# reads pg_class tuple T0 for vactest.orig50, xmax invalid
+	at2(wakeinval4)	# T0 becomes eligible for pruning, T1 is successor
+	vac3(at2)		# T0 becomes LP_UNUSED
+	grant1(wakegrant4)	# pauses at heap_update(T0)
+	wakegrant4		# s1 wakes, assertion failure
+	wakeinval4

--l8Qf8/O6KW2x3VTE--