v59_amit_1.txt

text/plain

Filename: v59_amit_1.txt
Type: text/plain
Part: 0
Message: Re: Proposal: Conflict log history table for Logical Replication
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 973d3607838..a3cda01d80f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6924,7 +6924,11 @@ ATSimplePermissions(AlterTableType cmdtype, Relation rel, int allowed_targets)
 	/*
 	 * Conflict log tables are used internally for logical replication conflict
 	 * logging and should not be altered directly, as it could disrupt conflict
-	 * logging.
+	 * logging. Direct ALTER commands are already rejected during relation
+	 * lookup in RangeVarCallbackForAlterRelation(), and AlterTableMoveAll()
+	 * skips these tables, so a conflict log table does not normally reach here;
+	 * this check guards any internal caller that arrives via
+	 * AlterTableInternal().
 	 */
 	if (IsConflictLogTableClass(rel->rd_rel))
 		ereport(ERROR,
@@ -17592,11 +17596,16 @@ AlterTableMoveAll(AlterTableMoveAllStmt *stmt)
 		 *
 		 * Also, explicitly avoid any shared tables, temp tables, or TOAST
 		 * (TOAST will be moved with the main table).
+		 *
+		 * Conflict log tables are system-managed for logical replication and
+		 * cannot be altered directly, so skip them as well; otherwise a single
+		 * such table in the source tablespace would abort the whole bulk move.
 		 */
 		if (IsCatalogNamespace(relForm->relnamespace) ||
 			relForm->relisshared ||
 			isAnyTempNamespace(relForm->relnamespace) ||
-			IsToastNamespace(relForm->relnamespace))
+			IsToastNamespace(relForm->relnamespace) ||
+			IsConflictLogTableNamespace(relForm->relnamespace))
 			continue;
 
 		/* Only move the object type requested */
diff --git a/src/test/subscription/t/035_conflicts.pl b/src/test/subscription/t/035_conflicts.pl
index 86d3f8903c5..4260c4a5803 100644
--- a/src/test/subscription/t/035_conflicts.pl
+++ b/src/test/subscription/t/035_conflicts.pl
@@ -716,12 +716,23 @@ ok( $node_A->poll_query_until(
 	"the slot 'pg_conflict_detection' has been dropped on Node A");
 
 ###############################################################################
-# Verify that ATSimplePermissions blocks moving a CLT via
-# ALTER TABLE ALL IN TABLESPACE.  We use an isolated database containing only
-# the CLT so no other tables are moved before the error fires.
+# A conflict log table is system-managed and cannot be altered directly, so
+# moving it to another tablespace must be rejected.
+###############################################################################
+
+(undef, undef, $stderr) = $node_subscriber->psql('postgres',
+	"ALTER TABLE $clt SET TABLESPACE pg_default");
+like(
+	$stderr,
+	qr/permission denied: "pg_conflict_log_\d+" is a conflict log table/,
+	"moving a conflict log table with ALTER TABLE SET TABLESPACE is rejected");
+
+###############################################################################
+# ALTER TABLE ALL IN TABLESPACE must skip conflict log tables, the same way it
+# skips catalog and TOAST tables, instead of failing.  Use an isolated database
+# so the bulk move only touches the objects created here.
 ###############################################################################
 
-# Create an isolated database with a single CLT and no user tables.
 $node_subscriber->safe_psql('postgres', "CREATE DATABASE clt_ts_test");
 $node_subscriber->safe_psql('clt_ts_test',
 	"CREATE SUBSCRIPTION sub_ts_test
@@ -729,6 +740,9 @@ $node_subscriber->safe_psql('clt_ts_test',
 	     PUBLICATION pub
 	     WITH (connect=false, conflict_log_destination='table')");
 
+# A plain user table that should be moved, alongside the CLT that must not be.
+$node_subscriber->safe_psql('clt_ts_test', "CREATE TABLE user_tbl (i int)");
+
 # Create a tablespace backed by a directory inside the data dir.
 my $ts_dir = $node_subscriber->data_dir . '/backup_space';
 mkdir($ts_dir)
@@ -736,23 +750,31 @@ mkdir($ts_dir)
 $node_subscriber->safe_psql('postgres',
 	"CREATE TABLESPACE backup_space LOCATION '$ts_dir'");
 
-# The CLT is the only non-system table in pg_default of clt_ts_test, so
-# ALTER TABLE ALL IN TABLESPACE hits it immediately and ATSimplePermissions
-# raises the expected error.
-(undef, undef, $stderr) = $node_subscriber->psql('clt_ts_test',
+# The bulk move succeeds: the user table is relocated while the CLT is skipped.
+$node_subscriber->safe_psql('clt_ts_test',
 	"ALTER TABLE ALL IN TABLESPACE pg_default SET TABLESPACE backup_space");
-like(
-	$stderr,
-	qr/permission denied: "pg_conflict_log_\d+" is a conflict log table/,
-	"ATSimplePermissions blocks moving CLT via ALTER TABLE ALL IN TABLESPACE");
 
-# Cleanup
+is( $node_subscriber->safe_psql(
+		'clt_ts_test',
+		"SELECT reltablespace <> 0 FROM pg_class WHERE relname = 'user_tbl'"),
+	't',
+	"ALTER TABLE ALL IN TABLESPACE moves an ordinary user table");
+
+is( $node_subscriber->safe_psql(
+		'clt_ts_test',
+		"SELECT count(*) FROM pg_class c JOIN pg_subscription s
+		   ON c.relname = 'pg_conflict_log_' || s.oid
+		 WHERE s.subname = 'sub_ts_test' AND c.reltablespace <> 0"),
+	'0',
+	"ALTER TABLE ALL IN TABLESPACE skips the conflict log table");
+
+# Cleanup.  The subscription has no real publisher connection, so detach its
+# slot before dropping it.
 $node_subscriber->safe_psql('clt_ts_test',
 	"ALTER SUBSCRIPTION sub_ts_test DISABLE");
 $node_subscriber->safe_psql('clt_ts_test',
 	"ALTER SUBSCRIPTION sub_ts_test SET (slot_name = NONE)");
-$node_subscriber->safe_psql('clt_ts_test',
-	"DROP SUBSCRIPTION sub_ts_test");
+$node_subscriber->safe_psql('clt_ts_test', "DROP SUBSCRIPTION sub_ts_test");
 $node_subscriber->safe_psql('postgres', "DROP DATABASE clt_ts_test");
 $node_subscriber->safe_psql('postgres', "DROP TABLESPACE backup_space");