[BACKPORT pg15-cherrypicks][#23882] YSQL: Improve cache re-invalidati…

…on for alter table commands

Summary:
- tablecmds.c:
     - ATController:
         - YB master commit 5dc71ea renames the variable `handles` to `ybAlteredTableIds`. It also introduces bad style for the call to `ATRewriteCatalogs()`.
         - YB PG15 commit 4c65fa6 resolves previous merge conflict on the same lines.
         - Resolve the conflicts by choosing the YB PG15 changes (to keep the style and the new `context` variable) and then replacing `handles` with `ybAlteredTableIds`.

other changes (not due to conflicts):
- yb_tablespaces.sql:
   - YB_TODO:
         - Remove the YB_TODO. While the issue mentioned (#23825 -- double output due to schema version mismatch) in the YB_TODO is still relevant, it no longer occurs in the test. The YB master commit 5dc71ea fixes the unexpected schema version mismatch.
- yb_tablespaces.out:
     - Remove the YB_TODO (see explanation for yb_tablespaces.sql) and update the test output.
Jira: DB-12786

Test Plan:
./yb_build.sh --cxx-test pgwrapper_pg_ddl_atomicity-test --gtest_filter PgDdlAtomicityTest.TestTableCacheAfterTxnVerification
./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressTablespaces'

Reviewers: jason, tfoucher

Reviewed By: tfoucher

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

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

@@ -700,7 +700,7 @@ static void YbATSetPKRewriteChildPartitions(List **yb_wqueue,
 											bool skip_copy_split_options);
 static void YbATCopyIndexSplitOptions(Oid oldId, IndexStmt *stmt,
 									  AlteredTableInfo *tab);
-static void YbATInvalidateTableCacheAfterAlter(List *handles);
+static void YbATInvalidateTableCacheAfterAlter(List *ybAlteredTableIds);
 /* ----------------------------------------------------------------
  *		DefineRelation
  *				Creates a new relation.
@@ -4738,19 +4738,19 @@ ATController(AlterTableStmt *parsetree,
 
 	/* Phase 2: update system catalogs */
 	List *rollbackHandles = NIL;
-	List *volatile handles = NIL;
+	List *volatile ybAlteredTableIds = NIL;
 	PG_TRY();
 	{
 		/*
 		 * ATRewriteCatalogs also executes changes to DocDB.
 		 * If Phase 3 fails, rollbackHandle will specify how to rollback the
 		 * changes done to DocDB.
 		 */
-		ATRewriteCatalogs(&wqueue, lockmode, context, &rollbackHandles, &handles);
+		ATRewriteCatalogs(&wqueue, lockmode, context, &rollbackHandles, &ybAlteredTableIds);
 	}
 	PG_CATCH();
 	{
-		YbATInvalidateTableCacheAfterAlter(handles);
+		YbATInvalidateTableCacheAfterAlter(ybAlteredTableIds);
 		PG_RE_THROW();
 	}
 	PG_END_TRY();
@@ -4759,7 +4759,7 @@ ATController(AlterTableStmt *parsetree,
 	PG_TRY();
 	{
 		ATRewriteTables(parsetree, &wqueue, lockmode, context);
-		YbATInvalidateTableCacheAfterAlter(handles);
+		YbATInvalidateTableCacheAfterAlter(ybAlteredTableIds);
 	}
 	PG_CATCH();
 	{
@@ -4777,7 +4777,7 @@ ATController(AlterTableStmt *parsetree,
 				YBCExecAlterTable(handle, RelationGetRelid(rel));
 			}
 		}
-		YbATInvalidateTableCacheAfterAlter(handles);
+		YbATInvalidateTableCacheAfterAlter(ybAlteredTableIds);
 		PG_RE_THROW();
 	}
 	PG_END_TRY();
@@ -5181,7 +5181,7 @@ static void
 ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
 				  AlterTableUtilityContext *context,
 				  List **rollbackHandles,
-				  List *volatile *handles)
+				  List *volatile *ybAlteredTableIds)
 {
 	int			pass;
 	ListCell   *ltab;
@@ -5198,12 +5198,13 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
 	AlteredTableInfo* info       = (AlteredTableInfo *) linitial(*wqueue);
 	Oid               main_relid = info->relid;
 	YBCPgStatement rollbackHandle = NULL;
-	*handles = YBCPrepareAlterTable(info->subcmds,
+	List *handles = YBCPrepareAlterTable(info->subcmds,
 										 AT_NUM_PASSES,
 										 main_relid,
 										 &rollbackHandle,
-										 false /* isPartitionOfAlteredTable */,
-										 info->rewrite);
+										 false /* isPartitionOfAlteredTable */);
+	if (handles)
+		*ybAlteredTableIds = lappend_oid(*ybAlteredTableIds, main_relid);
 	if (rollbackHandle)
 		*rollbackHandles = lappend(*rollbackHandles, rollbackHandle);
 
@@ -5227,13 +5228,14 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
 												   AT_NUM_PASSES,
 												   childrelid,
 												   &childRollbackHandle,
-												   true /*isPartitionOfAlteredTable */,
-												   info->rewrite);
+												   true /*isPartitionOfAlteredTable */);
+		if (child_handles)
+			*ybAlteredTableIds = lappend_oid(*ybAlteredTableIds, childrelid);
 		ListCell *listcell = NULL;
 		foreach(listcell, child_handles)
 		{
 			YBCPgStatement child = (YBCPgStatement) lfirst(listcell);
-			*handles = lappend(*handles, child);
+			handles = lappend(handles, child);
 		}
 		if (childRollbackHandle)
 			*rollbackHandles = lappend(*rollbackHandles, childRollbackHandle);
@@ -5262,7 +5264,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
 		 */
 		if (pass == AT_PASS_ADD_INDEX)
 		{
-			foreach(lc, *handles)
+			foreach(lc, handles)
 			{
 				YBCPgStatement handle = (YBCPgStatement) lfirst(lc);
 				YBCExecAlterTable(handle, main_relid);
@@ -5339,7 +5341,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
 		 */
 		if (yb_table_cloned)
 		{
-			foreach (lc, *handles)
+			foreach (lc, handles)
 			{
 				YBCPgStatement handle = (YBCPgStatement) lfirst(lc);
 				YBCPgAlterTableSetTableId(
@@ -22736,20 +22738,33 @@ static void YbATCopyIndexSplitOptions(Oid oldId, IndexStmt *stmt,
  * Used in YB to re-invalidate table cache entries at the end of an ALTER TABLE
  * operation.
  */
-static void YbATInvalidateTableCacheAfterAlter(List *handles)
+static void YbATInvalidateTableCacheAfterAlter(List *ybAlteredTableIds)
 {
-	if (YbDdlRollbackEnabled() && handles)
+	if (YbDdlRollbackEnabled() && ybAlteredTableIds)
 	{
 		/*
 		 * As part of DDL transaction verification, we may have incremented
 		 * the schema version for the affected tables. So, re-invalidate
 		 * the table cache entries of the affected tables.
 		 */
 		ListCell *lc = NULL;
-		foreach(lc, handles)
+		foreach(lc, ybAlteredTableIds)
 		{
-			YBCPgStatement handle = (YBCPgStatement) lfirst(lc);
-			HandleYBStatus(YBCPgAlterTableInvalidateTableCacheEntry(handle));
+			Oid relid = lfirst_oid(lc);
+			Relation rel = RelationIdGetRelation(relid);
+			/*
+			 * The relation may no longer exist if it was dropped as part of
+			 * a legacy rewrite operation. We can skip invalidation in that
+			 * case.
+			 */
+			if (!rel)
+			{
+				Assert(!yb_enable_alter_table_rewrite);
+				continue;
+			}
+			YBCPgAlterTableInvalidateTableByOid(YBCGetDatabaseOidByRelid(relid),
+				YbGetRelfileNodeIdFromRelId(relid));
+			RelationClose(rel);
 		}
 	}
 }

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

@@ -1174,8 +1174,7 @@ static List*
 YBCPrepareAlterTableCmd(AlterTableCmd* cmd, Relation rel, List *handles,
 						int* col, bool* needsYBAlter,
 						YBCPgStatement* rollbackHandle,
-						bool isPartitionOfAlteredTable,
-						int rewrite)
+						bool isPartitionOfAlteredTable)
 {
 	Oid relationId = RelationGetRelid(rel);
 	Oid relfileNodeId = YbGetRelfileNodeId(rel);
@@ -1396,13 +1395,6 @@ YBCPrepareAlterTableCmd(AlterTableCmd* cmd, Relation rel, List *handles,
 								cmd->name, RelationGetRelationName(rel))));
 				}
 				ReleaseSysCache(typeTuple);
-
-				/*
-				 * If this ALTER TYPE operation doesn't require a rewrite
-				 * we do not need to increment the schema version.
-				 */
-				if (!(rewrite & AT_REWRITE_COLUMN_REWRITE))
-					break;
 			}
 			/*
 			 * For these cases a YugaByte metadata does not need to be updated
@@ -1613,8 +1605,7 @@ YBCPrepareAlterTable(List** subcmds,
 					 int subcmds_size,
 					 Oid relationId,
 					 YBCPgStatement *rollbackHandle,
-					 bool isPartitionOfAlteredTable,
-					 int rewriteState)
+					 bool isPartitionOfAlteredTable)
 {
 	/* Appropriate lock was already taken */
 	Relation rel = relation_open(relationId, NoLock);
@@ -1642,7 +1633,7 @@ YBCPrepareAlterTable(List** subcmds,
 			handles = YBCPrepareAlterTableCmd(
 						(AlterTableCmd *) lfirst(lcmd), rel, handles,
 						&col, &needsYBAlter, rollbackHandle,
-						isPartitionOfAlteredTable, rewriteState);
+						isPartitionOfAlteredTable);
 		}
 	}
 	relation_close(rel, NoLock);

src/postgres/src/include/commands/ybccmds.h

@@ -97,8 +97,7 @@ extern List* YBCPrepareAlterTable(List** subcmds,
 										   int subcmds_size,
 										   Oid relationId,
 										   YBCPgStatement *rollbackHandle,
-										   bool isPartitionOfAlteredTable,
-										   int rewriteState);
+										   bool isPartitionOfAlteredTable);
 
 extern void YBCExecAlterTable(YBCPgStatement handle, Oid relationId);
 

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

@@ -355,12 +355,7 @@ lsm, for table "testschema.test_default_tab"
 lsm, for table "testschema.test_default_tab"
 Tablespace: "regress_tblspace"
 
--- YB_TODO: Update output after GH #23825 is fixed.
 SELECT * FROM testschema.test_default_tab;
- id
-----
-(0 rows)
-
  id
 ----
   1

src/postgres/src/test/regress/sql/yb_tablespaces.sql

@@ -199,7 +199,6 @@ SET default_tablespace TO regress_tblspace;
 ALTER TABLE testschema.test_default_tab ALTER id TYPE bigint;
 \d testschema.test_index1
 \d testschema.test_index2
--- YB_TODO: Update output after GH #23825 is fixed.
 SELECT * FROM testschema.test_default_tab;
 -- tablespace should not change even if there is an index rewrite
 ALTER TABLE testschema.test_default_tab ALTER id TYPE int;

src/yb/yql/pggate/ybc_pggate.cc

@@ -838,6 +838,11 @@ YBCStatus YBCPgInvalidateTableCacheByTableId(const char *table_id) {
   return YBCStatusOK();
 }
 
+void YBCPgAlterTableInvalidateTableByOid(
+    const YBCPgOid database_oid, const YBCPgOid table_relfilenode_oid) {
+  pgapi->InvalidateTableCache(PgObjectId(database_oid, table_relfilenode_oid));
+}
+
 // Tablegroup Operations ---------------------------------------------------------------------------
 
 YBCStatus YBCPgNewCreateTablegroup(const char *database_name,

src/yb/yql/pggate/ybc_pggate.h

@@ -332,6 +332,9 @@ YBCStatus YBCPgExecAlterTable(YBCPgStatement handle);
 
 YBCStatus YBCPgAlterTableInvalidateTableCacheEntry(YBCPgStatement handle);
 
+void YBCPgAlterTableInvalidateTableByOid(
+    const YBCPgOid database_oid, const YBCPgOid table_relfilenode_oid);
+
 YBCStatus YBCPgNewDropTable(YBCPgOid database_oid,
                             YBCPgOid table_relfilenode_oid,
                             bool if_exist,

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

@@ -1669,29 +1669,47 @@ TEST_F(PgDdlAtomicityMiniClusterTest, TestWaitForRollbackWithMasterRestart) {
 
 // Test that the table cache is correctly invalidated after transaction verification
 // completes for an ALTER TABLE operation that performs a table scan.
-TEST_F(PgDdlAtomicityMiniClusterTest, TestTableCacheAfterTxnVerification) {
+TEST_F(PgDdlAtomicityTest, TestTableCacheAfterTxnVerification) {
+  // Set report_ysql_ddl_txn_status_to_master to false, so that we can test the schema verification
+  // codepaths on master.
+  ASSERT_OK(cluster_->SetFlagOnTServers(
+      "report_ysql_ddl_txn_status_to_master", "false"));
   auto conn = ASSERT_RESULT(Connect());
   ASSERT_OK(conn.ExecuteFormat(
       "CREATE TABLE test (key INT PRIMARY KEY, value TEXT, num real, serialcol SERIAL) "
       "PARTITION BY LIST(key)"));
   ASSERT_OK(conn.ExecuteFormat(
       "CREATE TABLE test1 PARTITION OF test FOR VALUES IN (1)"));
   ASSERT_OK(conn.ExecuteFormat(
-      "CREATE TABLE test2 PARTITION OF test FOR VALUES IN (2, 3, 4)"));
+      "CREATE TABLE test2 PARTITION OF test FOR VALUES IN (2, 3, 4, 5)"));
   ASSERT_OK(conn.ExecuteFormat("INSERT INTO test VALUES (1, 'value', 1.0), (2, 'value', 2.0)"));
   ASSERT_OK(conn.TestFailDdl(
     "ALTER TABLE test DROP COLUMN value, ADD CONSTRAINT check_num CHECK (num > 0)"));
   // Ensure there is no schema version mismatch after a failed ALTER operation that performs
   // a table scan.
-  ASSERT_OK(conn.Execute("INSERT INTO test2 VALUES (3, 'value', 3.0)"));
+  ASSERT_OK(conn.Execute("BEGIN; INSERT INTO test2 VALUES (3, 'value', 3.0); COMMIT;"));
   ASSERT_OK(conn.ExecuteFormat(
     "ALTER TABLE test DROP COLUMN value, ADD CONSTRAINT check_num CHECK (num > 0)"));
   // Ensure there is no schema version mismatch after a successful ALTER operation that performs
   // a table scan.
-  ASSERT_OK(conn.Execute("INSERT INTO test2 VALUES (4, 4.0)"));
+  ASSERT_OK(conn.Execute("BEGIN; INSERT INTO test2 VALUES (4, 4.0); COMMIT;"));
   auto rows =
       ASSERT_RESULT((conn.FetchRows<int32_t, float, int32_t>("SELECT * FROM test2 ORDER BY key")));
   ASSERT_EQ(rows, (decltype(rows){{2, 2, 2}, {3, 3, 3}, {4, 4, 4}}));
+  // Ensure there is no schema version mismatch for various ALTERs.
+  // Alter type (with no rewrite).
+  ASSERT_OK(conn.Execute("ALTER TABLE test ALTER COLUMN num TYPE double precision"));
+  ASSERT_OK(conn.Execute("BEGIN; INSERT INTO test VALUES (5, 5.0); COMMIT;"));
+  // Legacy table rewrite.
+  ASSERT_OK(conn.Execute("CREATE TABLE test3 (key INT, value TEXT)"));
+  ASSERT_OK(conn.Execute("INSERT INTO test3 VALUES (1, 'value')"));
+  ASSERT_OK(conn.Execute("SET yb_enable_alter_table_rewrite = OFF;"));
+  ASSERT_OK(conn.Execute(
+      "ALTER TABLE test3 ADD PRIMARY KEY (key), ALTER COLUMN value SET NOT NULL"));
+  ASSERT_OK(conn.Execute("BEGIN; INSERT INTO test3 VALUES (2, 'value2'); COMMIT;"));
+  ASSERT_OK(conn.Execute("ALTER TABLE test3 ALTER COLUMN value TYPE int USING length(value),"
+      "ADD CONSTRAINT check_value CHECK (value > 0)"));
+  ASSERT_OK(conn.Execute("BEGIN; INSERT INTO test3 VALUES (3, 6); COMMIT;"));
 }
 
 // Test that DDL-related metadata in TableInfo objects is cleared on drop.