Date: Tue, 2 Feb 2021 12:21:33 +0800
From: Julien Rouhaud <rjuju123@gmail.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Faulty HEAP_XMAX_LOCK_ONLY & HEAP_KEYS_UPDATED hintbit
 combination
Message-ID: <20210202042133.GA72239@nol>
References: <20210124061758.GA11756@nol>
 <20210201170048.GA22366@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="CE+1k2dSO48ffgeK"
Content-Disposition: inline
In-Reply-To: <20210201170048.GA22366@alvherre.pgsql>
Precedence: bulk


--CE+1k2dSO48ffgeK
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Feb 01, 2021 at 02:00:48PM -0300, Alvaro Herrera wrote:
> On 2021-Jan-24, Julien Rouhaud wrote:
> 
> > While working on pg14 compatibility for an extension relying on an apparently
> > uncommon combination of FOR UPDATE and stored function calls, I hit some new
> > Asserts introduced in 866e24d47db (Extend amcheck to check heap pages):
> > 
> > +	/*
> > +	 * Do not allow tuples with invalid combinations of hint bits to be placed
> > +	 * on a page.  These combinations are detected as corruption by the
> > +	 * contrib/amcheck logic, so if you disable one or both of these
> > +	 * assertions, make corresponding changes there.
> > +	 */
> > +	Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_LOCK_ONLY) &&
> > +			 (tuple->t_data->t_infomask2 & HEAP_KEYS_UPDATED)));
> > 
> > 
> > I attach a simple self contained script to reproduce the problem, the last
> > UPDATE triggering the Assert.
> 
> Maybe we should contest the idea that this is a sensible thing to Assert
> against.  AFAICS this was originally suggested here:
> https://www.postgresql.org/message-id/flat/CAFiTN-syyHc3jZoou51v0SR8z0POoNfktqEO6MaGig4YS8mosA%40mail.gmail.com#ad215d0ee0606b5f67bbc57d011c96b8
> and it appears now to have been a bad idea.  If I recall correctly,
> HEAP_KEYS_UPDATED is supposed to distinguish locks/updates that don't
> modify the key columns from those that do.  Since SELECT FOR UPDATE
> stands for a future update that may modify arbitrary portions of the
> tuple (including "key" columns), then it produces that bit, just as said
> UPDATE or a DELETE; as opposed to SELECT FOR NO KEY UPDATE which stands
> for a future UPDATE that will only change columns that aren't part of
> any keys.

Thanks for the clarification, that makes sense.

> So I think that I misspoke earlier in this thread when I said this is a
> bug, and that the right fix here is to remove the Assert() and change
> amcheck to match.

I'm attaching a patch to do so.

> Separately, maybe it'd also be good to have a test case based on
> Julien's SQL snippet that produces this particular infomask combination
> (and other interesting combinations) and passes them through VACUUM etc
> to see that everything behaves correctly.

I also updated amcheck perl regression tests to generate such a combination,
add added an additional pass of verify_heapam() just after the VACUUM.

> 
> You could also argue the HEAP_KEYS_UPDATED is a misnomer and that we'd
> do well to change its name, and update README.tuplock.

Changing the name may be overkill, but claryfing the hint bit usage in
README.tuplock would definitely be useful, especially since the combination
isn't always produced.  How about adding something like:

 HEAP_KEYS_UPDATED
  This bit lives in t_infomask2.  If set, indicates that the XMAX updated
  this tuple and changed the key values, or it deleted the tuple.
+  It can also be set in combination of HEAP_XMAX_LOCK_ONLY.
  It's set regardless of whether the XMAX is a TransactionId or a MultiXactId.


--CE+1k2dSO48ffgeK
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0001-Allow-HEAP_XMAX_LOCK_ONLY-and-HEAP_KEYS_UPDATED-c.patch"

From 529dff8991d4bd7db661612a450b40bbc53a1429 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 22 Jan 2021 00:06:34 +0800
Subject: [PATCH v1] Allow HEAP_XMAX_LOCK_ONLY and HEAP_KEYS_UPDATED
 combination.

This combination of hint bits was previously detected as a form of corruption,
but it can be obtained with some mix of SELECT ... FOR UPDATE and UPDATE
queries.

Discussion: https://postgr.es/m/20210124061758.GA11756@nol
---
 contrib/amcheck/t/001_verify_heapam.pl | 14 +++++++++++++-
 contrib/amcheck/verify_heapam.c        |  7 -------
 src/backend/access/heap/hio.c          |  8 +++-----
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/contrib/amcheck/t/001_verify_heapam.pl b/contrib/amcheck/t/001_verify_heapam.pl
index a2f65b826d..6050feb712 100644
--- a/contrib/amcheck/t/001_verify_heapam.pl
+++ b/contrib/amcheck/t/001_verify_heapam.pl
@@ -4,7 +4,7 @@ use warnings;
 use PostgresNode;
 use TestLib;
 
-use Test::More tests => 79;
+use Test::More tests => 80;
 
 my ($node, $result);
 
@@ -47,6 +47,9 @@ detects_heap_corruption(
 #
 fresh_test_table('test');
 $node->safe_psql('postgres', q(VACUUM (FREEZE, DISABLE_PAGE_SKIPPING) test));
+detects_no_corruption(
+	"verify_heapam('test')",
+	"all-frozen not corrupted table");
 corrupt_first_page('test');
 detects_heap_corruption("verify_heapam('test')",
 	"all-frozen corrupted table");
@@ -92,6 +95,15 @@ sub fresh_test_table
 		ALTER TABLE $relname ALTER b SET STORAGE external;
 		INSERT INTO $relname (a, b)
 			(SELECT gs, repeat('b',gs*10) FROM generate_series(1,1000) gs);
+		BEGIN;
+		SAVEPOINT s1;
+		SELECT 1 FROM $relname WHERE a = 42 FOR UPDATE;
+		UPDATE $relname SET b = b WHERE a = 42;
+		RELEASE s1;
+		SAVEPOINT s1;
+		SELECT 1 FROM $relname WHERE a = 42 FOR UPDATE;
+		UPDATE $relname SET b = b WHERE a = 42;
+		COMMIT;
 	));
 }
 
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 88ab32490c..49f5ca0ef2 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -608,13 +608,6 @@ check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, HeapCheckContext *ctx)
 								   ctx->tuphdr->t_hoff, ctx->lp_len));
 		header_garbled = true;
 	}
-	if ((ctx->tuphdr->t_infomask & HEAP_XMAX_LOCK_ONLY) &&
-		(ctx->tuphdr->t_infomask2 & HEAP_KEYS_UPDATED))
-	{
-		report_corruption(ctx,
-						  pstrdup("tuple is marked as only locked, but also claims key columns were updated"));
-		header_garbled = true;
-	}
 
 	if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
 		(ctx->tuphdr->t_infomask & HEAP_XMAX_IS_MULTI))
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index fb7ad0bab4..75223c9bea 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -49,12 +49,10 @@ RelationPutHeapTuple(Relation relation,
 
 	/*
 	 * Do not allow tuples with invalid combinations of hint bits to be placed
-	 * on a page.  These combinations are detected as corruption by the
-	 * contrib/amcheck logic, so if you disable one or both of these
-	 * assertions, make corresponding changes there.
+	 * on a page.  This combination is detected as corruption by the
+	 * contrib/amcheck logic, so if you disable this assertion, make
+	 * corresponding changes there.
 	 */
-	Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_LOCK_ONLY) &&
-			 (tuple->t_data->t_infomask2 & HEAP_KEYS_UPDATED)));
 	Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
 			 (tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI)));
 
-- 
2.20.1


--CE+1k2dSO48ffgeK--