[#24313] YSQL: test tidy up

Summary:
Follow up on 68f7de8 / D39430 to
address outstanding review comments.
Jira: DB-13202

Test Plan: ./yb_build.sh --cxx-test pgwrapper_pg_index_backfill-test --gtest_filter PgIndexBackfillTest.PhantomIdxEntry

Reviewers: jason

Reviewed By: jason

Subscribers: yql

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

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

@@ -2300,8 +2300,23 @@ class PgIndexBackfillReadCommitted : public PgIndexBackfillTest {
 
 // Test for https://github.com/yugabyte/yugabyte-db/issues/24313
 // Verify that concurrent updates do not leave phantom entries in the index
+// Phantom entries were created because inplace index update function did not check if the index
+// was ready. The index build procedure wait until "old" transactions (those who don't know about
+// the index) are completed before proceeding to "ready" state because update by old transaction may
+// prevent proper index update. This happens as follows ("new" transaction thinks the index is
+// ready, "old" transaction thinks the index does not exist):
+// - "new" transaction changes value in indexed column of record R from 'a' to 'b'.
+//   Index key is changed from 'a' to 'b'. Index is being built, the 'a' key may not exist yet, in
+//   this case new key is created.
+// - "old" transaction changes value in indexed column of record R from 'b' to 'c'.
+//   Index key is not updated, it remains 'b'.
+// - "new" transaction changes value in indexed column of record R from 'c' to 'd'.
+//   The transaction attempts to change the index key from 'c' to 'd'. The key 'c' does not exist
+//   in the index, so new key is created. Now index has two keys, 'b' and 'd' pointing to the same
+//   record R.
+// If index readiness is not checked, any transaction aware of the index acts as "new".
 TEST_F_EX(PgIndexBackfillTest, PhantomIdxEntry, PgIndexBackfillReadCommitted) {
-  constexpr int64_t kNumRows = 10;
+  constexpr int kNumRows = 10;
   const IndexStateFlags index_live_flags{IndexStateFlag::kIndIsLive};
   ASSERT_OK(conn_->ExecuteFormat("CREATE TABLE $0 (i int, t text)", kTableName));
   ASSERT_OK(conn_->ExecuteFormat("INSERT INTO $0 VALUES (generate_series(1, $1), 'a')",
@@ -2315,42 +2330,30 @@ TEST_F_EX(PgIndexBackfillTest, PhantomIdxEntry, PgIndexBackfillReadCommitted) {
   });
   // There's no reliable indicator that index build has stopped before 'indislive' phase, just give
   // the index creation thread some extra time.
-  SleepFor(MonoDelta::FromMilliseconds(RegularBuildVsSanitizers(5000, 60000)));
+  SleepFor(RegularBuildVsSanitizers(5s, 60s));
   auto other_conn = ASSERT_RESULT(ConnectToDB(kDatabaseName));
-  // The index shouldn't be visible
   ASSERT_FALSE(ASSERT_RESULT(IsAtTargetIndexStateFlags(kIndexName, IndexStateFlags{})));
-  // Start transaction and make sure it is in progress
   LOG(INFO) << "Begin older txn";
   ASSERT_OK(other_conn.Execute("BEGIN"));
   const std::string query = Format("SELECT t FROM $0 WHERE i = $1", kTableName, 1);
   auto rows = ASSERT_RESULT((other_conn.FetchRows<std::string>(query)));
   ASSERT_EQ(rows, (decltype(rows){{"a"}}));
-  // Allow index build to proceed to the next phase
   ASSERT_OK(cluster_->SetFlagOnTServers("ysql_yb_test_block_index_phase", "indisready"));
   ASSERT_OK(WaitForIndexStateFlags(index_live_flags, kIndexName));
-  // New transaction can see the index and add 'b' into it
   LOG(INFO) << "Update record by newer txn";
   ASSERT_OK(conn_->ExecuteFormat("UPDATE $0 SET t = 'b' WHERE i = $1", kTableName, 2));
-  // If old transaction use cached metadata, it does not see index, so don't replace 'b' with 'c'
-  // index record b becomes phantom
   LOG(INFO) << "Update record by older txn";
   ASSERT_OK(other_conn.ExecuteFormat("UPDATE $0 SET t = 'c' WHERE i = $1", kTableName, 2));
   ASSERT_OK(other_conn.Execute("COMMIT"));
-  // New transaction attempts to replace 'c' with 'd' but if there's the phantom record, it only
-  // inserts 'd'
   LOG(INFO) << "Update record by newer txn again";
   ASSERT_OK(conn_->ExecuteFormat("UPDATE $0 SET t = 'd' WHERE i = $1", kTableName, 2));
   ASSERT_TRUE(ASSERT_RESULT(IsAtTargetIndexStateFlags(kIndexName, index_live_flags)));
-  // Make sure catalog versions differ
-  // Complete the index build
   ASSERT_OK(cluster_->SetFlagOnTServers("ysql_yb_test_block_index_phase", "none"));
   thread_holder_.Stop();
-  // Check rowcount
   const std::string count_query = Format("SELECT count(t) FROM $0 WHERE t = 'b'", kTableName);
   ASSERT_OK(WaitForIndexScan(count_query));
   LOG(INFO) << "Check for phantom record";
   auto idx_count = ASSERT_RESULT((conn_->FetchRow<PGUint64>(count_query)));
-  // Phantom entry makes count 1
   ASSERT_EQ(idx_count, 0);
 }