Re: Skipping schema changes in publication

From: Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>
To: Peter Smith <smithpb2250(at)gmail(dot)com>
Cc: vignesh C <vignesh21(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, YeXiu <1518981153(at)qq(dot)com>, Ian Lawrence Barwick <barwick(at)gmail(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Skipping schema changes in publication
Date: 2025-11-19 10:04:04
Message-ID: CANhcyEXCKPCAdoqBLAhxt64Nwf+7T52dd8daE3qvhBNTvro13Q@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, 14 Nov 2025 at 12:15, Peter Smith <smithpb2250(at)gmail(dot)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

Attachment Content-Type Size
v28-0002-Support-ADD-ALL-TABLES-in-ALTER-PUBLICATION.patch application/octet-stream 16.8 KB
v28-0001-Add-RESET-clause-to-Alter-Publication-which-will.patch application/octet-stream 17.7 KB
v28-0003-Skip-publishing-the-tables-specified-in-EXCEPT-T.patch application/octet-stream 70.9 KB
v28-0004-Skip-publishing-the-columns-specified-in-FOR-TAB.patch application/octet-stream 77.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Shlok Kyal 2025-11-19 10:05:05 Re: Skipping schema changes in publication
Previous Message Peter Eisentraut 2025-11-19 09:36:55 Re: backend/nodes cleanup: Move loop variables definitions into for statement