On Thu, 10 Sept 2026 at 14:55, shveta malik <[email protected]> wrote: > > On Thu, Sep 10, 2026 at 1:24 PM Chao Li <[email protected]> wrote: > > > > > > > > > On Sep 10, 2026, at 13:39, vignesh C <[email protected]> wrote: > > > > > > Finding #5: SET UNLOGGED on an excluded table produces an unrestorable > > > catalog state > > > > > > CREATE PUBLICATION ... FOR ALL TABLES EXCEPT (...) correctly rejects > > > unlogged tables. However, ALTER TABLE ... SET UNLOGGED does not > > > perform the same check. Its publication check uses > > > GetRelationIncludedPublications(), which ignores pg_publication_rel > > > rows marked as prexcept. > > > As a result, the following currently succeeds: > > > CREATE TABLE t (a int); > > > CREATE PUBLICATION p FOR ALL TABLES EXCEPT (TABLE t); > > > ALTER TABLE t SET UNLOGGED; > > > > > > This leaves an unlogged table in the publication's EXCEPT list, even > > > though an unlogged table cannot be added to an EXCEPT clause directly. > > > This causes failures when restoring a dump or running pg_upgrade. > > > For example, pg_dump produces: > > > CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT (TABLE ONLY public.t1) > > > WITH (publish = 'insert, update, delete, truncate'); > > > where t1 is an unlogged table. Restoring the dump fails with: > > > ERROR: cannot specify relation "public.t1" in the publication EXCEPT > > > clause > > > DETAIL: This operation is not supported for unlogged tables. > > > > > > A similar error is seen during pg_upgrade: > > > ERROR: cannot specify relation "public.t1" in the publication EXCEPT > > > clause > > > DETAIL: This operation is not supported for unlogged tables. > > > > > > The fix is to reject changing a table to UNLOGGED when it is > > > referenced in a publication's EXCEPT clause, similar to the existing > > > check for tables included in a publication. > > > > > > > > > Regards, > > > Vignesh > > > <v1-0001-Fix-ALTER-PUBLICATION-race-with-concurrent-SET-AL.patch><v1-0005-Prevent-unlogged-tables-in-publication-EXCEPT-cla.patch><v1-0003-Fix-missing-check-in-test_except_root_partition.patch><v1-0002-Fix-ALTER-PUBLICATION-validation-race.patch><v1-0004-Fix-test-to-use-a-fresh-subscription.patch> > > > > For v1-0005, the code change itself looks good to me. > > > > However, I have some concern about the design. Since a table in the EXCEPT > > list is not published anyway, do we really need to reject SET UNLOGGED? > > Would it make more sense to remove the table from the EXCEPT list and emit > > a NOTICE to inform the user? > > I had given a similar comment offlist yesterday, but upon rethinking, > I feel automatically removing the table from the EXCEPT list is > slightly riskier even with NOTICE given. The user may later change the > table back to LOGGED, in which case the publication semantics would > have changed silently; the table would now be published(for ALL TABLEs > case) even though the user never changed the publication > configuration. > > ~~ > > I think the proposed fix is also incomplete though for partitioned tables: > > CREATE TABLE root (a int) PARTITION BY RANGE (a); > CREATE TABLE part1 PARTITION OF root FOR VALUES FROM (1) TO (100); > > CREATE PUBLICATION root_pub FOR ALL TABLES EXCEPT (TABLE root); > > -- This fails: > ALTER TABLE root SET UNLOGGED; > > -- But this succeeds: > ALTER TABLE part1 SET UNLOGGED; > > Shouldn't the second command (for partition) fail too?
I don't think this is an issue. Only the top-most ancestor can appear in the EXCEPT clause, and we skip publishing based on the table's last_oid. There is also no issue with dump/restore. Regards, Vignesh
