From 522dc1f52df4d0aedacaec839f49910e7e851844 Mon Sep 17 00:00:00 2001 From: Ajin Cherian Date: Fri, 18 Sep 2026 17:21:06 +1000 Subject: [PATCH v2] Recheck table persistence after table_rewrite triggers ALTER TABLE ... SET LOGGED/UNLOGGED checks publication membership and foreign key relationships during command preparation. A table_rewrite event trigger, or a DEFAULT/CHECK expression evaluated during the rewrite, can execute DDL after those checks, allowing the command to leave tables with incompatible persistence or make an unlogged table part of a publication. Run a same check as preparation phase immediately after firing an event trigger. --- src/backend/commands/tablecmds.c | 107 ++++++++++++++------ src/test/regress/expected/event_trigger.out | 78 ++++++++++++++ src/test/regress/sql/event_trigger.sql | 90 ++++++++++++++++ 3 files changed, 243 insertions(+), 32 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 2f073ddb84a..006fadb7601 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -692,6 +692,7 @@ static void ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const cha static void ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethodId); static void ATPrepChangePersistence(AlteredTableInfo *tab, Relation rel, bool toLogged); +static void ATRewriteChangePersistence(AlteredTableInfo *tab); static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, const char *tablespacename, LOCKMODE lockmode); static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode); @@ -6108,6 +6109,20 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode, } } + /* + * Event triggers, or DEFAULT/CHECK expressions evaluated during + * the rewrite, can execute DDL that invalidates the persistence + * checks performed during preparation, so repeat them now that + * all tables have been rewritten. + */ + foreach(ltab, *wqueue) + { + AlteredTableInfo *tab = lfirst(ltab); + + if (tab->chgPersistence) + ATRewriteChangePersistence(tab); + } + /* * Foreign key constraints are checked in a final pass, since (a) it's * generally best to examine each one separately, and (b) it's at least @@ -19498,48 +19513,20 @@ ATExecSetCompression(Relation rel, return address; } - /* - * Preparation phase for SET LOGGED/UNLOGGED + * Do common checks for SET LOGGED/UNLOGGED. * - * This verifies that we're not trying to change a temp table. Also, - * existing foreign key constraints are checked to avoid ending up with - * permanent tables referencing unlogged tables. + * Ensure that unlogged tables are not named in publications and that the + * persistence of tables connected by foreign key constraints is compatible. */ static void -ATPrepChangePersistence(AlteredTableInfo *tab, Relation rel, bool toLogged) +CommonCheckForChangePersistence(Relation rel, bool toLogged) { Relation pg_constraint; HeapTuple tuple; SysScanDesc scan; ScanKeyData skey[1]; - /* - * Disallow changing status for a temp table. Also verify whether we can - * get away with doing nothing; in such cases we don't need to run the - * checks below, either. - */ - switch (rel->rd_rel->relpersistence) - { - case RELPERSISTENCE_TEMP: - ereport(ERROR, - (errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("cannot change logged status of table \"%s\" because it is temporary", - RelationGetRelationName(rel)), - errtable(rel))); - break; - case RELPERSISTENCE_PERMANENT: - if (toLogged) - /* nothing to do */ - return; - break; - case RELPERSISTENCE_UNLOGGED: - if (!toLogged) - /* nothing to do */ - return; - break; - } - /* * UNLOGGED tables can neither be published nor be named in a * publication's EXCEPT clause, so reject the change if the table is @@ -19619,6 +19606,45 @@ ATPrepChangePersistence(AlteredTableInfo *tab, Relation rel, bool toLogged) systable_endscan(scan); table_close(pg_constraint, AccessShareLock); +} + +/* + * Preparation phase for SET LOGGED/UNLOGGED + * + * This verifies that we're not trying to change a temp table. Also, + * existing foreign key constraints are checked to avoid ending up with + * permanent tables referencing unlogged tables. + */ +static void +ATPrepChangePersistence(AlteredTableInfo *tab, Relation rel, bool toLogged) +{ + /* + * Disallow changing status for a temp table. Also verify whether we can + * get away with doing nothing; in such cases we don't need to run the + * checks below, either. + */ + switch (rel->rd_rel->relpersistence) + { + case RELPERSISTENCE_TEMP: + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot change logged status of table \"%s\" because it is temporary", + RelationGetRelationName(rel)), + errtable(rel))); + break; + case RELPERSISTENCE_PERMANENT: + if (toLogged) + /* nothing to do */ + return; + break; + case RELPERSISTENCE_UNLOGGED: + if (!toLogged) + /* nothing to do */ + return; + break; + } + + CommonCheckForChangePersistence(rel, toLogged); /* force rewrite if necessary; see comment in ATRewriteTables */ tab->rewrite |= AT_REWRITE_ALTER_PERSISTENCE; @@ -19629,6 +19655,23 @@ ATPrepChangePersistence(AlteredTableInfo *tab, Relation rel, bool toLogged) tab->chgPersistence = true; } +/* + * Same as ATPrepChangePersistence, but for the rewrite phase. + */ +static void +ATRewriteChangePersistence(AlteredTableInfo *tab) +{ + Relation rel; + bool toLogged; + + rel = table_open(tab->relid, AccessShareLock); + toLogged = (tab->newrelpersistence == RELPERSISTENCE_PERMANENT); + + CommonCheckForChangePersistence(rel, toLogged); + + table_close(rel, AccessShareLock); +} + /* * Execute ALTER TABLE SET SCHEMA */ diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out index f57e8ffa7a5..bc05297f81a 100644 --- a/src/test/regress/expected/event_trigger.out +++ b/src/test/regress/expected/event_trigger.out @@ -639,6 +639,84 @@ ERROR: cannot alter type "rewritetype" because column "rewritemetoo3.a" uses it drop table rewriteme; drop event trigger no_rewrite_allowed; drop function test_evtrig_no_rewrite(); +-- Recheck persistence restrictions after table rewrite event triggers. +-- Case: adding a table to a publication during a rewrite event. +CREATE TABLE rewrite_pub_target (a int); +-- Suppress warning that depends on wal_level +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION rewrite_pub; +RESET client_min_messages; +-- Define a function which adds a table to the publication for a rewrite +-- event trigger. +CREATE FUNCTION test_evtrig_add_table() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + IF pg_event_trigger_table_rewrite_oid() = 'rewrite_pub_target'::regclass THEN + EXECUTE 'ALTER PUBLICATION rewrite_pub ADD TABLE rewrite_pub_target'; + END IF; +END; +$$; +CREATE EVENT TRIGGER add_table_during_rewrite ON table_rewrite + WHEN TAG IN ('ALTER TABLE') + EXECUTE FUNCTION test_evtrig_add_table(); +-- Should fail +ALTER TABLE rewrite_pub_target SET UNLOGGED; +ERROR: cannot change table "rewrite_pub_target" to unlogged because it is referenced by a publication +DETAIL: Unlogged relations cannot be published or excluded via an EXCEPT clause. +HINT: Drop the table from the publication, or remove it from the publication's EXCEPT clause, first. +-- Cleanup +DROP EVENT TRIGGER add_table_during_rewrite; +DROP FUNCTION test_evtrig_add_table(); +DROP PUBLICATION rewrite_pub; +DROP TABLE rewrite_pub_target; +-- Case: changing the referenced table to unlogged during a rewrite event. +CREATE TABLE rewrite_referenced (a int PRIMARY KEY); +CREATE UNLOGGED TABLE rewrite_to_logged (a int REFERENCES rewrite_referenced); +-- Define a function which sets a table to unlogged for a rewrite event +-- trigger. +CREATE FUNCTION test_evtrig_set_unlogged() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + IF pg_event_trigger_table_rewrite_oid() = 'rewrite_to_logged'::regclass THEN + EXECUTE 'ALTER TABLE rewrite_referenced SET UNLOGGED'; + END IF; +END; +$$; +CREATE EVENT TRIGGER set_unlogged_during_rewrite ON table_rewrite + WHEN TAG IN ('ALTER TABLE') + EXECUTE FUNCTION test_evtrig_set_unlogged(); +-- Should fail +ALTER TABLE rewrite_to_logged SET LOGGED; +ERROR: could not change table "rewrite_to_logged" to logged because it references unlogged table "rewrite_referenced" +-- Cleanup +DROP EVENT TRIGGER set_unlogged_during_rewrite; +DROP FUNCTION test_evtrig_set_unlogged(); +DROP TABLE rewrite_to_logged; +DROP TABLE rewrite_referenced; +-- Case: changing the referencing table to logged during a rewrite event. +CREATE TABLE rewrite_to_unlogged (a int PRIMARY KEY); +CREATE UNLOGGED TABLE rewrite_referencing (a int REFERENCES rewrite_to_unlogged); +-- Define a function which sets a table to logged for a rewrite event +-- trigger. +CREATE FUNCTION test_evtrig_set_logged() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + IF pg_event_trigger_table_rewrite_oid() = 'rewrite_to_unlogged'::regclass THEN + EXECUTE 'ALTER TABLE rewrite_referencing SET LOGGED'; + END IF; +END; +$$; +CREATE EVENT TRIGGER set_logged_during_rewrite ON table_rewrite + WHEN TAG IN ('ALTER TABLE') + EXECUTE FUNCTION test_evtrig_set_logged(); +-- Should fail +ALTER TABLE rewrite_to_unlogged SET UNLOGGED; +ERROR: could not change table "rewrite_to_unlogged" to unlogged because it references logged table "rewrite_referencing" +-- Cleanup +DROP EVENT TRIGGER set_logged_during_rewrite; +DROP FUNCTION test_evtrig_set_logged(); +DROP TABLE rewrite_referencing; +DROP TABLE rewrite_to_unlogged; -- Tests for REINDEX CREATE OR REPLACE FUNCTION reindex_start_command() RETURNS event_trigger AS $$ diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql index 32e9bb58c5e..bfa2f1f1cc1 100644 --- a/src/test/regress/sql/event_trigger.sql +++ b/src/test/regress/sql/event_trigger.sql @@ -476,6 +476,96 @@ drop table rewriteme; drop event trigger no_rewrite_allowed; drop function test_evtrig_no_rewrite(); +-- Recheck persistence restrictions after table rewrite event triggers. + +-- Case: adding a table to a publication during a rewrite event. +CREATE TABLE rewrite_pub_target (a int); + +-- Suppress warning that depends on wal_level +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION rewrite_pub; +RESET client_min_messages; + +-- Define a function which adds a table to the publication for a rewrite +-- event trigger. +CREATE FUNCTION test_evtrig_add_table() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + IF pg_event_trigger_table_rewrite_oid() = 'rewrite_pub_target'::regclass THEN + EXECUTE 'ALTER PUBLICATION rewrite_pub ADD TABLE rewrite_pub_target'; + END IF; +END; +$$; + +CREATE EVENT TRIGGER add_table_during_rewrite ON table_rewrite + WHEN TAG IN ('ALTER TABLE') + EXECUTE FUNCTION test_evtrig_add_table(); + +-- Should fail +ALTER TABLE rewrite_pub_target SET UNLOGGED; + +-- Cleanup +DROP EVENT TRIGGER add_table_during_rewrite; +DROP FUNCTION test_evtrig_add_table(); +DROP PUBLICATION rewrite_pub; +DROP TABLE rewrite_pub_target; + +-- Case: changing the referenced table to unlogged during a rewrite event. +CREATE TABLE rewrite_referenced (a int PRIMARY KEY); +CREATE UNLOGGED TABLE rewrite_to_logged (a int REFERENCES rewrite_referenced); + +-- Define a function which sets a table to unlogged for a rewrite event +-- trigger. +CREATE FUNCTION test_evtrig_set_unlogged() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + IF pg_event_trigger_table_rewrite_oid() = 'rewrite_to_logged'::regclass THEN + EXECUTE 'ALTER TABLE rewrite_referenced SET UNLOGGED'; + END IF; +END; +$$; + +CREATE EVENT TRIGGER set_unlogged_during_rewrite ON table_rewrite + WHEN TAG IN ('ALTER TABLE') + EXECUTE FUNCTION test_evtrig_set_unlogged(); + +-- Should fail +ALTER TABLE rewrite_to_logged SET LOGGED; + +-- Cleanup +DROP EVENT TRIGGER set_unlogged_during_rewrite; +DROP FUNCTION test_evtrig_set_unlogged(); +DROP TABLE rewrite_to_logged; +DROP TABLE rewrite_referenced; + +-- Case: changing the referencing table to logged during a rewrite event. +CREATE TABLE rewrite_to_unlogged (a int PRIMARY KEY); +CREATE UNLOGGED TABLE rewrite_referencing (a int REFERENCES rewrite_to_unlogged); + +-- Define a function which sets a table to logged for a rewrite event +-- trigger. +CREATE FUNCTION test_evtrig_set_logged() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + IF pg_event_trigger_table_rewrite_oid() = 'rewrite_to_unlogged'::regclass THEN + EXECUTE 'ALTER TABLE rewrite_referencing SET LOGGED'; + END IF; +END; +$$; + +CREATE EVENT TRIGGER set_logged_during_rewrite ON table_rewrite + WHEN TAG IN ('ALTER TABLE') + EXECUTE FUNCTION test_evtrig_set_logged(); + +-- Should fail +ALTER TABLE rewrite_to_unlogged SET UNLOGGED; + +-- Cleanup +DROP EVENT TRIGGER set_logged_during_rewrite; +DROP FUNCTION test_evtrig_set_logged(); +DROP TABLE rewrite_referencing; +DROP TABLE rewrite_to_unlogged; + -- Tests for REINDEX CREATE OR REPLACE FUNCTION reindex_start_command() RETURNS event_trigger AS $$ -- 2.47.3