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>
 <CAJpy0uAamUiZN5LkN9y7_-zT8=OZmW846T8dfgkojFwreV8XOw@mail.gmail.com>
 <CANhcyEU9jX5wM0R3jHs5-=UFaFehcXC8KMkKow_rphHVben_Nw@mail.gmail.com>
 <CAJpy0uDWRRgg9O8dmbBsS2Lu7SJMMOmsbfYNJG4R8saPVK-EQw@mail.gmail.com>
 <CAJpy0uAB+4nB5s2P0YdFAbNhOuB1UFFL5SyogDuGNedETA+d_w@mail.gmail.com>
 <CANhcyEU9ENjbfEMRmw3MAZsOP04ORXB0Se+t_7Qcqv61sLc0dA@mail.gmail.com>
 <CAJpy0uB=J9a=zQXKuznTYqRrCYy8rm1pHfWLA10XHp0UojYpdA@mail.gmail.com>
In-Reply-To: 
 <CAJpy0uB=J9a=zQXKuznTYqRrCYy8rm1pHfWLA10XHp0UojYpdA@mail.gmail.com>
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: Tue, 22 Jul 2025 01:05:40 +0530
Message-ID: 
 <CANhcyEXYrzFrGqzw+=qjuC0RFMDaoX+b1SZV4yCJNG6KWxodPQ@mail.gmail.com>
Subject: Re: Skipping schema changes in publication
To: shveta malik <shveta.malik@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
	"Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>,
 vignesh C <vignesh21@gmail.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"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CANhcyEXYrzFrGqzw%2B%3DqjuC0RFMDaoX%2Bb1SZV4yCJNG6KWxodPQ%40mail.gmail.com>
Precedence: bulk

On Mon, 21 Jul 2025 at 16:22, shveta malik <shveta.malik@gmail.com> wrote:
>
> On Sat, Jul 19, 2025 at 4:17=E2=80=AFPM Shlok Kyal <shlok.kyal.oss@gmail.=
com> wrote:
> >
> > On Mon, 30 Jun 2025 at 16:25, shveta malik <shveta.malik@gmail.com> wro=
te:
> > >
> > > Few more comments on 002:
> > >
> > > 5)
> > > +GetAllTablesPublicationRelations(Oid pubid, bool pubviaroot)
> > >  {
> > >
> > > + List    *exceptlist;
> > > +
> > > + exceptlist =3D GetPublicationRelations(pubid, PUBLICATION_PART_ALL)=
;
> > >
> > >
> > > a) Here, we are assuming that the list provided by
> > > GetPublicationRelations() will be except-tables list only, but there
> > > is no validation of that.
> > > b) We are using GetPublicationRelations() to get the relations which
> > > are excluded from the publication. The name of function and comments
> > > atop function are not in alignment with this usage.
> > >
> > > Suggestion:
> > > We can have a new GetPublicationExcludeRelations() function for the
> > > concerned usage. The existing logic of GetPublicationRelations() can
> > > be shifted to a new internal-logic function which will accept a
> > > 'except-flag' as well. Both GetPublicationRelations() and
> > > GetPublicationExcludeRelations() can call that new function by passin=
g
> > > 'except-flag' as false and true respectively. The new internal
> > > function will validate 'prexcept' against that except-flag passed and
> > > will return the results.
> > >
> > I have made the above change.
> >
> >
> > > 6)
> > > Before your patch002, GetTopMostAncestorInPublication() was checking
> > > pg_publication_rel and pg_publication_namespace to find out if the
> > > table in the ancestor-list is part of a given particular. Both
> > > pg_publication_rel and pg_publication_namespace did not have the entr=
y
> > > "for all tables" publications. That means
> > > GetTopMostAncestorInPublication() was originally not checking whether
> > > the given puboid is an "for all tables" publication to see if a rel
> > > belongs to that particular pub or not. I
> > >
> > > But now with the current change, we do check if pub is all-tables pub=
,
> > > if so, return relid and mark ancestor_level (provided table is not
> > > part of the except list).  IIUC, the result in 2 cases may be
> > > different. Is that the intention? Let me know if my understanding is
> > > wrong.
> > >
> > This is intentional, in function get_rel_sync_entry, we are setting
> > pub_relid to the topmost published ancestor. In HEAD we are directly
> > setting using:
> >             /*
> >              * If this is a FOR ALL TABLES publication, pick the partit=
ion
> >              * root and set the ancestor level accordingly.
> >              */
> >             if (pub->alltables)
> >             {
> >                 publish =3D true;
> >                 if (pub->pubviaroot && am_partition)
> >                 {
> >                     List       *ancestors =3D get_partition_ancestors(r=
elid);
> >
> >                     pub_relid =3D llast_oid(ancestors);
> >                     ancestor_level =3D list_length(ancestors);
> >                 }
> >             }
> > In HEAD, we can directly use 'llast_oid(ancestors)' to get the topmost
> > ancestor for case of FOR ALL TABLES.
> > But with this proposal. This change will no longer be valid as the
> > 'llast_oid(ancestors)' may be excluded in the publication. So, to
> > handle this change was made in GetTopMostAncestorInPublication.
> >
> >
> > Also, during testing with the partitioned table and
> > publish_via_partition_root the behaviour of the current patch is  as
> > below:
> > For example we have a partitioned table t1. It has partitions part1
> > and part2. Now consider the following cases:
> > 1. with publish_via_partition_root =3D true
> >      I. If we create publication on all tables with EXCEPT t1, no data
> > for t1, part1 or part2 is replicated.
> >      II.  If we create publication on all tables with EXCEPT part1,
> > data for all tables t1, part1 and part2 is replicated.
> > 2. with publish_via_partition_root =3D false
> >      I. If we create publication on all tables with EXCEPT t1, no data
> > for t1, part1 or part2 is replicated.
> >      II. If we create publication on all tables with EXCEPT part1,
> > data for part1 is not replicated
> >
> > Is this behaviour fine?
> > I checked for other databases such as MySQL, SQL Server. In that we do
> > not have such cases as either we replicate the whole partitioned table
> > or we not replicated at all. We do not have partition level control.
> > For Oracle, I found that we can include or exclude partitions using
> > 'PARTITIONEXCLUDE' [2], but did not find something similar to
> > publish_via_partition_root or where partitions are published as
> > separate tables.
> > What are your thoughts on the above behaviour?
> >
>
> Thank You for the details. I will review this behaviour soon and will
> let you know my comments. Meanwhile, please find a few comments on
> v16-0001:
>
> 1)
> we do LockSchemaList() everywhere before we call
> PublicationDropSchemas() to prevent concurrent schema deletion. Do we
> need that in reset flow as well?
Added

>
> 2)
> + /* Drop the schemas associated with the publication */
> + schemas =3D GetPublicationSchemas(pubid);
> + PublicationDropSchemas(pubid, schemas, true);
> +
> + /* Get all relations associated with the publication */
> + relids =3D GetPublicationRelations(pubid, PUBLICATION_PART_ROOT);
>
> We can rename schemas to schemaids similar to relids, as
> GetPublicationSchemas return oids.
>
Fixed

> 3)
> + /* Drop the relations associated with the publication */
> + PublicationDropTables(pubform->oid, rels, true);
>
> we can pass 'pubid' here instead of pubform->oid
>
Modified

> 4)
> Shall we modify the comments:
> 'Drop the relations associated with the publication' to 'Remove the
> associated relations from the publication'
> 'Drop the schemas associated with the publication'  to 'Remove the
> associated schemas from the publication'
>
> Similar changes can be done in test file's comments as well
> --Verify that tables associated with the publication are dropped after
> RESET
> --Verify that schemas associated with the publication are dropped after R=
ESET
>
Fixed

I have made the changes in the latest v17 patch [1].
[1]: https://www.postgresql.org/message-id/CANhcyEUtYV-9ujtxLasnxN_peT%2B3L=
uZjcRx1xUECh1CCmANB8w%40mail.gmail.com

Thanks,
Shlok Kyal