Re: Skipping schema changes in publication

Shlok Kyal <shlok.kyal.oss@gmail.com>

From: Shlok Kyal <shlok.kyal.oss@gmail.com>

To: Peter Smith <smithpb2250@gmail.com>

Cc: vignesh C <vignesh21@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, YeXiu <1518981153@qq.com>, Ian Lawrence Barwick <barwick@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Date: 2025-11-19T10:04:04Z

Lists: pgsql-hackers

Attachments

v28-0002-Support-ADD-ALL-TABLES-in-ALTER-PUBLICATION.patch (application/octet-stream)
v28-0001-Add-RESET-clause-to-Alter-Publication-which-will.patch (application/octet-stream)
v28-0003-Skip-publishing-the-tables-specified-in-EXCEPT-T.patch (application/octet-stream)
v28-0004-Skip-publishing-the-columns-specified-in-FOR-TAB.patch (application/octet-stream)

On Fri, 14 Nov 2025 at 12:15, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Shlok.
>
> Some review comments for patch v27-0001.
>
> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> 1.
> +  <para>
> +   The <literal>RESET</literal> clause will reset the publication to
> the default
> +   state. This includes resetting all publication parameters, setting the
> +   <literal>ALL TABLES</literal> and <literal>ALL SEQUENCES</literal> flags to
> +   <literal>false</literal>, and removing all associated tables and
> schemas from
> +   the publication.
>    </para>
>
> It would be better to give references to the actual
> pg_publication.puballtables and .puballsequences flag fields [1]
> instead of vaguely calling them the "<literal>ALL TABLES</literal> and
> <literal>ALL SEQUENCES</literal> flags".
>
Fixed

> ======
> src/backend/commands/publicationcmds.c
>
> AlterPublicationReset:
>
> 2.
> + if (pubform->puballtables)
> + CacheInvalidateRelcacheAll();
>
> Does that also need to check ->puballsequences?
>
I think we call CacheInvalidateRelcacheAll to invalide the relsync
cache for the case of ALTER Publication. For sequences we do not build
RelSyncEntry.
Also I see there are other similar occurrences (such as
RemovePublicationById, AlterPublicationOptions) where we do not
invalidate cache if we modify all sequence publications.
So, I think we do not require this check for puballsequences.

> ======
> src/test/regress/sql/publication.sql
>
> 3.
> If you want to, you can easily combine many of these test cases and
> verify them in one go instead of separate ALTER/RESET for every kind
> of flag.
>
> ~~~
>
I agree. I have made the changes in the latest patch.

> 4.
> +-- Verify that 'ALL TABLES' flag is reset
>
> Missing test to check the 'ALL SEQUENCES' flag gets reset?
>
Added the test.

> ======
> [1] https://www.postgresql.org/docs/devel/catalog-pg-publication.html
>

I have also addressed the comments in [1], [2].

[1]: https://www.postgresql.org/message-id/CAHut%2BPtRzCD4-0894cutkU_h8cPNtosN0_oSHn2iAKEfg2ENOQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAHut+PuHn-hohA4OdEJz+Zfukfr41TvMTeTH7NwJ=wg1+94uNA@mail.gmail.com

Thanks,
Shlok Kyal