public inbox for [email protected]
help / color / mirror / Atom feedFrom: Shlok Kyal <[email protected]>
To: shveta malik <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Zhijie Hou (Fujitsu) <[email protected]>
Cc: vignesh C <[email protected]>
Cc: YeXiu <[email protected]>
Cc: Ian Lawrence Barwick <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Skipping schema changes in publication
Date: Sun, 3 Aug 2025 21:40:22 +0530
Message-ID: <CANhcyEVB3bHYBsi1fnVaAMm6skkLbu9AdDbzKJ+q3io7uVMQQQ@mail.gmail.com> (raw)
In-Reply-To: <CAJpy0uBE65YAkAQ+q75b3WZteAZszeRgsQifTsJTb0JDsNQTSw@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>
<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>
On Tue, 22 Jul 2025 at 14:29, shveta malik <[email protected]> wrote:
>
> On Sat, Jul 19, 2025 at 4:17 PM Shlok Kyal <[email protected]> wrote:
> >
> > On Mon, 30 Jun 2025 at 16:25, shveta malik <[email protected]> wrote:
> > >
> > > Few more comments on 002:
> > >
> > > 5)
> > > +GetAllTablesPublicationRelations(Oid pubid, bool pubviaroot)
> > > {
> > >
> > > + List *exceptlist;
> > > +
> > > + exceptlist = 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 passing
> > > '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_flag */
> 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 = 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 == pubrel->prexcept)
> result = 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 entry
> > > "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 partition
> > * root and set the ancestor level accordingly.
> > */
> > 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);
> > }
> > }
> > 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 = 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' = 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
> = 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 = 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/CANhcyEXkeg3sjkS3DS9yU1ckz4ozUBNZ%2BRmrWaRNSSVCR8RquA%40mail.g...
Thanks,
Shlok Kyal
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: <CANhcyEVB3bHYBsi1fnVaAMm6skkLbu9AdDbzKJ+q3io7uVMQQQ@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