[#27288] YSQL: Avoid 'restart read required' error during yb_index_check()

Summary:
yb_index_check() scans all the rows of the index and the base relation. Hence, it becomes prone to 'Restart read required' error if any row is involved in a write operation concurrently with the yb_index_check() read point, which is a high probability event.

This hurts usability, but more importantly, yb_index_check() is not concerned with read-after-commit-visibility. It picks up a read time (and an associated snapshot) and uses it to scan both the index and base relation. Even if a write that got committed before the scan read point (as observed by an external clock) is missing from the snapshot due to clock skew, that's acceptable—its effects will be absent from both the index and the base table scans.

So, it is safe to set GUC yb_read_after_commit_visibility to 'relaxed' while executing yb_index_check() to avoid these errors. Note that this GUC cannot be set from within a transaction block. Don't do it in such cases and throw a warning instead.
Jira: DB-16780

Test Plan: ./yb_build.sh --gtest_filter PgYbIndexCheckTest.YbIndexCheckRestartReadRequired

Reviewers: patnaik.balivada

Reviewed By: patnaik.balivada

Subscribers: svc_phabricator, yql

Differential Revision: https://phorge.dev.yugabyte.com/D46002

Author: arpang

src/postgres/src/backend/utils/misc/yb_index_check.c

@@ -141,15 +141,36 @@ yb_index_check(PG_FUNCTION_ARGS)
 {
 	Oid			indexoid = PG_GETARG_OID(0);
 	bool		multi_snapshot_mode = !PG_GETARG_BOOL(1);
+	int			savedGUCLevel = -1;
 
 	if (yb_test_index_check_num_batches_per_snapshot == 0)
 		multi_snapshot_mode = false;
 
 	uint64		original_read_point PG_USED_FOR_ASSERTS_ONLY =
 		YBCPgGetCurrentReadPoint();
 
+	bool		is_txn_block = IsTransactionBlock();
+
+	if (is_txn_block)
+		ereport(NOTICE,
+				(errmsg("yb_index_check() is prone to 'Restart read required' "
+						"erorrs when executed from within a transaction "
+						"block.")));
+
+	if (!is_txn_block)
+	{
+		savedGUCLevel = NewGUCNestLevel();
+
+		(void) set_config_option("yb_read_after_commit_visibility", "relaxed",
+								 PGC_USERSET, PGC_S_SESSION, GUC_ACTION_SAVE,
+								 true, 0, false);
+	}
+
 	do_index_check(indexoid, multi_snapshot_mode);
 
+	if (!is_txn_block)
+		AtEOXact_GUC(false, savedGUCLevel);
+
 	/*
 	 * yb_index_check() uses multiple snapshots in multi_snapshot_mode. Verify
 	 * that it restored the original read point at the end.

src/postgres/src/test/regress/expected/yb.orig.yb_index_check.out

@@ -305,6 +305,16 @@ SELECT yb_index_check('test_bytea_c_b_idx'::regclass::oid);
 
 (1 row)
 
+-- Inside a transaction block
+BEGIN;
+SELECT yb_index_check('abcd_b_c_d_idx'::regclass::oid);
+NOTICE:  yb_index_check() is prone to 'Restart read required' erorrs when executed from within a transaction block.
+ yb_index_check 
+----------------
+ 
+(1 row)
+
+COMMIT;
 -- Test with more data
 INSERT INTO abcd SELECT i, i, i, i FROM generate_series(12, 2000) i;
 INSERT INTO abcd values (2002, 2002, 1, 2);

src/postgres/src/test/regress/expected/yb.orig.yb_index_check_1.out

@@ -304,6 +304,16 @@ SELECT yb_index_check('test_bytea_c_b_idx'::regclass::oid);
 
 (1 row)
 
+-- Inside a transaction block
+BEGIN;
+SELECT yb_index_check('abcd_b_c_d_idx'::regclass::oid);
+NOTICE:  yb_index_check() is prone to 'Restart read required' erorrs when executed from within a transaction block.
+ yb_index_check 
+----------------
+ 
+(1 row)
+
+COMMIT;
 -- Test with more data
 INSERT INTO abcd SELECT i, i, i, i FROM generate_series(12, 2000) i;
 INSERT INTO abcd values (2002, 2002, 1, 2);

src/postgres/src/test/regress/sql/yb.orig.yb_index_check.sql

@@ -187,6 +187,11 @@ INSERT INTO test_bytea (a, b, c)  VALUES (2, 42, 'test');
 SELECT yb_index_check('test_bytea_b_c_idx'::regclass::oid);
 SELECT yb_index_check('test_bytea_c_b_idx'::regclass::oid);
 
+-- Inside a transaction block
+BEGIN;
+SELECT yb_index_check('abcd_b_c_d_idx'::regclass::oid);
+COMMIT;
+
 -- Test with more data
 INSERT INTO abcd SELECT i, i, i, i FROM generate_series(12, 2000) i;
 INSERT INTO abcd values (2002, 2002, 1, 2);

src/yb/yql/pgwrapper/pg_yb_index_check-test.cc

@@ -78,5 +78,29 @@ TEST_F(PgYbIndexCheckTest, YbIndexCheckSnapshotTooOld) {
   ASSERT_OK((conn.Fetch("SELECT yb_index_check('abcd_b_c_d_idx'::regclass)")));
 }
 
+TEST_F(PgYbIndexCheckTest, YbIndexCheckRestartReadRequired) {
+  auto conn = ASSERT_RESULT(Connect());
+  int rowcount = 1000;
+  ASSERT_OK(conn.Execute("SET yb_max_query_layer_retries=0"));
+  ASSERT_OK(conn.Execute(
+      "CREATE TABLE abcd(a int primary key, b int, c int, d int) SPLIT INTO 1 TABLETS;"));
+  ASSERT_OK(conn.Execute("CREATE INDEX abcd_b_c_d_idx ON abcd (b ASC) INCLUDE (c, d)"));
+  ASSERT_OK(conn.ExecuteFormat(
+      "INSERT INTO abcd SELECT i, i, i, i FROM generate_series(1, $0) i", rowcount));
+
+  CountDownLatch latch(1);
+  TestThreadHolder holder;
+  holder.AddThreadFunctor([this, &stop = holder.stop_flag(), &latch, &rowcount] {
+    auto conn2 = ASSERT_RESULT(Connect());
+    latch.CountDown();
+    while (!stop.load()) {
+      CHECK_OK(conn2.ExecuteFormat("INSERT INTO abcd VALUES ($0, $0, $0, $0)", ++rowcount));
+    }
+  });
+
+  latch.Wait();
+  ASSERT_OK((conn.Fetch("SELECT yb_index_check('abcd_b_c_d_idx'::regclass)")));
+}
+
 } // namespace pgwrapper
 } // namespace yb