Re: Skipping schema changes in publication - Mailing list pgsql-hackers

On Mon, Apr 20, 2026 at 10:13 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Hi,
>
> When changing a table to UNLOGGED, tables that appear in publications
> via EXCEPT clauses (prexcept = true) are currently allowed, but their
> entries remain in pg_publication_rel.
>
> For example:
> postgres=# create table t1(c1 int);
> CREATE TABLE
> postgres=# create publication pub1 for all tables except (table t1);
> CREATE PUBLICATION
> postgres=# alter table t1 set unlogged;
> ALTER TABLE
> postgres=# \d t1
>             Unlogged table "public.t1"
>  Column |  Type   | Collation | Nullable | Default
> --------+---------+-----------+----------+---------
>  c1     | integer |           |          |
> Except publications:
>     "pub1"
>
> Since UNLOGGED tables are not supported in publications, this leaves
> stale catalog entries. This patch removes such entries from
> pg_publication_rel when the table is changed to UNLOGGED, and emits a
> NOTICE to inform the user.
>
> Another option considered was to throw an error when setting such
> tables to UNLOGGED. However, allowing the operation was preferred,
> since UNLOGGED tables do not generate WAL and are not replicated
> anyway, so blocking the operation would be unnecessarily restrictive.
>
> Attached patch has the changes for the same.
>
> Thoughts?
>

Hi Vignesh -

A couple of review comments for patch v1-0001.

======
Background: I found when altering a published table from LOGGED to UNLOGGED...

- If it was published by "FOR ALL TABLES" then it just becomes
unpublished *silently*.
- Ditto if it was published by "FOR TABLES IN SCHEMA"
- It seems you only got an error on ALTER LOGGED->UNLOGGED when the
published table was specifically published by "FOR TABLE".

======
src/backend/commands/tablecmds.c

1.
+ /* Find all publications associated with the relation. */
+ pubrellist = SearchSysCacheList1(PUBLICATIONRELMAP,
+ ObjectIdGetDatum(RelationGetRelid(rel)));

This comment seems misleading. IIUC, it's only going to find relations
specifically mentioned by name in the command. Any relations that are
associated with the publication due to "FOR ALL TABLES" or "FOR TABLES
IN SCHEMA" are not known.

~~~

2.
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot change table \"%s\" to unlogged because it is part of
a publication",
+ RelationGetRelationName(rel)),
+ errdetail("Unlogged relations cannot be replicated.")))

This does not seem like a very good error message in the first place
(yes, I know this patch did not change it).

e.g.

a) It says it is "part of a publication", but it does not tell the
user which ones. Maybe list them in the message or provide a HINT to
say use \d to show them.

b) It mixes terminology, first calling it a 'table', and then calling
it a 'relation'

~~~

3.
+ ereport(NOTICE,
+ errmsg("relation \"%s\" removed from publication \"%s\" due to being
changed to UNLOGGED",
+    RelationGetRelationName(rel), pubname));

After some consideration, I think that this really should be an ERROR
too, not just a NOTICE.

Otherwise, if the user toggled the excluded table from
LOGGED->UNLOGGED->LOGGED, then the same table would go from being
excluded to being included! The consequences of publishing something
that was originally *specifically* excluded by the user might be very
bad, so IMO it's better to prevent that from happening -- i.e. sharing
the same ERROR code that was there before.

======
Kind Regards,
Peter Smith.
Fujitsu Australia