public inbox for [email protected]
help / color / mirror / Atom feedFrom: shveta malik <[email protected]>
To: Shlok Kyal <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Zhijie Hou (Fujitsu) <[email protected]>
Cc: YeXiu <[email protected]>
Cc: Ian Lawrence Barwick <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: shveta malik <[email protected]>
Subject: Re: Skipping schema changes in publication
Date: Mon, 22 Dec 2025 10:42:33 +0530
Message-ID: <CAJpy0uCf5tXvqyVS3GQzU9J5HdSLAxX6Lxt1UKY4HJ8qnimCAw@mail.gmail.com> (raw)
In-Reply-To: <CANhcyEVDTPVRWDm-BdJe0yaWFuz1ozTUFNHKvzX2YuiUHh=ArA@mail.gmail.com>
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+PtGu2j72yV_as_TVKfWr3ctd18svReGEx3xa=q5BtKyKA@mail.gmail.com>
<CANhcyEUh9ki36VTXyYf8UqUrfLX9ZhfP_f2LjpvvycgqWLQqqQ@mail.gmail.com>
<CAHut+Pv96u0VciuSx2B99jDHEvn7svVJynCmw-qYb=z4Kc2knA@mail.gmail.com>
<CANhcyEWGiWwGt1-v6d9bCAae9Np7cNPt+iRV9PXBZ0z=75XEVw@mail.gmail.com>
<CANhcyEWAQHtUfgNPA2m-+okEh7pXaK5irBm+yzyNVJXL2LUTXw@mail.gmail.com>
<CAHut+PuzcKXheZwgNvDJkwK5txd1kzNRxCmcJcbr=_9mGHjKtA@mail.gmail.com>
<CANhcyEXCKPCAdoqBLAhxt64Nwf+7T52dd8daE3qvhBNTvro13Q@mail.gmail.com>
<CAHut+Pu50yWjMR5Mswhi6uVKQmn3hO9o0ocRAgXyUUf4cnVTwg@mail.gmail.com>
<CANhcyEV7ewT+nfLM2owquxW-_6m8Ju+P93y=acoS=JCBHoT-MQ@mail.gmail.com>
<CAJpy0uDdQH7b=LRn_HevbmFACpvq9=c32Vb3tRvjSOnDsQd74w@mail.gmail.com>
<CAA4eK1KZ1Sb0soHp3HH2htwJ3=qka-eQjW35vOW3+4VeWw4VoQ@mail.gmail.com>
<CANhcyEXwLrQsec6g+1dqWTKyJQMQMh=getj28C+zLL14BjuumA@mail.gmail.com>
<CAJpy0uD35+YmDahVNUm+NZsjisV_k2hUDBkiFOJDwAJ1o1CT9A@mail.gmail.com>
<CANhcyEV_EVi5cgJ6WPvmeVAqjCS7Of+VAWuRHZtsVf8PQb_z7g@mail.gmail.com>
<CAHut+Pu5Ukuvd_5oK_mTEjSOEAYupbEXzRqXwSPoLe_sDkA_fg@mail.gmail.com>
<CANhcyEWg2WbEW_fFwk0D3J2KBrUF7th6VrE+gvESgkUKP9VpZg@mail.gmail.com>
<CAJpy0uBegaytfG=AS5VUb-6jAEDzC374-1icn-hP5AnRoMJ+Lg@mail.gmail.com>
<CAJpy0uD+orAxUWKq9Ogf5FEWtXcwkQXb_YZOvXqDc8b15nJe9A@mail.gmail.com>
<CANhcyEVDTPVRWDm-BdJe0yaWFuz1ozTUFNHKvzX2YuiUHh=ArA@mail.gmail.com>
On Thu, Dec 18, 2025 at 5:15 PM Shlok Kyal <[email protected]> wrote:
>
> On Thu, 18 Dec 2025 at 11:30, shveta malik <[email protected]> wrote:
> >
> > On Wed, Dec 17, 2025 at 11:24 AM shveta malik <[email protected]> wrote:
> > >
> > > On Tue, Dec 16, 2025 at 2:50 PM Shlok Kyal <[email protected]> wrote:
> > > >
> > > > I have also addressed the remaining comments and attached the latest patch.
> > > >
> > >
> > > Thanks. A few comments:
> > >
> > > 1)
> > > + if (!set_top && puballtables)
> > > + set_top = !list_member_oid(aexceptpubids, puboid);
> > >
> > > In GetTopMostAncestorInPublication(), we have made the above change
> > > which will now get ancestor from all-tables publication as well,
> > > provided table is not part of 'except' List. Earlier this function was
> > > only checking pg_subscription_rel and pg_publication_namespace which
> > > does not include all-tables publication. Won't it change the
> > > result-set for callers?
> > >
> It can change the result set of callers. I analysed more and saw that
> GetTopMostAncestorInPublication is called from 3 places.
> 1. pub_rf_contains_invalid_column: it is called when publication is
> not ALL TABLES. It will have no impact with the change.
> 2. pub_contains_invalid_column : it is called for all type of
> publication. it calls GetTopMostAncestorInPublication like:
> ```
> if (pubviaroot && relation->rd_rel->relispartition)
> {
> publish_as_relid = GetTopMostAncestorInPublication(pubid, ancestors,
> NULL, puballtables);
>
> if (!OidIsValid(publish_as_relid))
> publish_as_relid = relid;
> }
> ```
> In HEAD for ALL TABLES publication GetTopMostAncestorInPublication
> will always return InvalidOid. With this patch it can have some value.
> So there is a difference in behaviour.
>
> 3. get_rel_sync_entry
> in HEAD we had
> ```
> if (pub->alltables)
> {
> publish = true;
> if (pub->pubviaroot && am_partition)
> {
> List *ancestors = get_partition_ancestors(relid);
>
> pub_relid = llast_oid(ancestors);
> ancestor_level = list_length(ancestors);
> }
> }
> ```
> With patch this condition is not valid because we cannot set
> 'pub_relid = llast_oid(ancestors);' directly as the table can be
> excluded.
> So, the change in GetTopMostAncestorInPublication will even handle the
> case of "ALL TABLES" publication.
>
> Since we have a behaviour difference for the 2nd function, I have
> removed the changes for 'ALL TABLES' from
> GetTopMostAncestorInPublication and added it separately
> 'get_rel_sync_entry'. Thoughts?
I find the current implementation better, the previous one was
impacting the results of different paths.
Regarding:
+ if (list_member_oid(aexceptpubids, puboid))
+ {
+ list_free(aexceptpubids);
+ continue;
+ }
IMO, if puboid is part of apubids, that check is enough. This is
because aexceptpubids and apubids are mutually exclusive lists for a
particular 'ancestor'. But if we want to have it to avoid
schma-mapping check later, we should add a comment. How about:
This step isn’t strictly necessary, but we keep it so we can skip the
table if it appears in the EXCEPT list, avoiding an expensive
schema-mapping check later.
>
> > > 2)
> > > + * Publications declared with FOR ALL TABLES or FOR ALL SEQUENCES should use
> > > + * GetAllPublicationRelations() to obtain the complete set of tables covered by
> > > + * the publication.
> > > + */
> > > +List *
> > > +GetPublicationIncludedRelations(Oid pubid, PublicationPartOpt pub_partopt)
> > > +{
> > > + return GetPublicationRelationsInternal(pubid, pub_partopt, false);
> > > +}
> > >
> > > We can have an Assert here that pubid passed is not for ALL-Tables or
> > > ALL-sequences
> > >
> Added assert for all tables. I found during testing that this function
> can be called for ALL SEQUENCES in HEAD. So I have not added an
> assertion for it.
> I think it is a bug and shared the same in [1]. Will add assert for
> all sequences as well once the bug is fixed.
>
Okay.
> > > 3)
> > > GetAllPublicationRelations:
> > > * If the publication publishes partition changes via their respective root
> > > * partitioned tables, we must exclude partitions in favor of including the
> > > * root partitioned tables. This is not applicable to FOR ALL SEQUENCES
> > > * publication.
> > >
> > > + * The list does not include relations that are explicitly excluded via the
> > > + * EXCEPT TABLE clause of the publication specified by pubid.
> > >
> > > Suggestion:
> > > /*
> > > * If the publication publishes partition changes via their respective root
> > > * partitioned tables, we must exclude partitions in favor of including the
> > > * root partitioned tables. The list also excludes tables that are
> > > * explicitly excluded via the EXCEPT TABLE clause of the publication
> > > * identified by pubid. Neither of these rules applies to FOR ALL SEQUENCES
> > > * publications.
> > > */
> > >
> > > 4)
> > > GetAllPublicationRelations:
> > > + if (relkind == RELKIND_RELATION)
> > > + exceptlist = GetPublicationExcludedRelations(pubid, pubviaroot ?
> > > + PUBLICATION_PART_ALL :
> > > + PUBLICATION_PART_ROOT);
> > >
> > > Assert(!(relkind == RELKIND_SEQUENCE && pubviaroot));
> > >
> > > Generally we keep such parameters' sanity checks as the first step. We
> > > can add new code after Assert.
> > >
> > > 5)
> > > ObjectsInAllPublicationToOids() only has one caller which calls it
> > > under check: 'if (stmt->for_all_tables)'
> > >
> > > Thus IMO, we do not need a switch-case in
> > > ObjectsInAllPublicationToOids(). We can simply have a sanity check to
> > > see it is 'PUBLICATION_ALL_TABLES' and then do the needed operation
> > > for this pub-type.
> > >
> > > 6)
> > > CreatePublication():
> > > /*
> > > * If publication is for ALL TABLES and relations is not empty, it means
> > > * that there are some relations to be excluded from the publication.
> > > * Else, relations is the list of relations to be added to the
> > > * publication.
> > > */
> > >
> > > Shall we rephrase slightly to:
> > >
> > > /*
> > > * If the publication is for ALL TABLES and 'relations' is not empty,
> > > * it indicates that some relations should be excluded from the publication.
> > > * Add those excluded relations to the publication with 'prexcept' set to true.
> > > * Otherwise, 'relations' contains the list of relations to be explicitly
> > > * included in the publication.
> > > */
> > >
> > > 7)
> > > + /* Associate objects with the publication. */
> > > + if (stmt->for_all_tables)
> > > + {
> > > + /* Invalidate relcache so that publication info is rebuilt. */
> > > + CacheInvalidateRelcacheAll();
> > > + }
> > >
> > > I think this comment is misplaced. We shall have it at previous place, atop:
> > > if (stmt->for_all_tables)
> > > This is because here we are just trying to invalidate cache while at
> > > previous place we are trying to associate.
> > >
> >
> > Few more:
> >
> > 8)
> > get_rel_sync_entry()
> > + List *exceptTablePubids = NIL;
> >
> > At all other places, we are using exceptpubids, shall we use the same here?
> >
> > 9)
> > ObjectsInPublicationToOids()
> >
> > case PUBLICATIONOBJ_TABLE:
> > + case PUBLICATIONOBJ_EXCEPT_TABLE:
> > + pubobj->pubtable->except = (pubobj->pubobjtype ==
> > PUBLICATIONOBJ_EXCEPT_TABLE);
> > *rels = lappend(*rels, pubobj->pubtable);
> > break;
> >
> > It looks slightly odd that for pubobjtype case
> > 'PUBLICATIONOBJ_EXCEPT_TABLE', we have to check pubobjtype against
> > PUBLICATIONOBJ_EXCEPT_TABLE itself.
> >
> > Shall we make it:
> > case PUBLICATIONOBJ_EXCEPT_TABLE:
> > pubobj->pubtable->except = true;
> > /* fall through */
> > case PUBLICATIONOBJ_TABLE:
> > *rels = lappend(*rels, pubobj->pubtable);
> > break;
> >
> We should also make pubobj->pubtable->except = false for PUBLICATIONOBJ_TABLE?
yes, right.
> Updated the condition like:
> case PUBLICATIONOBJ_EXCEPT_TABLE:
> pubobj->pubtable->except = true;
> *rels = lappend(*rels, pubobj->pubtable);
> break;
> case PUBLICATIONOBJ_TABLE:
> pubobj->pubtable->except = false;
> *rels = lappend(*rels, pubobj->pubtable);
> break;
>
Looks good.
> > 10)
> > I want to understand the usage of DO_PUBLICATION_EXCEPT_REL. Can you
> > give a scenario where its usage in DOTypeNameCompare() will be hit?
> > Its all other usages too need some analysis and validation.
> >
> In the current patch we are not setting an objecttype to
> DO_PUBLICATION_EXCEPT_REL.
> We are storing the list of except tables in 'pubinfo[i].excepttbls'
> list in function getPublications and "pubinfo[i].dobj.objType =
> DO_PUBLICATION". So, I don't see any requirement of
> DO_PUBLICATION_EXCEPT_REL now. I have removed it.
>
Yes, that was my initial thought as well, that we might not need it.
But I’ll review it further and let you know.
> > 11)
> > + List *except_objects; /* List of publication object to be excluded */
> >
> > object --> objects
> > Currently since we exclude only tables, does it make sense to name it
> > as except_tables?
> >
> I have also addressed the remaining comments and attached the updated v33 patch.
> [1]: https://www.postgresql.org/message-id/CALDaNm0qoNtsX%2B9KPug6qb%3DuC-H2iPMYW%2BgL%3DHehx%2BNgOxga6w%...
>
Thanks, will review.
thanks
Shveta
view thread (377+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Skipping schema changes in publication
In-Reply-To: <CAJpy0uCf5tXvqyVS3GQzU9J5HdSLAxX6Lxt1UKY4HJ8qnimCAw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox