| 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 |
| 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 |