[#21230] YSQL: Fix create index error "Requested catalog version is t…

…oo high"

Summary:
Recent commit a76f9e6 introduced DDL support
during the tryout phase in the new cluster upgrade work flow. When upgrading to
a release that has the gflag `--ysql_enable_db_catalog_version_mode` default on,
during the tryout phase, the table `pg_yb_catalog_version` is still in
global-mode: it has only one row for database `template`.

The above commit introduced a regression:

What happened is that yb-master used to only rely on the gflag
`--ysql_enable_db_catalog_version_mode=true` to decide whether it operates in
perdb mode or not. After the above change, it also reads the
`pg_yb_catalog_version` table to see if it has more than 1 rows. Only when both
conditions are met, master will start to operate in perdb mode. But it is
reading `pg_yb_catalog_version` as part of heartbeat response message
preparation. Let’s say after a new master leader is elected, before it receives
the very first heartbeat request, it receives a request to get the current
catalog version operation mode: its `catalog_version_table_in_perdb_mode_` is
initialized to `false` so it will return `false`. But that’s not right, it
should be unknown or return an error, because the value isn’t set yet. Without
doing a just-in-time read of `pg_yb_catalog_version` the master does not know
which mode it should return. Because it returns false, the caller will think
this new master leader still operates in global catalog version mode. In global
catalog version mode it will read the current version from `template1` in
`pg_yb_catalog_version`, that’s incorrect because in reality
`pg_yb_catalog_version` has multiple rows already.

When a cluster already runs in perdb mode, there should not be any initial
moment when the master **thinks** it runs in global mode and later switch to perdb
mode when it receives a tserver heartbeat request.

Same issue also exists in tserver and PG backend. In tserver, when a tserver is
restarted, it may not have received heartbeat response from master yet to
properly initialize its `catalog_version_table_in_perdb_mode_`. Likewise, PG
backend should wait for the `catalog_version_table_in_perdb_mode_` in shared
memory to properly initialize.

At master side, I fixed the bug by adding a new function that will do on-demand read of
`pg_yb_catalog_version` table when --ysql_enable_db_catalog_version_mode=true and `catalog_version_table_in_perdb_mode_` is still
false (`CatalogManager::IsCatalogVersionTableInPerdbMode`). Once
`catalog_version_table_in_perdb_mode_` is set to true, it remains so and will
not be affected even if the table `pg_yb_catalog_version` is later changed back
to global mode. For tserver and PG backend, I changed
`catalog_version_table_in_perdb_mode_` from bool to std::optional<bool> so that
we can tell whether the variable has been properly initialized.

Added a new unit test which has a TEST gflag.
Jira: DB-10157

Test Plan:
(1) ./yb_build.sh --cxx-test pg_catalog_version-test
(2) Verify that when the new unit test (and the new TEST gflag) is run without
the fix, it would fail with an error like:
```
Bad status: Network error (yb/yql/pgwrapper/libpq_utils.cc:410): Execute of 'CREATE INDEX idx ON t(id)' failed: 7, message: ERROR:  Requested catalog version is too high: req version 3, master version 1 (pgsql error XX000) (aux msg ERROR:  Requested catalog version is too high: req version 3, master version 1)
```
(3) With QA help, ran the test that reported this bug on two clusters and
verified the fix successfully.

Reviewers: jason, tverona

Reviewed By: jason

Subscribers: yql, ybase, bogdan

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

src/yb/master/catalog_manager.cc

@@ -639,6 +639,8 @@ DEFINE_RUNTIME_uint32(maximum_tablet_leader_lease_expired_secs, 2 * 60,
     "If the leader lease in master's view has expired for this amount of seconds, "
     "treat the lease as expired for too long time.");
 
+DEFINE_test_flag(bool, disable_set_catalog_version_table_in_perdb_mode, false,
+                 "Whether to disable setting the catalog version table in perdb mode.");
 namespace yb {
 namespace master {
 
@@ -10404,12 +10406,39 @@ Status CatalogManager::GetYsqlAllDBCatalogVersions(
     *fingerprint = FingerprintCatalogVersions<DbOidToCatalogVersionMap>(*versions);
     VLOG_WITH_FUNC(2) << "databases: " << versions->size() << ", fingerprint: " << *fingerprint;
   }
-  if (versions->size() > 1) {
+  if (versions->size() > 1 && !FLAGS_TEST_disable_set_catalog_version_table_in_perdb_mode) {
+    LOG(INFO) << "set catalog_version_table_in_perdb_mode_ to true";
     catalog_version_table_in_perdb_mode_ = true;
   }
   return Status::OK();
 }
 
+// When a cluster is running in per-database catalog version mode, normally
+// catalog_version_table_in_perdb_mode_ is set to true as part of preparing for
+// tserver heartbeat response and once set to true it is never reset back to
+// false. In case a new leader master has not received any tserver heartbeat
+// request, then catalog_version_table_in_perdb_mode_ still has its initial
+// value false, but we cannot assume that the table pg_yb_catalog_version is
+// still in global mode. That's why this function does an on-demand reading of
+// pg_yb_catalog_version table to find out.
+Status CatalogManager::IsCatalogVersionTableInPerdbMode(bool* perdb_mode) {
+  DCHECK(FLAGS_ysql_enable_db_catalog_version_mode);
+  if (!catalog_version_table_in_perdb_mode_) {
+    DbOidToCatalogVersionMap versions;
+    RETURN_NOT_OK(GetYsqlAllDBCatalogVersions(
+        false /* use_cache */, &versions, nullptr /* fingerprint */));
+    // If FLAGS_TEST_disable_set_catalog_version_table_in_perdb_mode is set, for
+    // unit test purpose, we return perdb_mode properly while leaving
+    // catalog_version_table_in_perdb_mode_ not set.
+    if (FLAGS_TEST_disable_set_catalog_version_table_in_perdb_mode) {
+      *perdb_mode = versions.size() > 1;
+      return Status::OK();
+    }
+  }
+  *perdb_mode = catalog_version_table_in_perdb_mode_;
+  return Status::OK();
+}
+
 Status CatalogManager::InitializeTransactionTablesConfig(int64_t term) {
   SysTransactionTablesConfigEntryPB transaction_tables_config;
   transaction_tables_config.set_version(0);

src/yb/master/catalog_manager.h

@@ -681,9 +681,8 @@ class CatalogManager : public tserver::TabletPeerLookupIf,
       EXCLUDES(heartbeat_pg_catalog_versions_cache_mutex_);
   Status GetYsqlDBCatalogVersion(
       uint32_t db_oid, uint64_t* catalog_version, uint64_t* last_breaking_version) override;
-  bool catalog_version_table_in_perdb_mode() const override {
-    return catalog_version_table_in_perdb_mode_;
-  }
+
+  Status IsCatalogVersionTableInPerdbMode(bool* perdb_mode);
 
   Status InitializeTransactionTablesConfig(int64_t term);
 

src/yb/master/catalog_manager_if.h

@@ -127,7 +127,6 @@ class CatalogManagerIf {
       uint64_t* fingerprint) = 0;
   virtual Status GetYsqlDBCatalogVersion(
       uint32_t db_oid, uint64_t* catalog_version, uint64_t* last_breaking_version) = 0;
-  virtual bool catalog_version_table_in_perdb_mode() const = 0;
 
   virtual Status GetClusterConfig(GetMasterClusterConfigResponsePB* resp) = 0;
   virtual Status GetClusterConfig(SysClusterConfigEntryPB* config) = 0;

src/yb/master/master_tserver.cc

@@ -148,10 +148,6 @@ void MasterTabletServer::get_ysql_db_catalog_version(uint32_t db_oid,
   }
 }
 
-bool MasterTabletServer::catalog_version_table_in_perdb_mode() const {
-  return master_->catalog_manager()->catalog_version_table_in_perdb_mode();
-}
-
 tserver::TServerSharedData& MasterTabletServer::SharedObject() {
   return master_->shared_object();
 }

src/yb/master/master_tserver.h

@@ -58,8 +58,6 @@ class MasterTabletServer : public tserver::TabletServerIf,
   void get_ysql_db_catalog_version(uint32_t db_oid,
                                    uint64_t* current_version,
                                    uint64_t* last_breaking_version) const override;
-  bool catalog_version_table_in_perdb_mode() const override;
-
   Status get_ysql_db_oid_to_cat_version_info_map(
       const tserver::GetTserverCatalogVersionInfoRequestPB& req,
       tserver::GetTserverCatalogVersionInfoResponsePB *resp) const override;

src/yb/master/ysql_backends_manager.cc

@@ -133,10 +133,13 @@ Status YsqlBackendsManager::WaitForYsqlBackendsCatalogVersion(
       RETURN_NOT_OK(CheckLeadership(&l, resp));
     }
     Status s;
+    bool perdb_mode = false;
     // TODO(jason): using the gflag to determine per-db mode may not work for initdb, so make sure
     // to handle that case if initdb ever goes through this codepath.
-    if (FLAGS_ysql_enable_db_catalog_version_mode &&
-        master_->catalog_manager_impl()->catalog_version_table_in_perdb_mode()) {
+    if (FLAGS_ysql_enable_db_catalog_version_mode) {
+      RETURN_NOT_OK(master_->catalog_manager_impl()->IsCatalogVersionTableInPerdbMode(&perdb_mode));
+    }
+    if (perdb_mode) {
       s = master_->catalog_manager_impl()->GetYsqlDBCatalogVersion(
           db_oid, &master_version, nullptr /* last_breaking_version */);
     } else {

src/yb/tserver/tablet_server.cc

@@ -913,8 +913,22 @@ void TabletServer::SetYsqlDBCatalogVersions(
                                                  .last_breaking_version = new_breaking_version,
                                                  .shm_index = -1})));
     if (ysql_db_catalog_version_map_.size() > 1) {
-      catalog_version_table_in_perdb_mode_ = true;
-      shared_object().SetCatalogVersionTableInPerdbMode();
+      if (!catalog_version_table_in_perdb_mode_.has_value() ||
+          !catalog_version_table_in_perdb_mode_.value()) {
+        LOG(INFO) << "set pg_yb_catalog_version table in perdb mode";
+        catalog_version_table_in_perdb_mode_ = true;
+        shared_object().SetCatalogVersionTableInPerdbMode(true);
+      }
+    } else {
+      DCHECK_EQ(ysql_db_catalog_version_map_.size(), 1);
+      if (!catalog_version_table_in_perdb_mode_.has_value()) {
+        // We can initialize to false at most one time. Once set,
+        // catalog_version_table_in_perdb_mode_ can only go from false to
+        // true (i.e., from global mode to perdb mode).
+        LOG(INFO) << "set pg_yb_catalog_version table in global mode";
+        catalog_version_table_in_perdb_mode_ = false;
+        shared_object().SetCatalogVersionTableInPerdbMode(false);
+      }
     }
     bool row_inserted = it.second;
     bool row_updated = false;

src/yb/tserver/tablet_server.h

@@ -243,7 +243,7 @@ class TabletServer : public DbServerBase, public TabletServerIf {
     }
   }
 
-  bool catalog_version_table_in_perdb_mode() const override {
+  std::optional<bool> catalog_version_table_in_perdb_mode() const {
     std::lock_guard l(lock_);
     return catalog_version_table_in_perdb_mode_;
   }
@@ -409,7 +409,7 @@ class TabletServer : public DbServerBase, public TabletServerIf {
   uint64_t ysql_last_breaking_catalog_version_ = 0;
   tserver::DbOidToCatalogVersionInfoMap ysql_db_catalog_version_map_;
   // See same variable comments in CatalogManager.
-  bool catalog_version_table_in_perdb_mode_ = false;
+  std::optional<bool> catalog_version_table_in_perdb_mode_{std::nullopt};
 
   // Fingerprint of the catalog versions map.
   std::atomic<std::optional<uint64_t>> catalog_versions_fingerprint_;

src/yb/tserver/tablet_server_interface.h

@@ -55,7 +55,6 @@ class TabletServerIf : public LocalTabletServer {
   virtual void get_ysql_db_catalog_version(uint32_t db_oid,
                                            uint64_t* current_version,
                                            uint64_t* last_breaking_version) const = 0;
-  virtual bool catalog_version_table_in_perdb_mode() const = 0;
 
   virtual Status get_ysql_db_oid_to_cat_version_info_map(
       const tserver::GetTserverCatalogVersionInfoRequestPB& req,

src/yb/tserver/tablet_service.cc

@@ -1977,8 +1977,18 @@ void TabletServiceAdminImpl::WaitForYsqlBackendsCatalogVersion(
       [catalog_version, database_oid, this, &ts_catalog_version]() -> Result<bool> {
         // TODO(jason): using the gflag to determine per-db mode may not work for initdb, so make
         // sure to handle that case if initdb ever goes through this codepath.
-        if (FLAGS_ysql_enable_db_catalog_version_mode &&
-            server_->catalog_version_table_in_perdb_mode()) {
+        bool perdb_mode = false;
+        if (FLAGS_ysql_enable_db_catalog_version_mode) {
+          const std::optional<bool> catalog_version_table_in_perdb_mode =
+            server_->catalog_version_table_in_perdb_mode();
+          if (!catalog_version_table_in_perdb_mode.has_value()) {
+            // This is a temporary known case when this tserver hasn't get the answer
+            // from master yet via heartbeat response.
+            return false;
+          }
+          perdb_mode = catalog_version_table_in_perdb_mode.value();
+        }
+        if (perdb_mode) {
           server_->get_ysql_db_catalog_version(
               database_oid, &ts_catalog_version, nullptr /* last_breaking_catalog_version */);
         } else {

src/yb/tserver/tserver_shared_mem.h

@@ -85,11 +85,11 @@ class TServerSharedData {
     return db_catalog_versions_[index].load(std::memory_order_acquire);
   }
 
-  void SetCatalogVersionTableInPerdbMode() {
-    catalog_version_table_in_perdb_mode_.store(true, std::memory_order_release);
+  void SetCatalogVersionTableInPerdbMode(bool perdb_mode) {
+    catalog_version_table_in_perdb_mode_.store(perdb_mode, std::memory_order_release);
   }
 
-  bool catalog_version_table_in_perdb_mode() const {
+  std::optional<bool> catalog_version_table_in_perdb_mode() const {
     return catalog_version_table_in_perdb_mode_.load(std::memory_order_acquire);
   }
 
@@ -121,7 +121,7 @@ class TServerSharedData {
 
   std::atomic<uint64_t> db_catalog_versions_[kMaxNumDbCatalogVersions] = {0};
   // See same variable comments in CatalogManager.
-  std::atomic<bool> catalog_version_table_in_perdb_mode_{false};
+  std::atomic<std::optional<bool>> catalog_version_table_in_perdb_mode_{std::nullopt};
 };
 
 YB_STRONGLY_TYPED_BOOL(Create);

src/yb/yql/pggate/pggate.cc

@@ -1928,7 +1928,26 @@ Result<uint32_t> PgApiImpl::GetNumberOfDatabases() {
 }
 
 Result<bool> PgApiImpl::CatalogVersionTableInPerdbMode() {
-  return tserver_shared_object_->catalog_version_table_in_perdb_mode();
+  DCHECK(FLAGS_ysql_enable_db_catalog_version_mode);
+  if (!tserver_shared_object_->catalog_version_table_in_perdb_mode().has_value()) {
+    // If this tserver has just restarted, it may not have received any
+    // heartbeat response from yb-master that has set a value in
+    // catalog_version_table_in_perdb_mode_ in the shared memory object
+    // yet. Let's wait with 500ms interval until a value is set or until
+    // a 10-second timeout.
+    auto status = LoggedWaitFor(
+        [this]() -> Result<bool> {
+          return tserver_shared_object_->catalog_version_table_in_perdb_mode().has_value();
+        },
+        10s /* timeout */,
+        "catalog_version_table_in_perdb_mode is not set shared memory",
+        500ms /* initial_delay */,
+        1.0 /* delay_multiplier */);
+    RETURN_NOT_OK_PREPEND(
+        status,
+        "Failed to find out pg_yb_catalog_version mode");
+  }
+  return tserver_shared_object_->catalog_version_table_in_perdb_mode().value();
 }
 
 uint64_t PgApiImpl::GetSharedAuthKey() const {

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

@@ -1324,6 +1324,32 @@ TEST_F(PgCatalogVersionTest, SimulateLaggingPGInUpgradeFinalization) {
   ASSERT_STR_CONTAINS(status.ToString(), msg);
 }
 
+class PgCatalogVersionMasterLeadershipChange : public PgCatalogVersionTest {
+ protected:
+  int GetNumMasters() const override { return 3; }
+};
+
+TEST_F_EX(PgCatalogVersionTest, ChangeMasterLeadership,
+          PgCatalogVersionMasterLeadershipChange) {
+  auto conn_yugabyte = ASSERT_RESULT(Connect());
+  ASSERT_OK(PrepareDBCatalogVersion(&conn_yugabyte, true /* per_database_mode */));
+  RestartClusterWithDBCatalogVersionMode();
+  WaitForCatalogVersionToPropagate();
+  conn_yugabyte = ASSERT_RESULT(Connect());
+  ASSERT_OK(conn_yugabyte.Execute("CREATE TABLE t(id INT)"));
+  ASSERT_OK(conn_yugabyte.Execute("ALTER TABLE t ADD COLUMN c2 TEXT"));
+  LOG(INFO) << "Disable next master leader to set catalog version table in perdb mode";
+  ASSERT_OK(cluster_->SetFlagOnMasters(
+      "TEST_disable_set_catalog_version_table_in_perdb_mode", "true"));
+  auto leader_master_index = CHECK_RESULT(cluster_->GetLeaderMasterIndex());
+  LOG(INFO) << "Failing over master leader.";
+  ASSERT_OK(cluster_->StepDownMasterLeaderAndWaitForNewLeader());
+  auto new_leader_master_index = CHECK_RESULT(cluster_->GetLeaderMasterIndex());
+  LOG(INFO) << "The new master leader is at " << leader_master_index;
+  CHECK_NE(leader_master_index, new_leader_master_index);
+  ASSERT_OK(conn_yugabyte.Execute("CREATE INDEX idx ON t(id)"));
+}
+
 TEST_F(PgCatalogVersionTest, SqlCrossDBLoadWithDDL) {
 
   const std::vector<std::vector<string>> ddlLists = {