Reinstate HEAP_XMAX_LOCK_ONLY|HEAP_KEYS_UPDATED as allowed

Commit 866e24d47db1 added an assert that HEAP_XMAX_LOCK_ONLY and
HEAP_KEYS_UPDATED cannot appear together, on the faulty assumption that
the latter necessarily referred to an update and not a tuple lock; but
that's wrong, because SELECT FOR UPDATE can use precisely that
combination, as evidenced by the amcheck test case added here.

Remove the Assert(), and also patch amcheck's verify_heapam.c to not
complain if the combination is found.  Also, out of overabundance of
caution, update (across all branches) README.tuplock to be more explicit
about this.

Author: Julien Rouhaud <rjuju123@gmail.com>
Reviewed-by: Mahendra Singh Thalor <mahi6run@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Discussion: https://postgr.es/m/20210124061758.GA11756@nol

diff --git a/contrib/amcheck/t/001_verify_heapam.pl b/contrib/amcheck/t/001_verify_heapam.pl
index a2f65b826d375c0425809434b080e2e5f23ae41b..6050feb71215c0700147a5904dcd4ba5fe2a474f 100644 (file)
--- 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 88ab32490c0daa3edcd9ec95ca61a6e38fc63695..49f5ca0ef2c8f6d83aa35dfadd997a3da72c7574 100644 (file)
--- 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/README.tuplock b/src/backend/access/heap/README.tuplock
index d03ddf6cdcc82ab2160c13336dc8e4a8f15b6057..6441e8baf0e4af9979dd8800bbd0de0fab721c9d 100644 (file)
--- a/src/backend/access/heap/README.tuplock
+++ b/src/backend/access/heap/README.tuplock
@@ -146,9 +146,10 @@ The following infomask bits are applicable:
   FOR UPDATE; this is implemented by the HEAP_KEYS_UPDATED bit.
 
 - 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's set regardless of whether the XMAX is a TransactionId or a MultiXactId.
+  This bit lives in t_infomask2.  If set, indicates that the operation(s) done
+  by the XMAX compromise the tuple key, such as a SELECT FOR UPDATE, an UPDATE
+  that modifies the columns of the key, or a DELETE.  It's set regardless of
+  whether the XMAX is a TransactionId or a MultiXactId.
 
 We currently never set the HEAP_XMAX_COMMITTED when the HEAP_XMAX_IS_MULTI bit
 is set.

diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index fb7ad0bab47ac4049586bf92a787410f5dea7e39..75223c9beafd50c1acba9eff4e5cc955f49fe240 100644 (file)
--- 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)));