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
