[BACKPORT 2024.1][#22802] YSQL: Avoid renaming DocDb tables during le…

…gacy rewrite

Summary:
As part of legacy table rewrite operations that clone the table, the old table is
renamed to <table_name>_temp_old. Without DDL atomicity, if the operation fails at
any point after the rename, the PG metadata is rolled back (relname is rolled back to
<table_name>), but the DocDB table name is still <table_name>_temp_old.
Note: the same rename is also done for the table's indexes.

These renamed DocDB tables cause issues with backup/restore as the restore
operation relies on table names.

We can avoid these issues by avoiding the DocDB level renames altogether,
as the old tables are going to be dropped anyway.

Now, after a failed ADD/DROP pkey, ALTER TYPE operation (using the legacy rewrite),
the DocDB table will no longer be left with the name <table_name>_temp_old.
However, there will now be two DocDB tables with the same table name after a failed
legacy rewrite operation. The async transaction cleanup task on master will lookup
the entry for this orphaned table in pg_class (using the pg oid in the DocDB table id),
and will drop the table upon finding that it doesn't exist. If, for some reason,
the orphaned table is not cleaned up, or if a back up is taken before the cleanup happens,
the backup code will simply ignore this orphaned table. Specifically, RepackSnapshotsForBackup
will not find the pg schema name metadata for this table, and will simply ignore it.
Therefore, there will be no ambiguity on restore side (which relies on the table names).
Jira: DB-11703
Original commit: c47b2d9 / D35943

Test Plan:
On an alma8 release build:

./yb_build.sh release --cxx-test tools_yb-backup-cross-feature-test --gtest_filter YBBackupTest.TestBackupWithFailedLegacyRewrite -n 100

Reviewers: myang

Reviewed By: myang

Subscribers: ybase, yql

Tags: #jenkins-ready

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

src/postgres/src/backend/commands/alter.c

@@ -352,7 +352,7 @@ ExecRenameStmt(RenameStmt *stmt)
 		case OBJECT_MATVIEW:
 		case OBJECT_INDEX:
 		case OBJECT_FOREIGN_TABLE:
-			return RenameRelation(stmt);
+			return RenameRelation(stmt, false /* yb_is_internal_clone_rename */);
 
 		case OBJECT_COLUMN:
 		case OBJECT_ATTRIBUTE:

src/postgres/src/backend/commands/tablecmds.c

@@ -3395,9 +3395,12 @@ RenameConstraint(RenameStmt *stmt)
 /*
  * Execute ALTER TABLE/INDEX/SEQUENCE/VIEW/MATERIALIZED VIEW/FOREIGN TABLE
  * RENAME
+ * When yb_is_internal_clone_rename is true we don't need to do a YB rename,
+ * as this rename is a part of a table clone operation, and the relation
+ * will be dropped after the clone operation is done anyway.
  */
 ObjectAddress
-RenameRelation(RenameStmt *stmt)
+RenameRelation(RenameStmt *stmt, bool yb_is_internal_clone_rename)
 {
 	Oid           relid;
 	ObjectAddress address;
@@ -3432,7 +3435,8 @@ RenameRelation(RenameStmt *stmt)
 	needs_yb_rename = IsYBRelation(rel) &&
 					  !(rel->rd_rel->relkind == RELKIND_INDEX &&
 						rel->rd_index->indisprimary) &&
-					  rel->rd_rel->relkind != RELKIND_PARTITIONED_INDEX;
+					  rel->rd_rel->relkind != RELKIND_PARTITIONED_INDEX
+					  && !yb_is_internal_clone_rename;
 	RelationClose(rel);
 
 	/* Do the work */
@@ -18312,7 +18316,7 @@ YbATCopyIndexes(Relation old_rel, Oid new_relid, AttrNumber *new2old_attmap,
 		rename_stmt->relation = makeRangeVar(
 			pstrdup(namespace_name), pstrdup(idx_orig_name), -1 /* location */);
 		rename_stmt->newname = pstrdup(idx_temp_old_name);
-		RenameRelation(rename_stmt);
+		RenameRelation(rename_stmt, true /* yb_is_internal_clone_rename */);
 		CommandCounterIncrement();
 
 		/* Create a new index taking up the freed name. */
@@ -18736,7 +18740,7 @@ YbATCloneRelationSetPrimaryKey(Relation old_rel, IndexStmt *stmt,
 	 */
 	rename_stmt =
 		YbATGetRenameStmt(namespace_name, orig_table_name, temp_old_table_name);
-	RenameRelation(rename_stmt);
+	RenameRelation(rename_stmt, true /* yb_is_internal_clone_rename */);
 
 	/* Make caches changes visible. */
 	CommandCounterIncrement();
@@ -18950,7 +18954,7 @@ YbATCloneRelationSetColumnType(Relation old_rel,
 	 */
 	rename_stmt =
 		YbATGetRenameStmt(namespace_name, orig_table_name, temp_old_table_name);
-	RenameRelation(rename_stmt);
+	RenameRelation(rename_stmt, true /* yb_is_internal_clone_rename */);
 
 	/* Make caches changes visible. */
 	CommandCounterIncrement();

src/postgres/src/include/commands/tablecmds.h

@@ -64,7 +64,8 @@ extern ObjectAddress renameatt_type(RenameStmt *stmt);
 
 extern ObjectAddress RenameConstraint(RenameStmt *stmt);
 
-extern ObjectAddress RenameRelation(RenameStmt *stmt);
+extern ObjectAddress RenameRelation(RenameStmt *stmt,
+									bool yb_is_internal_clone_rename);
 
 extern void RenameRelationInternal(Oid myrelid,
 					   const char *newrelname, bool is_internal);

src/yb/tools/yb-backup/yb-backup-cross-feature-test.cc

@@ -2088,5 +2088,48 @@ TEST_P(YBBackupTestWithTableRewrite,
       )#"
   ));
 }
+
+TEST_F(YBBackupTest, YB_DISABLE_TEST_IN_SANITIZERS(TestBackupWithFailedLegacyRewrite)) {
+  ASSERT_OK(cluster_->SetFlagOnTServers("ysql_yb_enable_alter_table_rewrite", "false"));
+  ASSERT_OK(cluster_->SetFlagOnMasters("enable_transactional_ddl_gc", "false"));
+  ASSERT_OK(cluster_->SetFlagOnTServers("ysql_yb_ddl_rollback_enabled", "false"));
+
+  const auto table_name = "t1";
+  // Create the table.
+  ASSERT_NO_FATALS(CreateTable(Format("CREATE TABLE $0 (a int, b int)", table_name)));
+  // Insert some data.
+  ASSERT_NO_FATALS(InsertRows(Format(
+      "INSERT INTO $0 (a, b) VALUES (generate_series(1, 5), generate_series(1, 5))", table_name),
+      5));
+  // Create an index.
+  ASSERT_NO_FATALS(CreateIndex(Format("CREATE INDEX idx1 ON $0 (b DESC)", table_name)));
+  // Perform a failed ADD PKEY operation.
+  ASSERT_NO_FATALS(RunPsqlCommand(
+      Format("SET yb_test_fail_next_ddl = true;"
+             "ALTER TABLE $0 ADD PRIMARY KEY (a ASC)", table_name), "SET"));
+  // Verify the original table and the orphaned table exist.
+  ASSERT_EQ(ASSERT_RESULT(client_->ListTables(table_name)).size(), 2);
+
+  // Verify backup/restore works correctly.
+  const string backup_dir = GetTempDir("backup");
+  ASSERT_OK(RunBackupCommand(
+      {"--backup_location", backup_dir, "--keyspace", "ysql.yugabyte", "create"}));
+  ASSERT_OK(RunBackupCommand(
+      {"--backup_location", backup_dir, "--keyspace", "ysql.yugabyte_new", "restore"}));
+
+  SetDbName("yugabyte_new");
+  ASSERT_NO_FATALS(RunPsqlCommand(
+      Format("SELECT * FROM $0 ORDER BY a", table_name),
+      R"#(
+         a | b
+        ---+---
+         1 | 1
+         2 | 2
+         3 | 3
+         4 | 4
+         5 | 5
+        (5 rows)
+      )#"));
+}
 }  // namespace tools
 }  // namespace yb