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>
 <CAJpy0uBE65YAkAQ+q75b3WZteAZszeRgsQifTsJTb0JDsNQTSw@mail.gmail.com>
In-Reply-To: 
 <CAJpy0uBE65YAkAQ+q75b3WZteAZszeRgsQifTsJTb0JDsNQTSw@mail.gmail.com>
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: Sun, 3 Aug 2025 21:40:22 +0530
Message-ID: 
 <CANhcyEVB3bHYBsi1fnVaAMm6skkLbu9AdDbzKJ+q3io7uVMQQQ@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/CANhcyEVB3bHYBsi1fnVaAMm6skkLbu9AdDbzKJ%2Bq3io7uVMQQQ%40mail.gmail.com>
Precedence: bulk

On Tue, 22 Jul 2025 at 14:29, 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.
>
> Thank You for the changes.
>
> 1)
> But on rethinking, shall we make GetPublicationRelations() similar to :
>
> /* Gets list of publication oids for a relation that matches the except_f=
lag */
> GetRelationPublications(Oid relid, bool except_flag)
>
> i.e. we can have a single function GetPublicationRelations() taking
> except_flag and comment can say: 'Gets list of relation oids for a
> publication that matches the except_flag.'
>
> We can get rid of GetPubIncludedOrExcludedRels() and
> GetPublicationExcludeRelations().
>
> Thoughts?
>
This seems reasonable to me. I have made the changes for the same.

>
> 2)
> we can rename except_table to except_flag to be consistent with
> GetRelationPublications()
>
> 3)
> + if ((except_table && pubrel->prexcept) || !except_table)
> + result =3D GetPubPartitionOptionRelations(result, pub_partopt,
> + pubrel->prrelid);
>
> 3a)
> In the case of '!except_table', we are not matching it with
> 'pubrel->prexcept', is that intentional?
>
> 3 b)
> Shall we simplify this similar to the changes in GetRelationPublications(=
) i.e.
> if (except_table/flag =3D=3D pubrel->prexcept)
>    result =3D GetPubPartitionOptionRelations(...)
>
>
> >
> > > 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.
>
> Okay. Agreed.
>
> >      II.  If we create publication on all tables with EXCEPT part1,
> > data for all tables t1, part1 and part2 is replicated.
>
> Okay. Is this because part1 changes are replicated through t1 and
> since t1 changes are not restricted, part1 changes will also not be
> restricted? In other words, part1 was never published directly in the
> first place and thus 'EXCEPT part1' has no meaning when
> 'publish_via_partition_root' =3D true? IMO, it is in alignment with the
> 'publish_via_partition_root' definition but it might not be that
> intuitive for users. So shall we emit a WARNING:
>
> WARNING: Partition "part1" is excluded, but publish_via_partition_root
> =3D true, so this will have no effect.
> Thoughts?
Your understanding is correct. I have added a WARNING for this case

>
> > 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.
>
> I think we shall still publish partitions here. Since
> publish_via_partition_root is false, part1 and part2 are published
> individually and thus shall we allow publishing of part1 and part 2
> here? Thoughts?
I made a mistake in explaining this point. Yes your point is correct.
Changes for partitions part1 and part2 will be replicated.
I have documented the behaviour in the docs.

>
> >      II. If we create publication on all tables with EXCEPT part1,
> > data for part1 is not replicated
> >
>
> Agreed.
>

I have addressed the comments and have attached the updated patch in [1].
[1]: https://www.postgresql.org/message-id/CANhcyEXkeg3sjkS3DS9yU1ckz4ozUBN=
Z%2BRmrWaRNSSVCR8RquA%40mail.gmail.com

Thanks,
Shlok Kyal