Re: Skipping schema changes in publication

On Fri, 24 Oct 2025 at 06:41, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Vignesh,
>
> I had a look at patch v24-0001.
>
> FYI -- a rebase is needed
>
> [postgres(at)CentOS7-x64 oss_postgres_misc]$ git apply
> ../patches_misc/v24-0001-Add-RESET-clause-to-Alter-Publication-which-will.patch
> error: patch failed: doc/src/sgml/ref/alter_publication.sgml:69
> error: doc/src/sgml/ref/alter_publication.sgml: patch does not apply
>
> Here are some other review comments:
>
> ======
>
> 1.
> There seems to be some basic omission of the ALTER PUBLICATION in that
> it does not support "ALL TABLES" as a publication_object.
>
> Therefore, if you have:
> CREATE PUBLICATION mypub FOR ALL TABLES;
>
> and then you do:
> ALTER PUBLICATION mypub RESET;
>
> There seems to be no way to restore mpub to be an ALL TABLES publication again!
>
> ~~~
>
> I think if you are going to implement a RESET, then you also need a
> way to get back to what you had before you did the reset. So you'll
> also need to implement the ALTER PUBLICATION mypub SET ALL TABLES;
>
> IMO, supporting "SET ALL TABLES" should be your new 0001 patch
> because AFAIK, this situation already exists if the user had created
> an "empty" publication:
> CREATE PUBLICATION myemptypub;
>
With current patches we can add ALL TABLES to a publication using "ADD
ALL TABLES"
I think once the ADD ALL TABLES patch is committed, we can add SET ALL TABLES.

> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> 2.
> Probably need to mention ALL SEQUENCES now too?
>
> ======
> src/backend/commands/publicationcmds.c
>
> 3.
> +/* CREATE PUBLICATION default values for flags and publication parameters */
> +#define PUB_DEFAULT_ACTION_INSERT true
> +#define PUB_DEFAULT_ACTION_UPDATE true
> +#define PUB_DEFAULT_ACTION_DELETE true
> +#define PUB_DEFAULT_ACTION_TRUNCATE true
> +#define PUB_DEFAULT_VIA_ROOT false
> +#define PUB_DEFAULT_ALL_TABLES false
> +#define PUB_DEFAULT_GENCOLS PUBLISH_GENCOLS_NONE
> +
>
> Is it better to put all these in the catalog/pg_publication.h where
> the catalog was defined?
>
> ~~~
>
> AlterPublicationReset:
>
> 4.
> + /* Set ALL TABLES flag to false */
> + if (pubform->puballtables)
> + {
> + values[Anum_pg_publication_puballtables - 1] =
> BoolGetDatum(PUB_DEFAULT_ALL_TABLES);
> + replaces[Anum_pg_publication_puballtables - 1] = true;
> + CacheInvalidateRelcacheAll();
> + }
>
> Why not just do this anyway without the condition?
>
> ======
> src/backend/parser/gram.y
>
> 6.
> It would be nicer if all these grammar productions were coded in the
> same order as the comment above them.
>
> ======
> src/include/nodes/parsenodes.h
>
> 7.
> AP_AddObjects, /* add objects to publication */
> AP_DropObjects, /* remove objects from publication */
> AP_SetObjects, /* set list of objects */
> + AP_ResetPublication, /* reset the publication */
> } AlterPublicationAction;
>
> It is already called "AlterPublicationAction", so maybe the enum value
> only needs to be AP_Reset instead of AP_ResetPublication.
>
>
> ======
> src/test/regress/expected/publication.out
>
> 8.
> Expected output all needs rebasing now that there is a new "All
> sequences" column.
>
> ======
> src/test/regress/sql/publication.sql
>
> 9.
> +ALTER PUBLICATION testpub_reset ADD TABLE pub_sch1.tbl1;
> +
> +-- Verify that associated tables are removed from the publication after RESET
> +\dRp+ testpub_reset
> +ALTER PUBLICATION testpub_reset RESET;
> +\dRp+ testpub_reset
> +
>
> I felt the ADD TABLE should be after the comment.
>
> And ditto for all the other test cases -- should be that same pattern too.
>
> # comment about test
> ALTER .. do something
> \dRp+ pub
> ALTER .. RESET
> \dRp+ pub
>
> ~~~
>
> 10.
> +-- Verify that associated schemas are reomved from the publication after RESET
>
> typo: /reomved/removed/
>
> ~~~
>
> 11.
> +-- Verify that only superuser can reset a publication
> +ALTER PUBLICATION testpub_reset OWNER TO regress_publication_user2;
> +SET ROLE regress_publication_user2;
> +ALTER PUBLICATION testpub_reset RESET; -- fail - must be superuser
> +SET ROLE regress_publication_user;
> +
>
> Perhaps this should be the first test?
>
> ======

I have addressed the remaining comments in the latest v26 patch [1].
[1]: https://www.postgresql.org/message-id/CANhcyEWGiWwGt1-v6d9bCAae9Np7cNPt%2BiRV9PXBZ0z%3D75XEVw%40mail.gmail.com

Thanks,
Shlok Kyal