From 7f549d0f72e1e296b1818da98b6a9c3fa1c87795 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi Date: Wed, 11 Nov 2026 22:19:48 +0000 Subject: [PATCH REL_17 v3 2/3] 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 | 58 ++++++++++++++++----- src/test/regress/expected/alter_table.out | 21 ++++++++ src/test/regress/expected/event_trigger.out | 26 +++++++++ src/test/regress/expected/foreign_key.out | 8 ++- src/test/regress/sql/alter_table.sql | 18 +++++++ src/test/regress/sql/event_trigger.sql | 25 +++++++++ src/test/regress/sql/foreign_key.sql | 6 ++- 7 files changed, 146 insertions(+), 16 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 88c0402c847..4138b86f771 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -626,6 +626,7 @@ static void ATExecDropCluster(Relation rel, LOCKMODE lockmode); static void ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname); static void ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethodId); static bool ATPrepChangePersistence(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); @@ -6005,6 +6006,24 @@ 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. Partitioned tables are included, as SET + * LOGGED/UNLOGGED checks their foreign keys even though nothing is + * changed for them. + */ + foreach(ltab, *wqueue) + { + AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab); + + if (tab->chgPersistence) + ATCheckPersistenceForeignKeys(tab); + } } /* @@ -17865,9 +17884,8 @@ 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. * * Return value is false if the operation is a no-op (in which case the * checks are skipped), otherwise true. @@ -17875,11 +17893,6 @@ ATExecSetCompression(Relation rel, static bool ATPrepChangePersistence(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 @@ -17918,11 +17931,28 @@ ATPrepChangePersistence(Relation rel, bool toLogged) RelationGetRelationName(rel)), errdetail("Unlogged relations cannot be replicated."))); - /* - * Check existing foreign key constraints to preserve the invariant that - * permanent tables cannot reference unlogged ones. Self-referencing - * foreign keys can safely be ignored. - */ + return 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); /* @@ -18003,7 +18033,7 @@ ATPrepChangePersistence(Relation rel, bool toLogged) table_close(pg_constraint, AccessShareLock); - return true; + table_close(rel, NoLock); } /* diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index c15a0fad1e9..81e8623b190 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -3861,6 +3861,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' @@ -3879,6 +3895,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 777f1660c0d..8fe8b89280d 100644 --- a/src/test/regress/expected/event_trigger.out +++ b/src/test/regress/expected/event_trigger.out @@ -629,6 +629,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 314155fd3c3..061dc9db1aa 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -3325,7 +3325,10 @@ DETAIL: Permanent table "fk" would reference unlogged table "pk1". CREATE UNLOGGED TABLE ufk (x int REFERENCES pk); ALTER TABLE ufk SET LOGGED; -- fails ERROR: could not change table "ufk" to logged because it references unlogged table "pk1" -DROP TABLE ufk, pk; +CREATE UNLOGGED TABLE ufk2 (x int); +ALTER TABLE ufk2 SET LOGGED, ADD FOREIGN KEY (x) REFERENCES pk; -- fails +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); CREATE UNLOGGED TABLE pk1 PARTITION OF pk FOR VALUES FROM (0) TO (100); -- fails @@ -3371,6 +3374,9 @@ ERROR: constraints on permanent tables may reference only permanent tables DETAIL: Permanent table "fk41" would reference unlogged table "pk". ALTER TABLE fk1 SET LOGGED; -- fails ERROR: could not change table "fk1" to logged because it references unlogged table "pk" +-- SET LOGGED on a partitioned table changes nothing, but is still checked +ALTER TABLE fk SET LOGGED; -- fails +ERROR: could not change table "fk" to logged because it references unlogged table "pk" DROP TABLE fk, fk3, fk4, pk; -- both sides partitioned CREATE TABLE pk (a int PRIMARY KEY) PARTITION BY RANGE (a); diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index d084aa8efd0..9c980e4d426 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -2460,6 +2460,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' @@ -2469,6 +2482,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 c613c0cfd43..53e8b2c342c 100644 --- a/src/test/regress/sql/event_trigger.sql +++ b/src/test/regress/sql/event_trigger.sql @@ -471,6 +471,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 eb1875b0908..10a5c376622 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -2394,7 +2394,9 @@ CREATE UNLOGGED TABLE pk1 PARTITION OF pk FOR VALUES FROM (0) TO (100); CREATE TABLE fk (x int REFERENCES pk); -- fails CREATE UNLOGGED TABLE ufk (x int REFERENCES pk); ALTER TABLE ufk SET LOGGED; -- fails -DROP TABLE ufk, pk; +CREATE UNLOGGED TABLE ufk2 (x int); +ALTER TABLE ufk2 SET LOGGED, ADD FOREIGN KEY (x) REFERENCES pk; -- fails +DROP TABLE ufk, ufk2, pk; CREATE TABLE pk (a int PRIMARY KEY) PARTITION BY RANGE (a); CREATE TABLE fk (x int REFERENCES pk); CREATE UNLOGGED TABLE pk1 PARTITION OF pk FOR VALUES FROM (0) TO (100); -- fails @@ -2424,6 +2426,8 @@ CREATE UNLOGGED TABLE fk4 (x int) PARTITION BY RANGE (x); CREATE TABLE fk41 PARTITION OF fk4 FOR VALUES FROM (100) TO (150); ALTER TABLE fk ATTACH PARTITION fk4 FOR VALUES FROM (100) TO (200); -- fails ALTER TABLE fk1 SET LOGGED; -- fails +-- SET LOGGED on a partitioned table changes nothing, but is still checked +ALTER TABLE fk SET LOGGED; -- fails DROP TABLE fk, fk3, fk4, pk; -- both sides partitioned CREATE TABLE pk (a int PRIMARY KEY) PARTITION BY RANGE (a); -- 2.56.0