Review items for EXCEPT TABLE publication

On Thu, Sep 10, 2026 at 11:09 AM vignesh C <vignesh21@gmail.com> wrote:

I ran claude to identify issues related to the EXCEPT TABLE
publication changes.

Thanks for doing an AI based review of this feature. I'll look into
it. We can add an open item for this report.

--
With Regards,
Amit Kapila.

On Thu, 10 Sept 2026 at 11:15, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Sep 10, 2026 at 11:09 AM vignesh C <vignesh21@gmail.com> wrote:

I ran claude to identify issues related to the EXCEPT TABLE
publication changes.

Thanks for doing an AI based review of this feature. I'll look into
it. We can add an open item for this report.

Added "Review items for EXCEPT TABLE publication" open item at [1]https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items#Open_Issues.
[1]: https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items#Open_Issues

Regards,
Vignesh

On Sep 10, 2026, at 13:39, vignesh C <vignesh21@gmail.com> wrote:

Hi,

I ran claude to identify issues related to the EXCEPT TABLE
publication changes. After reviewing the findings, I found the
following issues that need to be fixed.
Finding #1: ALTER PUBLICATION race
AlterPublicationOptions() performs validation using the publication
tuple before acquiring the publication lock. A concurrent ALTER
PUBLICATION ... SET ALL TABLES can change puballtables while the
second command is waiting for the lock. Since the tuple is not
re-fetched after acquiring the lock, the validation can proceed based
on stale state while subsequent catalog lookups see the updated state.
In an assert-enabled build, this can trigger the assertion in
GetIncludedPublicationRelations() because of
"Assert(!GetPublication(pubid)->alltables)" and crash the backend.

Test to reproduce:
-- session 1: -- session 2:
CREATE PUBLICATION p;
BEGIN;
ALTER PUBLICATION p
SET ALL TABLES;
ALTER PUBLICATION p
SET
(publish_via_partition_root = false);
-- reads puballtables=false,
-- enters the branch
-- blocks while acquiring
-- AccessShareLock
COMMIT; -- resumes and assertion fires

The fix is to check the current publication state after acquiring the
publication lock.

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>

Since each commit addresses one finding, I’ll review and reply to the commits one by one.

For 0001, I have two comments:

1. I don’t think this fix completely resolves the race. With GetPublication(pubform->oid)->alltables moved after acquiring the lock, S2 has to wait for S1 to commit, so it can see the alltables change made by S1.

However, tup was fetched before AlterPublicationOptions() was called. After S1 commits its update, a new tuple version has been created, so the tup held by S2 is stale. I think S2 should re-fetch the publication tuple after acquiring the lock, so that both the validation and the subsequent catalog update operate on the current tuple version.

2. From a code-structure perspective, before this patch the relation-validation block was entered only when the publication was not FOR ALL TABLES. With this patch, the outer if is entered regardless of puballtables, and when GetPublication(pubform->oid)->alltables is true, root_relids is simply left empty.

This is not a problem today because the only code that follows is the foreach, which does nothing when root_relids is empty. However, if more code is added after the loop in the future, it may unintentionally run for an ALL TABLESpublication as well.

I think it would be clearer and safer to keep the whole relation-validation section, including the foreach, inside the post-lock !alltables branch.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Sep 10, 2026, at 13:39, vignesh C <vignesh21@gmail.com> wrote:

Finding #2: ALTER PUBLICATION validates against pre-lock state

CheckAlterPublication() validates that pg_publication_rel entries are
either all inclusions or all exclusions. However, this check is
performed before LockDatabaseObject(). A concurrent ALTER PUBLICATION
can therefore add an inclusion while another session is validating and
waiting for the lock. The second command then proceeds without
revalidating after the lock is acquired, leaving the publication with
a mixture of inclusion and exclusion entries.

Test to reproduce:
-- session 1: -- session 2:
CREATE PUBLICATION p;
BEGIN;
ALTER PUBLICATION p
ADD TABLE t2;
ALTER PUBLICATION p
SET ALL TABLES EXCEPT (TABLE t1);
-- passes CheckAlterPublication()
-- while s1's row is invisible,
-- then blocks on the lock
COMMIT; -- resumes without re-checking

The fix is to perform CheckAlterPublication() after acquiring the
publication lock, so that validation is performed against the current
publication state.

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>

v1-0002 looks correct and appears to fix the race described in finding 2.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Sep 10, 2026, at 13:39, vignesh C <vignesh21@gmail.com> wrote:

Finding #3: Missing check in test_except_root_partition

In test_except_root_partition, the test assigns the result of:
SELECT count(*) = 0 FROM pg_logical_slot_get_binary_changes(...)
to $result under a comment saying that it verifies the table is not
published, but there is no is() check. $result is then overwritten by
the following loop, so the check has no effect.

Added an is() check to verify the result of the query.

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>

v1-0003 looks good to me. The added is() correctly makes the previously ignored check effective. I also ran the test and it passed.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Sep 10, 2026, at 13:39, vignesh C <vignesh21@gmail.com> wrote:

Finding #4: Test uses a stale subscription

tap_sub is not dropped between the two multi-publication sub-tests. As
a result, the second CREATE SUBSCRIPTION tap_sub fails with "already
exists". Since the command uses psql(), the error is ignored.
The subsequent assertions therefore run against the existing
subscription rather than the intended fresh one. This means the test
does not exercise the intended scenario: a fresh subscription whose
initial COPY of tab1 must occur because tap_pub2 FOR ALL TABLES
overrides tap_pub1's EXCEPT clause.

Fixed this by dropping the subscription before recreating it.

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>

v1-0001 looks good to me. Changing to safe_psql() ensures that errors are not silently ignored, and dropping tap_sub ensures that the second case uses a fresh subscription as intended. I also ran the test, and it passed.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Sep 10, 2026, at 13:39, vignesh C <vignesh21@gmail.com> 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?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Thu, Sep 10, 2026 at 1:24 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Sep 10, 2026, at 13:39, vignesh C <vignesh21@gmail.com> 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?

thanks
Shveta

On Thu, Sep 10, 2026 at 1:24 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Sep 10, 2026, at 13:39, vignesh C <vignesh21@gmail.com> wrote:

Finding #5: SET UNLOGGED on an excluded table produces an unrestorable
catalog state

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 think removing the publication membership during another DDL will
unnecessarily widen the scope of publication memberships. For example,
consider, later one does, ALTER TABLE t SET LOGGED emits nothing, and
t is now published by p.

This is the damaging one and NOTICE in the previous message doesn't
prevent it. Either the subscriber lacks t, in which case apply fails
and the whole subscription
stalls, or it has t, in which case data the user deliberately excluded
starts flowing. Both are triggered by a local DDL statement with no
indication that replication scope just widened.

Also, it would be inconsistent with the INCLUDED case where we are
giving ERROR. I feel giving ERROR is the right thing to do here, so
that users can explicitly remove it from EXCEPT list and then later if
She wants to make table LOGGED again, She can execute following steps:
BEGIN;
ALTER TABLE t SET LOGGED;
ALTER PUBLICATION p SET ALL TABLES EXCEPT (TABLE t, ...);
COMMIT;

--
With Regards,
Amit Kapila.

patch 002:

Commit msg says:
----------
The command then re-fetches the publication tuple after acquiring the
lock, but does not revalidate the publication relations. This can leave
pg_publication_rel containing a mixture of inclusion and exclusion rows.

Re-run CheckAlterPublication() after acquiring the lock so that the
validation uses the current publication state.
---------

The commit message and the fix are not in sync and thus little
confusing. Commit msg talks about re-validation while patch has
shifted the validation to a later point. The code change looks good
though.

patch003: LGTM

patch004: LGTM

thanks
Shveta

On Sep 10, 2026, at 17:25, shveta malik <shveta.malik@gmail.com> wrote:

On Thu, Sep 10, 2026 at 1:24 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Sep 10, 2026, at 13:39, vignesh C <vignesh21@gmail.com> 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 actually considered that before I raised the comment. Now, to set a table unlogged, a user has to remove it from the EXPECT list manually, then if he sets the table logged again, the table will be published also, unless he remembers to move the table to the EXCEPT list again. So ends up the same result.

If we want to allow users to freely toggle logged/unlogged, maybe we need to leave unlogged table in EXPECT list.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Dear Vignesh,

I ran claude to identify issues related to the EXCEPT TABLE
publication changes.

Very nice. I think this should be part of the community's routine.

Finding #1: ALTER PUBLICATION race
Finding #2: ALTER PUBLICATION validates against pre-lock state

To confirm, why can't we acquire the lock at the beginning of AlterPublication()
and re-check the publication state? For now AlterPublicationOptions() acquires
the AccessShareLock only if the row filter and column list may be specified, but
it could cause elog(ERROR, "tuple concurrently updated") error. It cannot be
translated thus may be surprising. Instead, can we obtain AccessExclusive
(or ShareUpdateExclusiveLock, which can block each other) at the beginning to
block concurrent operations?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, Sep 10, 2026 at 12:41 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Sep 10, 2026, at 13:39, vignesh C <vignesh21@gmail.com> wrote:

Hi,

I ran claude to identify issues related to the EXCEPT TABLE
publication changes. After reviewing the findings, I found the
following issues that need to be fixed.
Finding #1: ALTER PUBLICATION race
AlterPublicationOptions() performs validation using the publication
tuple before acquiring the publication lock. A concurrent ALTER
PUBLICATION ... SET ALL TABLES can change puballtables while the
second command is waiting for the lock. Since the tuple is not
re-fetched after acquiring the lock, the validation can proceed based
on stale state while subsequent catalog lookups see the updated state.
In an assert-enabled build, this can trigger the assertion in
GetIncludedPublicationRelations() because of
"Assert(!GetPublication(pubid)->alltables)" and crash the backend.

Test to reproduce:
-- session 1: -- session 2:
CREATE PUBLICATION p;
BEGIN;
ALTER PUBLICATION p
SET ALL TABLES;
ALTER PUBLICATION p
SET
(publish_via_partition_root = false);
-- reads puballtables=false,
-- enters the branch
-- blocks while acquiring
-- AccessShareLock
COMMIT; -- resumes and assertion fires

The fix is to check the current publication state after acquiring the
publication lock.

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>

Since each commit addresses one finding, I’ll review and reply to the commits one by one.

For 0001, I have two comments:

1. I don’t think this fix completely resolves the race. With GetPublication(pubform->oid)->alltables moved after acquiring the lock, S2 has to wait for S1 to commit, so it can see the alltables change made by S1.

However, tup was fetched before AlterPublicationOptions() was called. After S1 commits its update, a new tuple version has been created, so the tup held by S2 is stale. I think S2 should re-fetch the publication tuple after acquiring the lock, so that both the validation and the subsequent catalog update operate on the current tuple version.

2. From a code-structure perspective, before this patch the relation-validation block was entered only when the publication was not FOR ALL TABLES. With this patch, the outer if is entered regardless of puballtables, and when GetPublication(pubform->oid)->alltables is true, root_relids is simply left empty.

This is not a problem today because the only code that follows is the foreach, which does nothing when root_relids is empty. However, if more code is added after the loop in the future, it may unintentionally run for an ALL TABLESpublication as well.

I agree here.

I think it would be clearer and safer to keep the whole relation-validation section, including the foreach, inside the post-lock !alltables branch.

+1.

thanks
Shveta

On Thu, 10 Sept 2026 at 15:11, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Vignesh,

I ran claude to identify issues related to the EXCEPT TABLE
publication changes.

Very nice. I think this should be part of the community's routine.

Finding #1: ALTER PUBLICATION race
Finding #2: ALTER PUBLICATION validates against pre-lock state

To confirm, why can't we acquire the lock at the beginning of AlterPublication()
and re-check the publication state? For now AlterPublicationOptions() acquires
the AccessShareLock only if the row filter and column list may be specified, but
it could cause elog(ERROR, "tuple concurrently updated") error. It cannot be
translated thus may be surprising. Instead, can we obtain AccessExclusive
(or ShareUpdateExclusiveLock, which can block each other) at the beginning to
block concurrent operations?

Thanks kuroda-san. I have addressed this in the v2 version patch
attached, however I did not change the lock mode and used the existing
AccessShareLock mode itself as I felt that should suffice. This
approach also addresses Chao's comments from [1]/messages/by-id/23BC758C-7DBC-4270-9232-2CC09C3679FB@gmail.com.
[1]: /messages/by-id/23BC758C-7DBC-4270-9232-2CC09C3679FB@gmail.com

Regards,
Vignesh

Dear Vignesh, Chao,

Finding #4: Test uses a stale subscription

Not critical, but DROP SUBSCRIPTION might be better to put before the DROP
PUBLICATION. Also related with the test, I feel below lines are not needed.

```
$node_subscriber->safe_psql('postgres', 'DROP SUBSCRIPTION tap_sub');
$node_publisher->safe_psql('postgres', 'DROP PUBLICATION tap_pub1');
$node_publisher->safe_psql('postgres', 'DROP PUBLICATION tap_pub2');

$node_publisher->stop('fast');
```

Finding #5: SET UNLOGGED on an excluded table produces an unrestorable
catalog state

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 prefer the Vignesh's idea. For now publications are created with extactly given
settings, but your idea may break the existing rule. Can you raise existing
examples?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, 10 Sept 2026 at 14:55, shveta malik <shveta.malik@gmail.com> wrote:

On Thu, Sep 10, 2026 at 1:24 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Sep 10, 2026, at 13:39, vignesh C <vignesh21@gmail.com> 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

On Thu, Sep 10, 2026 at 4:25 PM Chao Li <li.evan.chao@gmail.com> wrote:

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 actually considered that before I raised the comment. Now, to set a table unlogged, a user has to remove it from the EXPECT list manually, then if he sets the table logged again, the table will be published also, unless he remembers to move the table to the EXCEPT list again. So ends up the same result.

But if She explicitly removes it from the EXCEPT list then it will be
the user's responsibility to add it again. This is better than a
silent behavior.

If we want to allow users to freely toggle logged/unlogged, maybe we need to leave unlogged table in EXPECT list.

The biggest challenge with this is to have a dangling entry in
pg_publication_rel. If nothing else, the callers of
GetExcludedPublicationTables() should be careful that it can contain
unlogged relations. The existing callers are immune to this but all
the future callers need to be aware of the same and may need
additional handling. I feel this approach to fix the issue is doable
but adds more maintenance burden.

--
With Regards,
Amit Kapila.

Dear Vignesh,

Thanks kuroda-san. I have addressed this in the v2 version patch
attached, however I did not change the lock mode and used the existing
AccessShareLock mode itself as I felt that should suffice. This
approach also addresses Chao's comments from [1].

My intention was similar but can we unify codes for re-validation?
My idea attached here could be applied atop v2.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, Sep 10, 2026 at 5:11 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Vignesh,

Thanks kuroda-san. I have addressed this in the v2 version patch
attached, however I did not change the lock mode and used the existing
AccessShareLock mode itself as I felt that should suffice. This
approach also addresses Chao's comments from [1].

My intention was similar but can we unify codes for re-validation?
My idea attached here could be applied atop v2.

+1. I had similar idea in mind about this patch's correction that we
can pull the code part out in AlterPublication() for both 'if' and
'else' block.

thanks
Shveta