From a76f9e6227417e2647d34389b5d1a3ac202ed43e Mon Sep 17 00:00:00 2001 From: Minghui Yang Date: Thu, 8 Feb 2024 23:11:58 +0000 Subject: [PATCH] [#20300] YSQL: Fix CREATE INDEX failure in upgrading to perdb catalog version mode Summary: To reproduce the bug, create a universe with `ysql_enable_db_catalog_version_mode=true` and `allowed_preview_flags_csv=ysql_enable_db_catalog_version_mode`. Do NOT execute the following ``` SET yb_non_ddl_txn_for_sys_tables_allowed=true; SELECT yb_fix_catalog_version_table(true); ``` Run the next test ``` yugabyte=# create table t(id int); CREATE TABLE yugabyte=# insert into t values(1); INSERT 0 1 yugabyte=# CREATE TABLE tempTable AS SELECT * FROM t limit 1000000; SELECT 1 yugabyte=# CREATE INDEX idx2 ON tempTable(id); NOTICE: Retrying wait for backends catalog version: Requested catalog version is too high: req version 2, master version 0 NOTICE: Retrying wait for backends catalog version: Requested catalog version is too high: req version 2, master version 0 NOTICE: Retrying wait for backends catalog version: Requested catalog version is too high: req version 2, master version 0 NOTICE: Retrying wait for backends catalog version: Requested catalog version is too high: req version 2, master version 0 ERROR: Requested catalog version is too high: req version 2, master version 0 ``` This configuration has an mismatch between --ysql_enable_db_catalog_version_mode and pg_yb_catalog_version: --ysql_enable_db_catalog_version_mode=true but pg_yb_catalog_version is still in global-catalog version mode (only has one row for database template1). This is a special configuration that can legitimately happen during cluster upgrade to a new release that has the per-database catalog version mode on by default. The new binary executables are upgraded first which will have --ysql_enable_db_catalog_version_mode=true, but the YSQL migration script will be executed at a later time in the finalization phase of the upgrade process. There is a tryout phase before the finalization phase to allow possible rolling back of the upgrade. The tryout phase can last for an extended period of time (e.g., 1 month). Therefore we need to support DDL statements during the tryout phase when the gflag is on but the table pg_yb_catalog_version isn't upgraded yet. However we should still advise customers not to run any DDL statements during the finalization phase which includes running the YSQL migration scripts (at present this is not enforced). I added a variable `catalog_version_table_in_perdb_mode_` that is used to cache the state of pg_yb_catalog_version: whether it has been upgraded to have one row per database. The intention is to only support rolling upgrade to per-database catalog version mode (not downgrade back to global catalog version mode). Therefore once set to true, the variable `catalog_version_table_in_perdb_mode_` is never reset back to false. This variable is introduced in both yb-master and yb-tserver. Normally PG request carries the catalog version mode as part of the RPC arguments (`ysql_catalog_version` vs `ysql_db_catalog_version`) and yb-tserver will respect that. Before this fix, when no PG request is available, yb-tserver only uses --ysql_enable_db_catalog_version_mode to tell which catalog version mode to use. After this fix, only when both the gflag --ysql_enable_db_catalog_version_mode and `catalog_version_table_in_perdb_mode_` are true, yb-tserver operates in per-database catalog version mode. This fixes the CREATE INDEX failure. The variable `catalog_version_table_in_perdb_mode_` is also stored in shared memory to allow fast PG access to improve the performance of `YBIsDBCatalogVersionMode()` by replacing `YbGetNumberOfDatabases() > 1)` so we avoid a local RPC call during the tryout phase. Jira: DB-9266 Test Plan: ./yb_build.sh --cxx-test pg_catalog_version-test Reviewers: jason, tverona Reviewed By: jason Subscribers: ybase, yql, bogdan Differential Revision: https://phorge.dev.yugabyte.com/D32299 --- .../src/backend/utils/misc/pg_yb_utils.c | 9 ++++- src/postgres/src/include/pg_yb_utils.h | 1 + src/yb/master/catalog_manager.cc | 5 ++- src/yb/master/catalog_manager.h | 17 ++++++++++ src/yb/master/catalog_manager_if.h | 1 + src/yb/master/master_tserver.cc | 4 +++ src/yb/master/master_tserver.h | 1 + src/yb/master/ysql_backends_manager.cc | 3 +- src/yb/tserver/tablet_server.cc | 4 +++ src/yb/tserver/tablet_server.h | 7 ++++ src/yb/tserver/tablet_server_interface.h | 1 + src/yb/tserver/tablet_service.cc | 3 +- src/yb/tserver/tserver_shared_mem.h | 10 ++++++ src/yb/yql/pggate/pggate.cc | 4 +++ src/yb/yql/pggate/pggate.h | 1 + src/yb/yql/pggate/ybc_pggate.cc | 4 +++ src/yb/yql/pggate/ybc_pggate.h | 4 +++ .../yql/pgwrapper/pg_catalog_version-test.cc | 34 ++++++++++++++----- 18 files changed, 101 insertions(+), 12 deletions(-) diff --git a/src/postgres/src/backend/utils/misc/pg_yb_utils.c b/src/postgres/src/backend/utils/misc/pg_yb_utils.c index c28d34fd294d..f983415c37ea 100644 --- a/src/postgres/src/backend/utils/misc/pg_yb_utils.c +++ b/src/postgres/src/backend/utils/misc/pg_yb_utils.c @@ -639,7 +639,7 @@ YBCanEnableDBCatalogVersionMode() * upgrade, the pg_yb_catalog_version is transactionally updated * to have one row per database. */ - return (YbGetNumberOfDatabases() > 1); + return YbCatalogVersionTableInPerdbMode(); } /* @@ -4020,6 +4020,13 @@ uint32_t YbGetNumberOfDatabases() return num_databases; } +bool YbCatalogVersionTableInPerdbMode() +{ + bool perdb_mode = false; + HandleYBStatus(YBCCatalogVersionTableInPerdbMode(&perdb_mode)); + return perdb_mode; +} + static bool yb_is_batched_execution = false; bool YbIsBatchedExecution() diff --git a/src/postgres/src/include/pg_yb_utils.h b/src/postgres/src/include/pg_yb_utils.h index 3ea18897ab78..0647a5dafb22 100644 --- a/src/postgres/src/include/pg_yb_utils.h +++ b/src/postgres/src/include/pg_yb_utils.h @@ -907,6 +907,7 @@ void YbSetCatalogCacheVersion(YBCPgStatement handle, uint64_t version); uint64_t YbGetSharedCatalogVersion(); uint32_t YbGetNumberOfDatabases(); +bool YbCatalogVersionTableInPerdbMode(); /* * This function maps the user intended row-level lock policy i.e., "pg_wait_policy" of diff --git a/src/yb/master/catalog_manager.cc b/src/yb/master/catalog_manager.cc index d8bb8fa1a49a..e723d4a0ddce 100644 --- a/src/yb/master/catalog_manager.cc +++ b/src/yb/master/catalog_manager.cc @@ -10355,7 +10355,7 @@ Status CatalogManager::GetYsqlAllDBCatalogVersionsImpl(DbOidToCatalogVersionMap* Status CatalogManager::GetYsqlAllDBCatalogVersions( bool use_cache, DbOidToCatalogVersionMap* versions, uint64_t* fingerprint) { DCHECK(FLAGS_ysql_enable_db_catalog_version_mode); - if (use_cache) { + if (use_cache && catalog_version_table_in_perdb_mode_) { SharedLock lock(heartbeat_pg_catalog_versions_cache_mutex_); // We expect that the only caller uses this cache is the heartbeat service. // It is ok for heartbeat_pg_catalog_versions_cache_ to be empty: the @@ -10381,6 +10381,9 @@ Status CatalogManager::GetYsqlAllDBCatalogVersions( *fingerprint = FingerprintCatalogVersions(*versions); VLOG_WITH_FUNC(2) << "databases: " << versions->size() << ", fingerprint: " << *fingerprint; } + if (versions->size() > 1) { + catalog_version_table_in_perdb_mode_ = true; + } return Status::OK(); } diff --git a/src/yb/master/catalog_manager.h b/src/yb/master/catalog_manager.h index fb1e60114f44..1cce2dd21e2a 100644 --- a/src/yb/master/catalog_manager.h +++ b/src/yb/master/catalog_manager.h @@ -689,6 +689,9 @@ 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 InitializeTransactionTablesConfig(int64_t term); @@ -3215,6 +3218,20 @@ class CatalogManager : public tserver::TabletPeerLookupIf, std::atomic pg_catalog_versions_bg_task_running_ = {false}; rpc::ScheduledTaskTracker refresh_ysql_pg_catalog_versions_task_; + // For per-database catalog version mode upgrade support: when the gflag + // --ysql_enable_db_catalog_version_mode is true, whether the table + // pg_yb_catalog_version has been upgraded to have one row per database. + // During upgrade the binaries are installed first but before YSQL migration + // script is run pg_yb_catalog_version only has one row for template1. + // YB Note: + // (1) Each time we read the entire pg_yb_catalog_version table if the number + // of rows is > 1 we assume that the table has exactly one row per database. + // (2) This is only used to support per-database catalog version mode upgrade. + // Once set it is never reset back to false. It is an error to change + // pg_yb_catalog_version back to global catalog version mode when + // --ysql_enable_db_catalog_version_mode=true. + std::atomic catalog_version_table_in_perdb_mode_ = false; + // mutex on heartbeat_pg_catalog_versions_cache_ mutable MutexType heartbeat_pg_catalog_versions_cache_mutex_; std::optional heartbeat_pg_catalog_versions_cache_ diff --git a/src/yb/master/catalog_manager_if.h b/src/yb/master/catalog_manager_if.h index 6dad42ddb863..576a73d3b205 100644 --- a/src/yb/master/catalog_manager_if.h +++ b/src/yb/master/catalog_manager_if.h @@ -126,6 +126,7 @@ 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; diff --git a/src/yb/master/master_tserver.cc b/src/yb/master/master_tserver.cc index 763ac712f506..f7fe00b9031e 100644 --- a/src/yb/master/master_tserver.cc +++ b/src/yb/master/master_tserver.cc @@ -148,6 +148,10 @@ 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(); } diff --git a/src/yb/master/master_tserver.h b/src/yb/master/master_tserver.h index 3c1c24479d4e..9e7c2ff7c35e 100644 --- a/src/yb/master/master_tserver.h +++ b/src/yb/master/master_tserver.h @@ -58,6 +58,7 @@ 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, diff --git a/src/yb/master/ysql_backends_manager.cc b/src/yb/master/ysql_backends_manager.cc index 5b9d08ada19f..b3881dc8cd36 100644 --- a/src/yb/master/ysql_backends_manager.cc +++ b/src/yb/master/ysql_backends_manager.cc @@ -132,7 +132,8 @@ Status YsqlBackendsManager::WaitForYsqlBackendsCatalogVersion( Status s; // 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) { + if (FLAGS_ysql_enable_db_catalog_version_mode && + master_->catalog_manager_impl()->catalog_version_table_in_perdb_mode()) { s = master_->catalog_manager_impl()->GetYsqlDBCatalogVersion( db_oid, &master_version, nullptr /* last_breaking_version */); } else { diff --git a/src/yb/tserver/tablet_server.cc b/src/yb/tserver/tablet_server.cc index 07320be6ced1..e504f8d3aadf 100644 --- a/src/yb/tserver/tablet_server.cc +++ b/src/yb/tserver/tablet_server.cc @@ -912,6 +912,10 @@ void TabletServer::SetYsqlDBCatalogVersions( std::make_pair(db_oid, CatalogVersionInfo({.current_version = new_version, .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(); + } bool row_inserted = it.second; bool row_updated = false; int shm_index = -1; diff --git a/src/yb/tserver/tablet_server.h b/src/yb/tserver/tablet_server.h index 188132dd589f..c47458a51c35 100644 --- a/src/yb/tserver/tablet_server.h +++ b/src/yb/tserver/tablet_server.h @@ -243,6 +243,11 @@ class TabletServer : public DbServerBase, public TabletServerIf { } } + bool catalog_version_table_in_perdb_mode() const override { + std::lock_guard l(lock_); + return catalog_version_table_in_perdb_mode_; + } + Status get_ysql_db_oid_to_cat_version_info_map( const tserver::GetTserverCatalogVersionInfoRequestPB& req, tserver::GetTserverCatalogVersionInfoResponsePB* resp) const override; @@ -403,6 +408,8 @@ class TabletServer : public DbServerBase, public TabletServerIf { uint64_t ysql_catalog_version_ = 0; 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; // Fingerprint of the catalog versions map. std::atomic> catalog_versions_fingerprint_; diff --git a/src/yb/tserver/tablet_server_interface.h b/src/yb/tserver/tablet_server_interface.h index 3ce8c33b105c..b850238588bc 100644 --- a/src/yb/tserver/tablet_server_interface.h +++ b/src/yb/tserver/tablet_server_interface.h @@ -55,6 +55,7 @@ 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, diff --git a/src/yb/tserver/tablet_service.cc b/src/yb/tserver/tablet_service.cc index 01304aef4827..d76cb63bcfb3 100644 --- a/src/yb/tserver/tablet_service.cc +++ b/src/yb/tserver/tablet_service.cc @@ -1977,7 +1977,8 @@ void TabletServiceAdminImpl::WaitForYsqlBackendsCatalogVersion( [catalog_version, database_oid, this, &ts_catalog_version]() -> Result { // 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) { + if (FLAGS_ysql_enable_db_catalog_version_mode && + server_->catalog_version_table_in_perdb_mode()) { server_->get_ysql_db_catalog_version( database_oid, &ts_catalog_version, nullptr /* last_breaking_catalog_version */); } else { diff --git a/src/yb/tserver/tserver_shared_mem.h b/src/yb/tserver/tserver_shared_mem.h index 09775c6f412b..255c66531ab1 100644 --- a/src/yb/tserver/tserver_shared_mem.h +++ b/src/yb/tserver/tserver_shared_mem.h @@ -85,6 +85,14 @@ 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); + } + + bool catalog_version_table_in_perdb_mode() const { + return catalog_version_table_in_perdb_mode_.load(std::memory_order_acquire); + } + void SetPostgresAuthKey(uint64_t auth_key) { postgres_auth_key_ = auth_key; } @@ -112,6 +120,8 @@ class TServerSharedData { unsigned char tserver_uuid_[16]; // Tserver UUID is stored as raw bytes. std::atomic db_catalog_versions_[kMaxNumDbCatalogVersions] = {0}; + // See same variable comments in CatalogManager. + std::atomic catalog_version_table_in_perdb_mode_{false}; }; YB_STRONGLY_TYPED_BOOL(Create); diff --git a/src/yb/yql/pggate/pggate.cc b/src/yb/yql/pggate/pggate.cc index 63397a6ca77f..8c127982bfc5 100644 --- a/src/yb/yql/pggate/pggate.cc +++ b/src/yb/yql/pggate/pggate.cc @@ -1906,6 +1906,10 @@ Result PgApiImpl::GetNumberOfDatabases() { return info.num_entries(); } +Result PgApiImpl::CatalogVersionTableInPerdbMode() { + return tserver_shared_object_->catalog_version_table_in_perdb_mode(); +} + uint64_t PgApiImpl::GetSharedAuthKey() const { return tserver_shared_object_->postgres_auth_key(); } diff --git a/src/yb/yql/pggate/pggate.h b/src/yb/yql/pggate/pggate.h index 41fe797d67fd..593375f1b803 100644 --- a/src/yb/yql/pggate/pggate.h +++ b/src/yb/yql/pggate/pggate.h @@ -166,6 +166,7 @@ class PgApiImpl { Result GetSharedCatalogVersion(std::optional db_oid = std::nullopt); Result GetNumberOfDatabases(); + Result CatalogVersionTableInPerdbMode(); uint64_t GetSharedAuthKey() const; const unsigned char *GetLocalTserverUuid() const; diff --git a/src/yb/yql/pggate/ybc_pggate.cc b/src/yb/yql/pggate/ybc_pggate.cc index 9f4840422dae..9c519ee67ca0 100644 --- a/src/yb/yql/pggate/ybc_pggate.cc +++ b/src/yb/yql/pggate/ybc_pggate.cc @@ -1633,6 +1633,10 @@ YBCStatus YBCGetNumberOfDatabases(uint32_t* num_databases) { return ExtractValueFromResult(pgapi->GetNumberOfDatabases(), num_databases); } +YBCStatus YBCCatalogVersionTableInPerdbMode(bool* perdb_mode) { + return ExtractValueFromResult(pgapi->CatalogVersionTableInPerdbMode(), perdb_mode); +} + uint64_t YBCGetSharedAuthKey() { return pgapi->GetSharedAuthKey(); } diff --git a/src/yb/yql/pggate/ybc_pggate.h b/src/yb/yql/pggate/ybc_pggate.h index d9bc47ee7fbe..7bb827ed2424 100644 --- a/src/yb/yql/pggate/ybc_pggate.h +++ b/src/yb/yql/pggate/ybc_pggate.h @@ -77,6 +77,10 @@ YBCStatus YBCGetSharedDBCatalogVersion( // catalog version mode is enabled. YBCStatus YBCGetNumberOfDatabases(uint32_t* num_databases); +// Return true if the pg_yb_catalog_version table has been updated to +// have one row per database. +YBCStatus YBCCatalogVersionTableInPerdbMode(bool* perdb_mode); + // Return auth_key to the local tserver's postgres authentication key stored in shared memory. uint64_t YBCGetSharedAuthKey(); diff --git a/src/yb/yql/pgwrapper/pg_catalog_version-test.cc b/src/yb/yql/pgwrapper/pg_catalog_version-test.cc index dee2ca5f364e..8362df118c21 100644 --- a/src/yb/yql/pgwrapper/pg_catalog_version-test.cc +++ b/src/yb/yql/pgwrapper/pg_catalog_version-test.cc @@ -831,15 +831,20 @@ TEST_F(PgCatalogVersionTest, FixCatalogVersionTable) { CHECK_EQ(versions.size(), 1); ASSERT_OK(CheckMatch(versions.begin()->second, kNewCatalogVersion)); - // For a new connection, although --ysql_enable_db_catalog_version_mode is still - // true, the fact that the table pg_yb_catalog_version has only one row prevents - // a new connection to enter per-database catalog version mode. Verify that we - // can make a new connection to database "yugabyte". - ASSERT_RESULT(ConnectToDB("yugabyte")); + // Once a tserver enters per-database catalog version mode it remains so. + // It is an error to change pg_yb_catalog_version back to global catalog + // version mode when --ysql_enable_db_catalog_version_mode=true. + // Verify that we can not make a new connection to database "yugabyte" + // in this error state. + const auto yugabyte_db_oid = ASSERT_RESULT(GetDatabaseOid(&conn_yugabyte, kYugabyteDatabase)); + auto status = ResultToStatus(ConnectToDB("yugabyte")); + ASSERT_TRUE(status.IsNetworkError()) << status; + ASSERT_STR_CONTAINS(status.ToString(), + Format("catalog version for database $0 was not found", yugabyte_db_oid)); + ASSERT_STR_CONTAINS(status.ToString(), "Database might have been dropped by another user"); - // We can also make a new connection to database "template1" but the fact that - // now it is the only database that has a row in pg_yb_catalog_version table is - // not relevant. + // We can only make a new connection to database "template1" because now it + // is the only database that has a row in pg_yb_catalog_version table. conn_template1 = ASSERT_RESULT(ConnectToDB("template1")); // Sync up pg_yb_catalog_version with pg_database. @@ -1206,6 +1211,19 @@ TEST_F(PgCatalogVersionTest, RemoveRelCacheInitFiles) { RemoveRelCacheInitFilesHelper(false /* per_database_mode */); } +// This test that YSQL can execute DDL statements when the gflag +// --ysql_enable_db_catalog_version_mode is on but the pg_yb_catalog_version +// table isn't updated to have one row per database. +TEST_F(PgCatalogVersionTest, SimulateTryoutPhaseInUpgrade) { + auto conn_yugabyte = ASSERT_RESULT(Connect()); + ASSERT_OK(PrepareDBCatalogVersion(&conn_yugabyte, false /* per_database_mode */)); + RestartClusterWithDBCatalogVersionMode(); + conn_yugabyte = ASSERT_RESULT(Connect()); + ASSERT_OK(conn_yugabyte.Execute("CREATE TABLE t(id INT)")); + ASSERT_OK(conn_yugabyte.ExecuteFormat("CREATE INDEX idx ON t(id)")); + ASSERT_OK(conn_yugabyte.Execute("ALTER ROLE yugabyte SUPERUSER")); +} + TEST_F(PgCatalogVersionTest, SqlCrossDBLoadWithDDL) { const std::vector> ddlLists = {