MIME-Version: 1.0
References: 
 <CALDaNm3=JrucjhiiwsYQw5-PGtBHFONa6F7hhWCXMsGvh=tamA@mail.gmail.com>
 <CALj2ACVOzhs+BD+abFV2x4oKJdsDNd6SgsE7r8UjnZDCKGEckA@mail.gmail.com>
 <CAA4eK1K6Kr88d2S0zFdHRMyuoaZeNh+ktU+oigmCuD09_x_-+g@mail.gmail.com>
 <CAHut+PsvC-NezO3MJkdyEz=G1QRje2LntjwhQiEeVbmhOQuBMA@mail.gmail.com>
 <CALDaNm18VH2j8cTqfELHQ=0ZNognbGBhbHPteJenWQC6C2dueQ@mail.gmail.com>
 <CALDaNm0k_0Ccj47wzJzzPFwgQB7w=R5+Q2_nSqYrmMmjhmcRUw@mail.gmail.com>
 <CAHut+Pv_0DwyWoGQNMF+G2AGqMuJTzWQKRtmxaC+=zLTPL-Zkw@mail.gmail.com>
 <CALDaNm2-GJt2HsYTkLqQ=ecm=R-vOBw1=aM_d2EiYbz39x_cTQ@mail.gmail.com>
 <TYCPR01MB8373C3120C2B3112001ED6F1EDCF9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CALDaNm0iZZDB300Dez_97S8G6_RW5QpQ8ef6X3wq8tyK-8wnXQ@mail.gmail.com>
 <CAHut+PtiomM+iyAZHvb2dzfsPvRru266KuBe49hKy2n2h+m_zA@mail.gmail.com>
 <CALDaNm30KDnwX4Czi29fqLb8JBkuwqjbpj9ixwNXXox574NZqQ@mail.gmail.com>
 <CALDaNm1PfKRJsEzbKpyt=v4p3bw+_SzE+LFPsMhR5X+qs+0pPw@mail.gmail.com>
 <TYCPR01MB83730A2F1D6A5303E9C1416AEDD99@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CALDaNm0sAU4s1KTLOEWv=rYo5dQK6uFTJn_0FKj3XG1Nv4D-qw@mail.gmail.com>
 <CALDaNm3CLRa95tpas6tEj8x58MUNDShxBNoYS+P8Uq5cryoAOw@mail.gmail.com>
 <CALDaNm0EKC3o=v+F7GneGibuCULGKkBWXmNaVB4GR9HoqD066A@mail.gmail.com>
 <CALDaNm1Z1Rmqj9s6P9ZzmrVA9F_vZ_DwwhYAJmsjqmY6dS3-hA@mail.gmail.com>
 <CAB8KJ=jJGuW=ozKmXZzKDUHZ_-J2ZYGOtJo=i2cnNbSu6=KuYg@mail.gmail.com>
 <CALDaNm1mbFP8fxHU_H1Ex4cT2Aq3n8FE79tq0TO5ThvFnDUYMA@mail.gmail.com>
 <CAB8KJ=jq4RwTs8K7pokmXQwQppP2ChVJLMSAdXaxAX+c1r+mdg@mail.gmail.com>
 <CALDaNm1mJvLni8GODebKBmyegXuZ18bLoG-Pz6H1MCX=vphCYA@mail.gmail.com>
 <CALDaNm3dWZCYDih55qTNAYsjCvYXMFv=46UsDWmfCnXMt3kPCg@mail.gmail.com>
 <CALDaNm1AQZYgT0tALRrkvpP1Q+8+e7vkGCUjQ-jim1C0q3e=zA@mail.gmail.com>
 <CAA4eK1KRdAPC=5=7tQ1GW0cRwD=zaDMi+T4u_k4GxPhPY6e8BQ@mail.gmail.com>
 <OS3PR01MB5718C8BE84B862E7E0CEC29B94BD2@OS3PR01MB5718.jpnprd01.prod.outlook.com>
 <CAA4eK1KYQz7cf46_D=6VkZ4J6Y8vJ88MMi=6zm2TJXDP+V1mLg@mail.gmail.com>
 <CANhcyEXZq4mP5dNgg7u=sMPwvxA4_ZN9U92uZEuzs=0xTu+8Yg@mail.gmail.com>
 <CANhcyEXspT3v5-Tdop9uqQV2HWBvZoN5P0BxXQ6Md6Mr7GXK9A@mail.gmail.com>
 <CAHut+PuiaLOCkiAx9nPnjk6wTbPFvnm9T5svTuKbgwJwTdea8w@mail.gmail.com>
 <CANhcyEV_MePxgftHY65et1WdOAk70M0C7PZ1STPUO8PXHVB1YA@mail.gmail.com>
 <CAHut+Ps0hSNqrjv_jT1AuXxO-CrZue3ixE0jKsxVhtArMrkujQ@mail.gmail.com>
 <CANhcyEXX3viVpYcGHD_fzhf_f6CDQWr2+VBywrJf5zm_XiB4tg@mail.gmail.com>
 <CAHut+PsXP_61ZXuVOx5u9FZGK3oH4taaA59oOzgqyygZx8ezWw@mail.gmail.com>
 <CANhcyEU+aPu6iAH2cTA0cDtn3pd6c_njBONCt3FubYZoEEnm8Q@mail.gmail.com>
 <CAHut+Pv2P6dJ7hZj_fmzN+=xzjvpOpgkAJvDZg3TD2xpvmY1NQ@mail.gmail.com>
 <CANhcyEW2LK4diNeCG862DE40yQoV3VAgf59kXUq2TuR8fnw5vQ@mail.gmail.com>
 <CAHut+PuSHScrODVGCM7P53Mv1HE2N6ThzkH4+gQ1eFXVeD-OCA@mail.gmail.com>
 <CANhcyEUtYV-9ujtxLasnxN_peT+3LuZjcRx1xUECh1CCmANB8w@mail.gmail.com>
 <CAHut+PuviFA6C7qps=+kDYfe3P99as8NCjbR=SYxoi0o96ipoA@mail.gmail.com>
 <CANhcyEXkeg3sjkS3DS9yU1ckz4ozUBNZ+RmrWaRNSSVCR8RquA@mail.gmail.com>
 <CAHut+PsHavMy_KJ0MwR9J6q0BTTty54TxS-KBZc7X6tb4u7rfA@mail.gmail.com>
 <CANhcyEU=k4+0BqOu25N76g738XKUwfLGGdf8e+ssGiRKHC4RwQ@mail.gmail.com>
 <CAHut+Ptdi8txJbMc+5Sp2uB4QLGOp-rZdkBbXyhAkxim0iAhBQ@mail.gmail.com>
 <CANhcyEW+uJB_bvQLEaZCgoRTc1=i+QnrPPHxZ2=0SBSCyj9pkg@mail.gmail.com>
 <CAHut+Ptq0-tMYUOvG3yR34AvuEzR9vUH=muqV_=uEO3zCuA6rA@mail.gmail.com>
 <CANhcyEUEMWSkTfGc7Q3B+UiOzSiOmOGLgK-+C5DXwtCGOnDBhg@mail.gmail.com>
 <CAHut+PuAgSDr3kbSxPMYEyCeGiJ5hgaT1JUvuiYPRT=Q+--O-Q@mail.gmail.com>
 <CANhcyEU_uuiKMRrd_E_DeYsyCvwY_u995E-Do3d66J7tQnzdzw@mail.gmail.com>
 <CANhcyEWK9sj9UY4uaV36Q8qxUv=8Joch0o98RCN5U3xLjUxAag@mail.gmail.com>
 <CALDaNm32XQDR4qsOhPQeophVbZ8r+ShJSSssoVfdPcwG6joPHQ@mail.gmail.com>
 <CANhcyEVt2CBnG7MOktaPPV4rYapHR-VHe5=qoziTZh1L9SVc6w@mail.gmail.com>
 <CAHut+PvoOVo=_O-sG8wNaLRBPSD+6S=4PXOH2r=yKTxbpAbHkg@mail.gmail.com>
In-Reply-To: 
 <CAHut+PvoOVo=_O-sG8wNaLRBPSD+6S=4PXOH2r=yKTxbpAbHkg@mail.gmail.com>
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: Tue, 11 Nov 2025 15:54:06 +0530
Message-ID: 
 <CANhcyEUSQDKgu6-8rPZcy5Uzyaqt+0qTyTzp9KiVV=m-2RnVUQ@mail.gmail.com>
Subject: Re: Skipping schema changes in publication
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>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CANhcyEUSQDKgu6-8rPZcy5Uzyaqt%2B0qTyTzp9KiVV%3Dm-2RnVUQ%40mail.gmail.com>
Precedence: bulk

On Fri, 24 Oct 2025 at 06:41, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh,
>
> I had a look at patch v24-0001.
>
> FYI -- a rebase is needed
>
> [postgres@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