Review items for EXCEPT TABLE publication

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.

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.

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.

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.

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.

I have attached patches for these issues:
Finding #1: v1-0001-Fix-ALTER-PUBLICATION-race-with-concurrent-SET-AL.patch
Finding #2: v1-0002-Fix-ALTER-PUBLICATION-validation-race.patch
Finding #3: v1-0003-Fix-missing-check-in-test_except_root_partition.patch
Finding #4: v1-0004-Fix-test-to-use-a-fresh-subscription.patch
Finding #5: v1-0005-Prevent-unlogged-tables-in-publication-EXCEPT-cla.patch

Regards,
Vignesh