From fa9fb1c9f3a54ed11c3f6f18a2b27f17d80333c5 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi Date: Wed, 11 Nov 2026 13:24:22 +0000 Subject: [PATCH v3 3/4] Check foreign keys after adding them in ALTER TABLE SET LOGGED Previously, ALTER TABLE ... SET LOGGED checked the table's foreign keys in the preparation phase, before any subcommand had run. A foreign key added by the same command did not exist yet, and when it was added the table was still unlogged, so it could reference an unlogged table. For example, the following left fk permanent while still referencing the unlogged table upk: ALTER TABLE fk SET LOGGED, ADD FOREIGN KEY (x) REFERENCES upk; User code running later in the same command had the same effect: a table_rewrite event trigger or a USING expression could add a foreign key, or make the referenced table unlogged, while the table still had its old persistence. The same applies to SET UNLOGGED. After a crash, the referenced rows are gone but the referencing ones remain. Fix this by checking the foreign keys at the end of phase 3, after the rewrites and everything else the command runs. This has been possible since commit f41872d0c12 added SET LOGGED. Backpatch to all supported versions. --- src/backend/commands/tablecmds.c | 68 +++++++++++++++------ src/test/regress/expected/alter_table.out | 21 +++++++ src/test/regress/expected/event_trigger.out | 26 ++++++++ src/test/regress/expected/foreign_key.out | 9 ++- src/test/regress/sql/alter_table.sql | 18 ++++++ src/test/regress/sql/event_trigger.sql | 25 ++++++++ src/test/regress/sql/foreign_key.sql | 7 ++- 7 files changed, 152 insertions(+), 22 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 0d246054cd1..248fbafa2c3 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -691,6 +691,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 ATCheckPersistenceForeignKeys(AlteredTableInfo *tab); static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, const char *tablespacename, LOCKMODE lockmode); static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode); @@ -6185,6 +6186,22 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode, CommandCounterIncrement(); } } + + /* + * Check the foreign keys of tables changing persistence last. Foreign + * keys added by this command must be seen, and so must anything done by + * user code that ran above, such as table_rewrite event triggers or + * expressions evaluated during a rewrite: those can add foreign keys, or + * change the persistence of the other side, while this table still has + * its old persistence. + */ + foreach(ltab, *wqueue) + { + AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab); + + if (tab->chgPersistence) + ATCheckPersistenceForeignKeys(tab); + } } /* @@ -19537,18 +19554,12 @@ ATExecSetCompression(Relation rel, /* * 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. + * This verifies that we're not trying to change a temp table. Foreign keys + * are checked later, by ATCheckPersistenceForeignKeys. */ static void ATPrepChangePersistence(AlteredTableInfo *tab, 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 @@ -19588,11 +19599,34 @@ ATPrepChangePersistence(AlteredTableInfo *tab, Relation rel, bool toLogged) errdetail("Unlogged relations cannot be published or excluded via an EXCEPT clause."), errhint("Drop the table from the publication, or remove it from the publication's EXCEPT clause, first."))); - /* - * Check existing foreign key constraints to preserve the invariant that - * permanent tables cannot reference unlogged ones. Self-referencing - * foreign keys can safely be ignored. - */ + /* force rewrite if necessary; see comment in ATRewriteTables */ + tab->rewrite |= AT_REWRITE_ALTER_PERSISTENCE; + if (toLogged) + tab->newrelpersistence = RELPERSISTENCE_PERMANENT; + else + tab->newrelpersistence = RELPERSISTENCE_UNLOGGED; + tab->chgPersistence = true; +} + +/* + * Check foreign keys for SET LOGGED/UNLOGGED + * + * This preserves the invariant that permanent tables cannot reference + * unlogged ones. It runs at the end of phase 3 rather than at preparation; + * see ATRewriteTables. + */ +static void +ATCheckPersistenceForeignKeys(AlteredTableInfo *tab) +{ + bool toLogged = (tab->newrelpersistence == RELPERSISTENCE_PERMANENT); + Relation rel; + Relation pg_constraint; + HeapTuple tuple; + SysScanDesc scan; + ScanKeyData skey[1]; + + rel = table_open(tab->relid, NoLock); + pg_constraint = table_open(ConstraintRelationId, AccessShareLock); /* @@ -19655,13 +19689,7 @@ ATPrepChangePersistence(AlteredTableInfo *tab, Relation rel, bool toLogged) table_close(pg_constraint, AccessShareLock); - /* force rewrite if necessary; see comment in ATRewriteTables */ - tab->rewrite |= AT_REWRITE_ALTER_PERSISTENCE; - if (toLogged) - tab->newrelpersistence = RELPERSISTENCE_PERMANENT; - else - tab->newrelpersistence = RELPERSISTENCE_UNLOGGED; - tab->chgPersistence = true; + table_close(rel, NoLock); } /* diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 20707c186b3..9f7251effc3 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -3676,6 +3676,22 @@ CREATE UNLOGGED TABLE unlogged3(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES unl ALTER TABLE unlogged3 SET LOGGED; -- skip self-referencing foreign key ALTER TABLE unlogged2 SET LOGGED; -- fails because a foreign key to an unlogged table exists ERROR: could not change table "unlogged2" to logged because it references unlogged table "unlogged1" +CREATE UNLOGGED TABLE unlogged4(f1 INTEGER); +ALTER TABLE unlogged4 SET LOGGED, ADD FOREIGN KEY (f1) REFERENCES unlogged1; -- fails, same with a foreign key added by the same command +ERROR: could not change table "unlogged4" to logged because it references unlogged table "unlogged1" +ALTER TABLE unlogged4 ADD FOREIGN KEY (f1) REFERENCES unlogged1, SET LOGGED; -- fails +ERROR: could not change table "unlogged4" to logged because it references unlogged table "unlogged1" +ALTER TABLE unlogged4 SET LOGGED, ADD FOREIGN KEY (f1) REFERENCES unlogged3; -- ok, unlogged3 is permanent now +CREATE UNLOGGED TABLE unlogged5(f1 INTEGER PRIMARY KEY, f2 INTEGER); +ALTER TABLE unlogged5 SET LOGGED, ADD FOREIGN KEY (f2) REFERENCES unlogged5; -- skip self-referencing foreign key +CREATE TABLE logged6(f1 INTEGER PRIMARY KEY); +CREATE UNLOGGED TABLE unlogged6(f1 INTEGER REFERENCES logged6, f2 INTEGER); +INSERT INTO logged6 VALUES (1); +INSERT INTO unlogged6 VALUES (1, 1); +CREATE FUNCTION unlog_logged6(int) RETURNS bigint LANGUAGE plpgsql AS +$$ BEGIN ALTER TABLE logged6 SET UNLOGGED; RETURN $1; END $$; +ALTER TABLE unlogged6 SET LOGGED, ALTER f2 TYPE bigint USING unlog_logged6(f2); -- fails, logged6 became unlogged during the rewrite +ERROR: could not change table "unlogged6" to logged because it references unlogged table "logged6" ALTER TABLE unlogged1 SET LOGGED; -- check relpersistence of an unlogged table after changing to permanent SELECT relname, relkind, relpersistence FROM pg_class WHERE relname ~ '^unlogged1' @@ -3694,6 +3710,11 @@ ORDER BY relname; (5 rows) ALTER TABLE unlogged1 SET LOGGED; -- silently do nothing +DROP TABLE unlogged6; +DROP TABLE logged6; +DROP FUNCTION unlog_logged6(int); +DROP TABLE unlogged5; +DROP TABLE unlogged4; DROP TABLE unlogged3; DROP TABLE unlogged2; DROP TABLE unlogged1; diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out index f57e8ffa7a5..d24d78450d0 100644 --- a/src/test/regress/expected/event_trigger.out +++ b/src/test/regress/expected/event_trigger.out @@ -639,6 +639,32 @@ 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(); +-- a table_rewrite event trigger cannot be used to add a foreign key between +-- a permanent and an unlogged table, while the table being rewritten still +-- has its old persistence +CREATE UNLOGGED TABLE rewrite_upk (id int PRIMARY KEY); +CREATE UNLOGGED TABLE rewrite_ufk (id int); +CREATE TABLE rewrite_ppk (id int PRIMARY KEY); +CREATE TABLE rewrite_pfk (id int); +CREATE FUNCTION test_evtrig_add_fk() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + IF pg_event_trigger_table_rewrite_oid() = 'rewrite_ufk'::regclass THEN + ALTER TABLE rewrite_ufk ADD FOREIGN KEY (id) REFERENCES rewrite_upk; + ELSIF pg_event_trigger_table_rewrite_oid() = 'rewrite_ppk'::regclass THEN + ALTER TABLE rewrite_pfk ADD FOREIGN KEY (id) REFERENCES rewrite_ppk; + END IF; +END; +$$; +CREATE EVENT TRIGGER add_fk ON table_rewrite + EXECUTE FUNCTION test_evtrig_add_fk(); +ALTER TABLE rewrite_ufk SET LOGGED; -- fails +ERROR: could not change table "rewrite_ufk" to logged because it references unlogged table "rewrite_upk" +ALTER TABLE rewrite_ppk SET UNLOGGED; -- fails +ERROR: could not change table "rewrite_ppk" to unlogged because it references logged table "rewrite_pfk" +DROP EVENT TRIGGER add_fk; +DROP FUNCTION test_evtrig_add_fk(); +DROP TABLE rewrite_ufk, rewrite_upk, rewrite_pfk, rewrite_ppk; -- Tests for REINDEX CREATE OR REPLACE FUNCTION reindex_start_command() RETURNS event_trigger AS $$ diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 4028fab4bd8..9b29b9dc6ae 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -4280,7 +4280,14 @@ ERROR: constraints on permanent tables may reference only permanent tables DETAIL: Table "pk1" is an unlogged partition of a table referenced by permanent table "fk". -- ok: an unlogged table may reference unlogged partitions CREATE UNLOGGED TABLE ufk (x int REFERENCES pk); -DROP TABLE ufk, pk; +-- fails: the referencing table cannot be made permanent afterwards +ALTER TABLE ufk SET LOGGED; +ERROR: could not change table "ufk" to logged because it references unlogged table "pk1" +-- fails: nor in the same command that adds the constraint +CREATE UNLOGGED TABLE ufk2 (x int); +ALTER TABLE ufk2 SET LOGGED, ADD FOREIGN KEY (x) REFERENCES pk; +ERROR: could not change table "ufk2" to logged because it references unlogged table "pk1" +DROP TABLE ufk, ufk2, pk; CREATE TABLE pk (a int PRIMARY KEY) PARTITION BY RANGE (a); CREATE TABLE fk (x int REFERENCES pk); -- fails: creating an unlogged partition of the referenced table diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 18f3fdac57a..3267ee728e7 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -2278,6 +2278,19 @@ CREATE UNLOGGED TABLE unlogged2(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES unl CREATE UNLOGGED TABLE unlogged3(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES unlogged3); -- self-referencing foreign key ALTER TABLE unlogged3 SET LOGGED; -- skip self-referencing foreign key ALTER TABLE unlogged2 SET LOGGED; -- fails because a foreign key to an unlogged table exists +CREATE UNLOGGED TABLE unlogged4(f1 INTEGER); +ALTER TABLE unlogged4 SET LOGGED, ADD FOREIGN KEY (f1) REFERENCES unlogged1; -- fails, same with a foreign key added by the same command +ALTER TABLE unlogged4 ADD FOREIGN KEY (f1) REFERENCES unlogged1, SET LOGGED; -- fails +ALTER TABLE unlogged4 SET LOGGED, ADD FOREIGN KEY (f1) REFERENCES unlogged3; -- ok, unlogged3 is permanent now +CREATE UNLOGGED TABLE unlogged5(f1 INTEGER PRIMARY KEY, f2 INTEGER); +ALTER TABLE unlogged5 SET LOGGED, ADD FOREIGN KEY (f2) REFERENCES unlogged5; -- skip self-referencing foreign key +CREATE TABLE logged6(f1 INTEGER PRIMARY KEY); +CREATE UNLOGGED TABLE unlogged6(f1 INTEGER REFERENCES logged6, f2 INTEGER); +INSERT INTO logged6 VALUES (1); +INSERT INTO unlogged6 VALUES (1, 1); +CREATE FUNCTION unlog_logged6(int) RETURNS bigint LANGUAGE plpgsql AS +$$ BEGIN ALTER TABLE logged6 SET UNLOGGED; RETURN $1; END $$; +ALTER TABLE unlogged6 SET LOGGED, ALTER f2 TYPE bigint USING unlog_logged6(f2); -- fails, logged6 became unlogged during the rewrite ALTER TABLE unlogged1 SET LOGGED; -- check relpersistence of an unlogged table after changing to permanent SELECT relname, relkind, relpersistence FROM pg_class WHERE relname ~ '^unlogged1' @@ -2287,6 +2300,11 @@ UNION ALL SELECT r.relname || ' toast index', ri.relkind, ri.relpersistence FROM pg_class r join pg_class t ON t.oid = r.reltoastrelid JOIN pg_index i ON i.indrelid = t.oid JOIN pg_class ri ON ri.oid = i.indexrelid WHERE r.relname ~ '^unlogged1' ORDER BY relname; ALTER TABLE unlogged1 SET LOGGED; -- silently do nothing +DROP TABLE unlogged6; +DROP TABLE logged6; +DROP FUNCTION unlog_logged6(int); +DROP TABLE unlogged5; +DROP TABLE unlogged4; DROP TABLE unlogged3; DROP TABLE unlogged2; DROP TABLE unlogged1; diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql index 32e9bb58c5e..ad82a548ea5 100644 --- a/src/test/regress/sql/event_trigger.sql +++ b/src/test/regress/sql/event_trigger.sql @@ -476,6 +476,31 @@ drop table rewriteme; drop event trigger no_rewrite_allowed; drop function test_evtrig_no_rewrite(); +-- a table_rewrite event trigger cannot be used to add a foreign key between +-- a permanent and an unlogged table, while the table being rewritten still +-- has its old persistence +CREATE UNLOGGED TABLE rewrite_upk (id int PRIMARY KEY); +CREATE UNLOGGED TABLE rewrite_ufk (id int); +CREATE TABLE rewrite_ppk (id int PRIMARY KEY); +CREATE TABLE rewrite_pfk (id int); +CREATE FUNCTION test_evtrig_add_fk() RETURNS event_trigger +LANGUAGE plpgsql AS $$ +BEGIN + IF pg_event_trigger_table_rewrite_oid() = 'rewrite_ufk'::regclass THEN + ALTER TABLE rewrite_ufk ADD FOREIGN KEY (id) REFERENCES rewrite_upk; + ELSIF pg_event_trigger_table_rewrite_oid() = 'rewrite_ppk'::regclass THEN + ALTER TABLE rewrite_pfk ADD FOREIGN KEY (id) REFERENCES rewrite_ppk; + END IF; +END; +$$; +CREATE EVENT TRIGGER add_fk ON table_rewrite + EXECUTE FUNCTION test_evtrig_add_fk(); +ALTER TABLE rewrite_ufk SET LOGGED; -- fails +ALTER TABLE rewrite_ppk SET UNLOGGED; -- fails +DROP EVENT TRIGGER add_fk; +DROP FUNCTION test_evtrig_add_fk(); +DROP TABLE rewrite_ufk, rewrite_upk, rewrite_pfk, rewrite_ppk; + -- Tests for REINDEX CREATE OR REPLACE FUNCTION reindex_start_command() RETURNS event_trigger AS $$ diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 339fc374df5..66fc5cd4d89 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -3207,7 +3207,12 @@ CREATE UNLOGGED TABLE pk1 PARTITION OF pk FOR VALUES FROM (0) TO (100); CREATE TABLE fk (x int REFERENCES pk); -- ok: an unlogged table may reference unlogged partitions CREATE UNLOGGED TABLE ufk (x int REFERENCES pk); -DROP TABLE ufk, pk; +-- fails: the referencing table cannot be made permanent afterwards +ALTER TABLE ufk SET LOGGED; +-- fails: nor in the same command that adds the constraint +CREATE UNLOGGED TABLE ufk2 (x int); +ALTER TABLE ufk2 SET LOGGED, ADD FOREIGN KEY (x) REFERENCES pk; +DROP TABLE ufk, ufk2, pk; CREATE TABLE pk (a int PRIMARY KEY) PARTITION BY RANGE (a); CREATE TABLE fk (x int REFERENCES pk); -- fails: creating an unlogged partition of the referenced table -- 2.56.0