[#16408] YSQL: add gflag TEST_generate_ybrowid_sequentially

Summary:
Motivation: many ported regress tests have tables with no PK, and
upstream PG often does SELECTs with no ORDER BY on these tables.
Upstream PG's ordering is consistent in that rows allocate a ctid
sequentially.  YB, on the other hand, randomly generates a UUID for
ybrowid, and furthermore, tables with no PK are HASH sorted.

Fix those two differences via a new tserver gflag
TEST_generate_ybrowid_sequentially.  The ybrowids are generated using
MonoTime::Now() serialized to 8 bytes compared to the usual 16 byte
UUID.  This works for regress tests as regress tests normally do not
concurrently write to the same table or use connections across different
nodes with clock skew.

There is another unresolved difference between PG and YB's ctid/ybrowid
allocation: in PG, UPDATEs reallocate ctid.  Since doing the same would
be very intrusive to the YB model of UPDATEs, leave this out.  Most
tests do not do selective UPDATEs anyway, and for the few cases that do,
they can resort to using a ybsort column.

Update a handful of tests, particularly changing those using the ybsort
workaround to no longer use it.  Of particular note is yb_pg_with, which
still needs the ybsort column for some tables due to some UPDATEs.

Ideally, all ported regress tests turn on this flag.  But the current
state of things makes it hard to separate ported tests from non-ported
ones.  Put that work off for later, particularly the pg15 branch, where
this code is already in place in commit
fedbdac.

For here in master (currently based on PG 11), fix just the
TestPgRegressIndex test using this flag.  Split it to TestPgRegressIndex
and TestPgRegressPgIndex for non-ported and ported tests respectively.
For the ported test, set the flag.

Also note that yb_pg_indexing's output changes for an error/hint
message because the pk-less table now has ASC ybrowid instead of HASH
ybrowid.  This causes a new code path to get taken which ends up calling
RelationGetIndexList:

    ATRewriteTables > make_new_heap > yb_copy_split_options > YbRelationSetNewRelfileNode > YbGetSplitOptions > RelationGetPrimaryKeyIndex > RelationGetIndexList

So when entering FetchUniqueConstraintName for forming the message, it
hits the else block because rd_pkindex is loaded since
RelationIdGetRelation now finds the index in the scan unlike before.
This is undesirable behavior since it differs from upstream PG, but
dealing with it is out of scope.
Jira: DB-5819

Test Plan:
    ./yb_build.sh fastdebug --gcc11 --java-test TestPgRegressIndex
    ./yb_build.sh fastdebug --gcc11 --java-test TestPgRegressPgIndex \
      -n 10 --tp 1

Close: #16408

Reviewers: myang

Reviewed By: myang

Subscribers: tfoucher, yql

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

java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgRegressIndex.java

@@ -46,12 +46,7 @@ protected Map<String, String> getTServerFlags() {
   }
 
   @Test
-  public void testPgRegressIndex() throws Exception {
-    runPgRegressTest("yb_index_serial_schedule");
-  }
-
-  @Test
-  public void testPgRegressIndex2() throws Exception {
-    runPgRegressTest("yb_index_serial2_schedule");
+  public void schedule() throws Exception {
+    runPgRegressTest("yb_index_schedule");
   }
 }

java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgRegressPgIndex.java

@@ -0,0 +1,34 @@
+// Copyright (c) YugabyteDB, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+// in compliance with the License.  You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software distributed under the License
+// is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+// or implied.  See the License for the specific language governing permissions and limitations
+// under the License.
+//
+package org.yb.pgsql;
+
+import java.util.Map;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.yb.util.YBTestRunnerNonTsanOnly;
+
+@RunWith(value=YBTestRunnerNonTsanOnly.class)
+public class TestPgRegressPgIndex extends BasePgRegressTest {
+  @Override
+  protected Map<String, String> getTServerFlags() {
+    Map<String, String> flagMap = super.getTServerFlags();
+    flagMap.put("TEST_generate_ybrowid_sequentially", "true");
+    return flagMap;
+  }
+
+  @Test
+  public void schedule() throws Exception {
+    runPgRegressTest("yb_pg_index_schedule");
+  }
+}

src/postgres/src/backend/commands/ybccmds.c

@@ -776,10 +776,19 @@ YBCCreateTable(CreateStmt *stmt, char *tableName, char relkind, TupleDesc desc,
 		recordDependencyOn(&myself, &tablegroup, DEPENDENCY_NORMAL);
 	}
 
+	OptSplit *split_options = stmt->split_options;
 	bool is_sys_catalog_table = YbIsSysCatalogTabletRelationByIds(relationId,
 																  namespaceId,
 																  schema_name);
 	const bool is_tablegroup = OidIsValid(tablegroupId);
+	/*
+	 * Generate ybrowid ASC sequentially if the flag is on unless a SPLIT INTO
+	 * option is provided, in which case the author likely intended a HASH
+	 * partitioned table.
+	 */
+	const bool can_generate_ybrowid_sequentially =
+		(*YBCGetGFlags()->TEST_generate_ybrowid_sequentially &&
+		 !(split_options && split_options->split_type == NUM_TABLETS));
 	/*
 	 * The hidden ybrowid column is added when there is no primary key.  This
 	 * column is HASH or ASC sorted depending on certain criteria.
@@ -788,7 +797,7 @@ YBCCreateTable(CreateStmt *stmt, char *tableName, char relkind, TupleDesc desc,
 	if (primary_key)
 		ybrowid_mode = PG_YBROWID_MODE_NONE;
 	else if (is_colocated_via_database || is_tablegroup ||
-			 is_sys_catalog_table)
+			 is_sys_catalog_table || can_generate_ybrowid_sequentially)
 		ybrowid_mode = PG_YBROWID_MODE_RANGE;
 	else
 		ybrowid_mode = PG_YBROWID_MODE_HASH;
@@ -816,7 +825,6 @@ YBCCreateTable(CreateStmt *stmt, char *tableName, char relkind, TupleDesc desc,
 						  is_tablegroup);
 
 	/* Handle SPLIT statement, if present */
-	OptSplit *split_options = stmt->split_options;
 	if (split_options)
 	{
 		if (is_colocated_via_database)

src/postgres/src/test/regress/expected/yb_pg_indexing.out

@@ -1023,8 +1023,8 @@ drop index idxpart0_pkey;								-- fail
 ERROR:  cannot drop index idxpart0_pkey because constraint idxpart0_pkey on table idxpart0 requires it
 HINT:  You can drop constraint idxpart0_pkey on table idxpart0 instead.
 drop index idxpart1_pkey;								-- fail
-ERROR:  cannot drop index idxpart1_pkey because index idxpart_pkey requires it
-HINT:  You can drop index idxpart_pkey instead.
+ERROR:  cannot drop index idxpart1_pkey because constraint idxpart1_pkey on table idxpart1 requires it
+HINT:  You can drop constraint idxpart1_pkey on table idxpart1 instead.
 alter table idxpart0 drop constraint idxpart0_pkey;		-- fail
 ERROR:  cannot drop inherited constraint "idxpart0_pkey" of relation "idxpart0"
 alter table idxpart1 drop constraint idxpart1_pkey;		-- fail

src/postgres/src/test/regress/yb_index_schedule

@@ -0,0 +1,11 @@
+# src/test/regress/yb_index_schedule
+# This schedule is for non-ported tests only.
+test: yb_indexing_order
+test: yb_create_index
+test: yb_index_scan
+test: yb_reindex
+test: yb_secondary_index_scan
+
+test: yb_index_scan_null_create
+test: yb_index_scan_null_hash
+test: yb_index_scan_null_asc

src/postgres/src/test/regress/yb_pg_index_schedule

@@ -0,0 +1,3 @@
+# src/test/regress/yb_pg_index_schedule
+# This schedule is for ported tests only.
+test: yb_pg_indexing

src/yb/yql/pggate/pg_session.cc

@@ -61,6 +61,15 @@ DECLARE_int32(TEST_user_ddl_operation_timeout_sec);
 DEFINE_UNKNOWN_bool(ysql_log_failed_docdb_requests, false, "Log failed docdb requests.");
 DEFINE_test_flag(bool, ysql_ignore_add_fk_reference, false,
                  "Don't fill YSQL's internal cache for FK check to force read row from a table");
+DEFINE_test_flag(bool, generate_ybrowid_sequentially, false,
+                 "For new tables without PK, make the ybrowid column ASC and generated using a"
+                 " naive per-node sequential counter. This can fail with collisions for a"
+                 " multi-node cluster, and the ordering can be inconsistent in case multiple"
+                 " connections generate ybrowid at the same time. In case a SPLIT INTO clause is"
+                 " provided, fall back to the old behavior. The primary use case of this flag is"
+                 " for ported pg_regress tests that expect deterministic output ordering based on"
+                 " ctid. This is a best-effort reproduction of that, but it still falls short in"
+                 " case of UPDATEs because PG regenerates ctid while YB doesn't.");
 
 namespace yb::pggate {
 namespace {
@@ -707,6 +716,17 @@ bool PgSession::IsHashBatchingEnabled() {
       GetIsolationLevel() != PgIsolationLevel::SERIALIZABLE;
 }
 
+std::string PgSession::GenerateNewYbrowid() {
+  if (PREDICT_FALSE(FLAGS_TEST_generate_ybrowid_sequentially)) {
+    unsigned char buf[sizeof(uint64_t)];
+    BigEndian::Store64(buf, MonoTime::Now().ToUint64());
+    return std::string(reinterpret_cast<char*>(buf), sizeof(buf));
+  }
+
+  // Generate a new random and unique v4 UUID.
+  return GenerateObjectId(true /* binary_id */);
+}
+
 Result<bool> PgSession::IsInitDbDone() {
   return pg_client_.IsInitDbDone();
 }

src/yb/yql/pggate/pg_session.h

@@ -359,10 +359,7 @@ class PgSession : public RefCountedThreadSafe<PgSession> {
     connected_database_ = "";
   }
 
-  // Generate a new random and unique rowid. It is a v4 UUID.
-  std::string GenerateNewRowid() {
-    return GenerateObjectId(true /* binary_id */);
-  }
+  std::string GenerateNewYbrowid();
 
   void InvalidateAllTablesCache();
 

src/yb/yql/pggate/pggate.cc

@@ -473,7 +473,7 @@ Result<dockv::KeyBytes> PgApiImpl::TupleIdBuilder::Build(
     }
     if (attr->attr_num == to_underlying(PgSystemAttrNum::kYBRowId)) {
       SCHECK(new_row_id_holder.empty(), Corruption, "Multiple kYBRowId attribute detected");
-      new_row_id_holder = session->GenerateNewRowid();
+      new_row_id_holder = session->GenerateNewYbrowid();
       expr_pb->mutable_value()->ref_binary_value(new_row_id_holder);
     } else {
       const auto& collation_info = attr->collation_info;
@@ -499,6 +499,7 @@ Result<dockv::KeyBytes> PgApiImpl::TupleIdBuilder::Build(
     doc_key_.set_hash(VERIFY_RESULT(
         target_desc->partition_schema().PgsqlHashColumnCompoundValue(hashed_values)));
   }
+  VLOG(5) << "Built ybctid: " << doc_key_.ToString();
   return doc_key_.Encode();
 }
 

src/yb/yql/pggate/ybc_pg_typedefs.h

@@ -395,6 +395,7 @@ typedef struct PgGFlagsAccessor {
   const bool*     ysql_enable_pg_per_database_oid_allocator;
   const bool*     ysql_enable_db_catalog_version_mode;
   const bool*     TEST_ysql_hide_catalog_version_increment_log;
+  const bool*     TEST_generate_ybrowid_sequentially;
   const bool*     ysql_use_fast_backward_scan;
 } YBCPgGFlagsAccessor;
 

src/yb/yql/pggate/ybc_pggate.cc

@@ -117,6 +117,7 @@ DEFINE_NON_RUNTIME_bool(
     "Fill postgres' caches with system items only");
 
 DECLARE_bool(TEST_ash_debug_aux);
+DECLARE_bool(TEST_generate_ybrowid_sequentially);
 
 DECLARE_bool(use_fast_backward_scan);
 
@@ -1949,6 +1950,8 @@ const YBCPgGFlagsAccessor* YBCGetGFlags() {
           &FLAGS_ysql_enable_db_catalog_version_mode,
       .TEST_ysql_hide_catalog_version_increment_log =
           &FLAGS_TEST_ysql_hide_catalog_version_increment_log,
+      .TEST_generate_ybrowid_sequentially =
+          &FLAGS_TEST_generate_ybrowid_sequentially,
       .ysql_use_fast_backward_scan = &FLAGS_use_fast_backward_scan,
   };
   // clang-format on