Skip to content

Commit

Permalink
Revert "[#20300] YSQL: Disallow new-version DDL in an invalid per-db …
Browse files Browse the repository at this point in the history 
…catalog version configuration"

Summary:
This reverts commit 0ca855f.

Background:
1. In the new upgrade work flow, there can be a significant window of tryout
time after the new binaries are installed and the YSQL migration scripts
are run which happens in the finalization stage. This is to allow the upgrade
to rollback if the tryout does not turn out well.

2. An auto gflag must be RUNTIME but --ysql_enable_db_catalog_version should
not be RUNTIME. So the plan is to make it a regular NON_RUNTIME gflag and on
by default. This implies that during the tryout time the gflag is true but
the table pg_yb_catalog_version remains in global catalog version mode: there
is only one row for database template1.

3. Therefore it is unreasonable to disallow DDLs that increment catalog version
during the tryout time.

4. It is still reasonable to advise customers not to run DDLs during finalization
phase, which includes promote persistent auto gflags, and running YSQL migration
scripts. The finalization phase isn't expected to take an extended period of
time like the tryout phase.

5. I plan to make a change in a follow up diff to make an alternative fix for
the issue #20300 by allowing `create index` statement during the tryout phase:
when --ysql_enable_db_catalog_version_mode=true but the pg_yb_catalog_version
isn't updated yet.

NOTE: the reverted diff had a small change from
  elog(INFO, "change to per-db mode");
to
  elog(LOG, "change to per-db mode");
This change was not reverted in this diff.
Jira: DB-9266

Test Plan: ./yb_build.sh --cxx-test pg_catalog_version-test

Reviewers: jason, tverona

Reviewed By: jason

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D32274
  • Loading branch information
@myang2021
myang2021 committed Feb 8, 2024
1 parent af5f5b3 commit ca30501
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 75 deletions.
40 changes: 0 additions & 40 deletions src/postgres/src/backend/utils/misc/pg_yb_utils.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -642,43 +642,6 @@ YBCanEnableDBCatalogVersionMode()
return (YbGetNumberOfDatabases() > 1);
}

void
YBCheckDdlForDBCatalogVersionMode(YbDdlMode mode)
{
/*
* When --ysql_enable_db_catalog_version_mode=true, we only need to check
* for incompatible pg_yb_catalog_version and disallow DDLs that increment
* the catalog version. DDLs that do not increment the catalog version are
* fine because there isn't any problem.
*/
if (!(mode & YB_SYS_CAT_MOD_ASPECT_VERSION_INCREMENT))
return;

bool db_catalog_version_mode_gflag =
YBCIsEnvVarTrueWithDefault("FLAGS_ysql_enable_db_catalog_version_mode",
false);
int num_databases = YbGetNumberOfDatabases();

/*
* Disallow DDL statement when FLAGS_ysql_enable_db_catalog_version_mode
* is on but pg_yb_catalog_version table only has one row. Note that
* the other mismatch (where FLAGS_ysql_enable_db_catalog_version_mode is
* off but pg_yb_catalog_version table has one row per database) is fine
* because we only use the first row (which is for template1) and ignore
* the other rows and therefore table pg_yb_catalog_version is used in the
* global catalog version mode. Also note that due to heart beat delay,
* this rejection is done at best effort.
*/
if (db_catalog_version_mode_gflag && num_databases == 1)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("this ddl statement is currently not allowed"),
errdetail("The pg_yb_catalog_version table is not in "
"per-database catalog version mode."),
errhint("Fix pg_yb_catalog_version table to per-database "
"catalog version mode.")));
}

/*
* Used to determine whether we should preload certain catalog tables.
*/
Expand Down Expand Up @@ -2106,9 +2069,6 @@ YBTxnDdlProcessUtility(

const bool is_ddl = ddl_mode.has_value;

if (is_ddl)
YBCheckDdlForDBCatalogVersionMode(ddl_mode.value);

PG_TRY();
{
if (is_ddl)
Expand Down
8 changes: 0 additions & 8 deletions src/postgres/src/include/pg_yb_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1049,14 +1049,6 @@ extern void** YbPtrListToArray(const List* str_list, size_t* length);
*/
extern char* YbReadWholeFile(const char *filename, int* length, int elevel);


/*
* If the tserver gflag --ysql_enable_db_catalog_version_mode is true
* but the number of rows in pg_yb_catalog_version is 1, disallow a DDL
* statement if it increments the catalog version.
*/
extern void YBCheckDdlForDBCatalogVersionMode(YbDdlMode mode);

extern bool yb_use_tserver_key_auth;

extern bool yb_use_tserver_key_auth_check_hook(bool *newval,
Expand Down
39 changes: 12 additions & 27 deletions src/yb/yql/pgwrapper/pg_catalog_version-test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -815,19 +815,21 @@ TEST_F(PgCatalogVersionTest, FixCatalogVersionTable) {
// There is only one row in the table pg_yb_catalog_version now.
CHECK_EQ(versions.size(), 1);
ASSERT_OK(CheckMatch(versions.begin()->second, kCurrentCatalogVersion));
// A global-impact DDL statement that increments catalog version is rejected.
auto status = conn_yugabyte.Execute("ALTER ROLE yugabyte SUPERUSER");
ASSERT_TRUE(status.IsNetworkError()) << status;
const auto yugabyte_db_oid = ASSERT_RESULT(GetDatabaseOid(&conn_yugabyte, kYugabyteDatabase));
ASSERT_STR_CONTAINS(status.ToString(),
Format("catalog version for database $0 was not found", yugabyte_db_oid));
// A global-impact DDL statement that increments catalog version still works.
ASSERT_OK(conn_yugabyte.Execute("ALTER ROLE yugabyte SUPERUSER"));
constexpr CatalogVersion kNewCatalogVersion{2, 2};
versions = ASSERT_RESULT(GetMasterCatalogVersionMap(&conn_yugabyte));
CHECK_EQ(versions.size(), 1);
ASSERT_OK(CheckMatch(versions.begin()->second, kCurrentCatalogVersion));
ASSERT_OK(CheckMatch(versions.begin()->second, kNewCatalogVersion));

ASSERT_OK(conn_yugabyte.Execute("ALTER TABLE test_table ADD COLUMN c2 INT"));

// A non-global-impact DDL statement that increments catalog version is
// rejected.
ASSERT_NOK(conn_yugabyte.Execute("ALTER TABLE test_table ADD COLUMN c2 INT"));
// The non-global-impact DDL statement does not have an effect on the
// table pg_yb_catalog_version when it tries to update the row of yugabyte
// because that row no longer exists. There is no user visible effect.
versions = ASSERT_RESULT(GetMasterCatalogVersionMap(&conn_yugabyte));
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
Expand Down Expand Up @@ -1204,23 +1206,6 @@ TEST_F(PgCatalogVersionTest, RemoveRelCacheInitFiles) {
RemoveRelCacheInitFilesHelper(false /* per_database_mode */);
}

// This test that YSQL rejects a DDL statement that increments catalog version
// 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.
// Note that due to heart beat delay, this rejection is done at best effort.
TEST_F(PgCatalogVersionTest, DisallowCatalogVersionBumpDDL) {
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)"));
auto status = conn_yugabyte.ExecuteFormat("CREATE INDEX idx ON t(id)");
ASSERT_TRUE(status.IsNetworkError()) << status;
ASSERT_STR_CONTAINS(status.ToString(),
"The pg_yb_catalog_version table is not in "
"per-database catalog version mode");
}

TEST_F(PgCatalogVersionTest, SqlCrossDBLoadWithDDL) {

const std::vector<std::vector<string>> ddlLists = {
Expand Down

0 comments on commit ca30501

Please sign in to comment.