nocfbot-Add_`relcheckxmin`_to_track_and_enforce_tuple_visibility_in_`pg_class`.patch

Subject: [PATCH] Add `relcheckxmin` to track and enforce tuple visibility in `pg_class`
---
Index: doc/src/sgml/mvcc.sgml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
--- a/doc/src/sgml/mvcc.sgml	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/doc/src/sgml/mvcc.sgml	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -1833,22 +1833,30 @@
    <title>Caveats</title>
 
    <para>
-    Some commands, currently only <link linkend="sql-truncate"><command>TRUNCATE</command></link>, the
-    table-rewriting forms of <link linkend="sql-altertable"><command>ALTER
-    TABLE</command></link> and <command>REPACK</command> with
-    the <literal>CONCURRENTLY</literal> option, are not
-    MVCC-safe.  This means that after the truncation or rewrite commits, the
+    Some commands, currently only <link linkend="sql-truncate"><command>TRUNCATE</command></link>,
+    are not MVCC-safe.  This means that after the truncation commits, the
     table will appear empty to concurrent transactions, if they are using a
-    snapshot taken before the command committed.  This will only be an
-    issue for a transaction that did not access the table in question
-    before the command started &mdash; any transaction that has done so
-    would hold at least an <literal>ACCESS SHARE</literal> table lock,
-    which would block the truncating or rewriting command until that transaction completes.
-    So these commands will not cause any apparent inconsistency in the
-    table contents for successive queries on the target table, but they
+    snapshot taken before the command committed.  This will only be an issue
+    for a transaction that did not access the table in question before the
+    command started &mdash; any transaction that has done so would hold at
+    least an <literal>ACCESS SHARE</literal> table lock, which would block
+    the <command>TRUNCATE</command> command until that transaction completes.
+    So <command>TRUNCATE</command> will not cause any apparent inconsistency
+    in the table contents for successive queries on the target table, but it
     could cause visible inconsistency between the contents of the target
     table and other tables in the database.
    </para>
+
+   <para>
+    The table-rewriting forms of
+    <link linkend="sql-altertable"><command>ALTER TABLE</command></link>
+    and <link linkend="sql-repack"><command>REPACK</command></link>
+    with the <literal>CONCURRENTLY</literal> option are also not MVCC-safe.
+    If a transaction attempts to access a relation that was rewritten
+    after the transaction's snapshot was taken, an error will be raised.
+    As above, this will only be an issue for a transaction that did not
+    access the table before the rewriting command started.
+   </para>
 
    <para>
     Support for the Serializable transaction isolation level has not yet
Index: src/backend/catalog/heap.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
--- a/src/backend/catalog/heap.c	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/backend/catalog/heap.c	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -953,6 +953,7 @@
 	values[Anum_pg_class_relrewrite - 1] = ObjectIdGetDatum(rd_rel->relrewrite);
 	values[Anum_pg_class_relfrozenxid - 1] = TransactionIdGetDatum(rd_rel->relfrozenxid);
 	values[Anum_pg_class_relminmxid - 1] = MultiXactIdGetDatum(rd_rel->relminmxid);
+	values[Anum_pg_class_relcheckxmin - 1] = TransactionIdGetDatum(rd_rel->relcheckxmin);
 	if (relacl != (Datum) 0)
 		values[Anum_pg_class_relacl - 1] = relacl;
 	else
@@ -1023,6 +1024,8 @@
 	/* relispartition is always set by updating this tuple later */
 	new_rel_reltup->relispartition = false;
 
+	new_rel_reltup->relcheckxmin = InvalidTransactionId;
+
 	/* fill rd_att's type ID with something sane even if reltype is zero */
 	new_rel_desc->rd_att->tdtypeid = new_type_oid ? new_type_oid : RECORDOID;
 	new_rel_desc->rd_att->tdtypmod = -1;
Index: src/backend/commands/cluster.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
--- a/src/backend/commands/cluster.c	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/backend/commands/cluster.c	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -1031,7 +1031,7 @@
 		 * rebuild the target's indexes and throw away the transient table.
 		 */
 		finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog,
-						 swap_toast_by_content, false, true, true,
+						 swap_toast_by_content, false, true, true, InvalidTransactionId,
 						 frozenXid, cutoffMulti,
 						 relpersistence);
 	}
@@ -1429,6 +1429,7 @@
 swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
 					bool swap_toast_by_content,
 					bool is_internal,
+					TransactionId check_xmin,
 					TransactionId frozenXid,
 					MultiXactId cutoffMulti,
 					Oid *mapped_tables)
@@ -1611,6 +1612,9 @@
 		relform2->relallfrozen = swap_allfrozen;
 	}
 
+	relform1->relcheckxmin = check_xmin;
+	relform2->relcheckxmin = check_xmin;
+
 	/*
 	 * Update the tuples in pg_class --- unless the target relation of the
 	 * swap is pg_class itself.  In that case, there is zero point in making
@@ -1688,6 +1692,7 @@
 									target_is_pg_class,
 									swap_toast_by_content,
 									is_internal,
+									InvalidTransactionId,
 									frozenXid,
 									cutoffMulti,
 									mapped_tables);
@@ -1791,6 +1796,7 @@
 							target_is_pg_class,
 							swap_toast_by_content,
 							is_internal,
+							check_xmin,
 							InvalidTransactionId,
 							InvalidMultiXactId,
 							mapped_tables);
@@ -1814,6 +1820,7 @@
 				 bool check_constraints,
 				 bool is_internal,
 				 bool reindex,
+				 TransactionId check_xmin,
 				 TransactionId frozenXid,
 				 MultiXactId cutoffMulti,
 				 char newrelpersistence)
@@ -1835,7 +1842,7 @@
 	 */
 	swap_relation_files(OIDOldHeap, OIDNewHeap,
 						(OIDOldHeap == RelationRelationId),
-						swap_toast_by_content, is_internal,
+						swap_toast_by_content, is_internal, check_xmin,
 						frozenXid, cutoffMulti, mapped_tables);
 
 	/*
@@ -3335,6 +3342,7 @@
 							false,	/* swap_toast_by_content */
 							true,
 							InvalidTransactionId,
+							InvalidTransactionId,
 							InvalidMultiXactId,
 							mapped_tables);
 
@@ -3371,7 +3379,7 @@
 	finish_heap_swap(old_table_oid, new_table_oid,
 					 is_system_catalog,
 					 false,		/* swap_toast_by_content */
-					 false, true, false,
+					 false, true, false, GetCurrentTransactionId(),
 					 frozenXid, cutoffMulti,
 					 relpersistence);
 }
Index: src/backend/commands/matview.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
--- a/src/backend/commands/matview.c	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/backend/commands/matview.c	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -892,7 +892,7 @@
 static void
 refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap, char relpersistence)
 {
-	finish_heap_swap(matviewOid, OIDNewHeap, false, false, true, true, true,
+	finish_heap_swap(matviewOid, OIDNewHeap, false, false, true, true, true, InvalidTransactionId,
 					 RecentXmin, ReadNextMultiXactId(), relpersistence);
 }
 
Index: src/backend/commands/tablecmds.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
--- a/src/backend/commands/tablecmds.c	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/backend/commands/tablecmds.c	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -6026,6 +6026,7 @@
 							 false, false, true,
 							 !OidIsValid(tab->newTableSpace),
 							 true,
+							 GetCurrentTransactionId(),
 							 RecentXmin,
 							 ReadNextMultiXactId(),
 							 persistence);
Index: src/backend/commands/vacuum.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
--- a/src/backend/commands/vacuum.c	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/backend/commands/vacuum.c	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -1534,6 +1534,9 @@
 		if (update)
 		{
 			pgcform->relfrozenxid = frozenxid;
+			/* Clear relcheckxmin if frozenxid is higher, now it is safe */
+			if (TransactionIdPrecedes(pgcform->relcheckxmin, pgcform->relfrozenxid))
+				pgcform->relcheckxmin = InvalidTransactionId;
 			dirty = true;
 			if (frozenxid_updated)
 				*frozenxid_updated = true;
Index: src/backend/executor/execUtils.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
--- a/src/backend/executor/execUtils.c	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/backend/executor/execUtils.c	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -865,6 +865,24 @@
 		estate->es_relations[rti - 1] = rel;
 	}
 
+	if (unlikely(TransactionIdIsValid(rel->rd_check_xmin)))
+	{
+		/* We know it is committed, just need to be sure it is visible for current exec */
+		TransactionId xmin = rel->rd_check_xmin;
+		/* Fast check first */
+		if (unlikely(!TransactionIdIsCurrentTransactionId(xmin) &&
+				!TransactionIdPrecedes(xmin, TransactionXmin)))
+		{
+			Snapshot snapshot = GetActiveSnapshot();
+			/* Test remaining rules */
+			if (XidInMVCCSnapshot(xmin, snapshot))
+				ereport(ERROR,
+						errcode(ERRCODE_RELATION_UNAVAILABLE_FOR_CURRENT_TRANSACTION),
+						errmsg("relation \"%s\" cannot be accessed in current transaction",
+							   RelationGetRelationName(rel)));
+		}
+	}
+
 	return rel;
 }
 
Index: src/backend/utils/cache/relcache.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
--- a/src/backend/utils/cache/relcache.c	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/backend/utils/cache/relcache.c	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -1203,6 +1203,8 @@
 	 */
 	RelationBuildTupleDesc(relation);
 
+	relation->rd_check_xmin = relp->relcheckxmin;
+
 	/* foreign key data is not loaded till asked for */
 	relation->rd_fkeylist = NIL;
 	relation->rd_fkeyvalid = false;
Index: src/backend/utils/errcodes.txt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt
--- a/src/backend/utils/errcodes.txt	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/backend/utils/errcodes.txt	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -259,6 +259,7 @@
 25P02    E    ERRCODE_IN_FAILED_SQL_TRANSACTION                              in_failed_sql_transaction
 25P03    E    ERRCODE_IDLE_IN_TRANSACTION_SESSION_TIMEOUT                    idle_in_transaction_session_timeout
 25P04    E    ERRCODE_TRANSACTION_TIMEOUT                                    transaction_timeout
+25P05    E    ERRCODE_RELATION_UNAVAILABLE_FOR_CURRENT_TRANSACTION           relation_unavailable_for_current_transaction
 
 Section: Class 26 - Invalid SQL Statement Name
 
Index: src/include/catalog/catversion.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
--- a/src/include/catalog/catversion.h	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/include/catalog/catversion.h	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -57,6 +57,6 @@
  */
 
 /*							yyyymmddN */
-#define CATALOG_VERSION_NO	202601221
+#define CATALOG_VERSION_NO	202601242
 
 #endif
Index: src/include/catalog/pg_class.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
--- a/src/include/catalog/pg_class.h	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/include/catalog/pg_class.h	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -131,6 +131,9 @@
 	/* all multixacts in this rel are >= this; it is really a MultiXactId */
 	TransactionId relminmxid BKI_DEFAULT(1);	/* FirstMultiXactId */
 
+	/* need to check to prevent MVCC violations */
+	TransactionId relcheckxmin BKI_DEFAULT(0);	/* InvalidTransactionId */
+
 #ifdef CATALOG_VARLEN			/* variable-length fields start here */
 	/* NOTE: These fields are not present in a relcache entry's rd_rel field. */
 	/* access permissions */
@@ -146,7 +149,7 @@
 
 /* Size of fixed part of pg_class tuples, not counting var-length fields */
 #define CLASS_TUPLE_SIZE \
-	 (offsetof(FormData_pg_class,relminmxid) + sizeof(TransactionId))
+	 (offsetof(FormData_pg_class,relcheckxmin) + sizeof(TransactionId))
 
 /* ----------------
  *		Form_pg_class corresponds to a pointer to a tuple with
Index: src/include/commands/cluster.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
--- a/src/include/commands/cluster.h	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/include/commands/cluster.h	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -130,6 +130,7 @@
 							 bool check_constraints,
 							 bool is_internal,
 							 bool reindex,
+							 TransactionId check_xmin,
 							 TransactionId frozenXid,
 							 MultiXactId cutoffMulti,
 							 char newrelpersistence);
Index: src/include/utils/rel.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
--- a/src/include/utils/rel.h	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/include/utils/rel.h	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -143,6 +143,11 @@
 	 */
 	TransactionId rd_partdesc_nodetached_xmin;
 
+	/*
+	 * Used at execution phase to prevent violation of MVCC rules.
+	 */
+	TransactionId rd_check_xmin;
+
 	/* data managed by RelationGetPartitionQual: */
 	List	   *rd_partcheck;	/* partition CHECK quals */
 	bool		rd_partcheckvalid;	/* true if list has been computed */
Index: src/test/modules/injection_points/Makefile
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
--- a/src/test/modules/injection_points/Makefile	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/test/modules/injection_points/Makefile	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -16,6 +16,7 @@
 	    inplace \
 	    repack \
 	    repack_toast \
+	    repack_mvcc \
 	    syscache-update-pruned \
 	    heap_lock_update
 ISOLATION_OPTS = --temp-config $(top_srcdir)/src/test/modules/injection_points/logical.conf
Index: src/test/modules/injection_points/expected/repack_mvcc.out
===================================================================
diff --git a/src/test/modules/injection_points/expected/repack_mvcc.out b/src/test/modules/injection_points/expected/repack_mvcc.out
new file mode 100644
--- /dev/null	(revision e5311f14c463819fabe6c57fa74512f10c881508)
+++ b/src/test/modules/injection_points/expected/repack_mvcc.out	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -0,0 +1,19 @@
+Parsed test spec with 2 sessions
+
+starting permutation: begin repack check
+step begin: 
+	BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+	SELECT 1;
+
+?column?
+--------
+       1
+(1 row)
+
+step repack: 
+	REPACK (CONCURRENTLY) repack_test;
+
+step check: 
+	SELECT * FROM repack_test;
+
+ERROR:  relation "repack_test" cannot be accessed in current transaction
Index: src/test/modules/injection_points/meson.build
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
--- a/src/test/modules/injection_points/meson.build	(revision 14bf8dbc0ac0d52a6121be2484f652fc0c04732d)
+++ b/src/test/modules/injection_points/meson.build	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -47,6 +47,7 @@
       'inplace',
       'repack',
       'repack_toast',
+      'repack_mvcc',
       'syscache-update-pruned',
       'heap_lock_update',
     ],
Index: src/test/modules/injection_points/specs/repack_mvcc.spec
===================================================================
diff --git a/src/test/modules/injection_points/specs/repack_mvcc.spec b/src/test/modules/injection_points/specs/repack_mvcc.spec
new file mode 100644
--- /dev/null	(revision e5311f14c463819fabe6c57fa74512f10c881508)
+++ b/src/test/modules/injection_points/specs/repack_mvcc.spec	(revision e5311f14c463819fabe6c57fa74512f10c881508)
@@ -0,0 +1,38 @@
+# REPACK (CONCURRENTLY);
+#
+# Test handling of preventing access to non-mvcc safe data.
+setup
+{
+	CREATE EXTENSION injection_points;
+
+	CREATE TABLE repack_test(i int PRIMARY KEY);
+	INSERT INTO repack_test(i) VALUES (1), (2), (3);
+}
+
+teardown
+{
+	DROP TABLE repack_test;
+	DROP EXTENSION injection_points;
+}
+
+session s1
+step repack
+{
+	REPACK (CONCURRENTLY) repack_test;
+}
+
+session s2
+step begin
+{
+	BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
+	SELECT 1;
+}
+step check
+{
+	SELECT * FROM repack_test;
+}
+
+permutation
+	begin
+	repack
+	check