Re: Skipping schema changes in publication

On Tue, 22 Jul 2025 at 14:29, shveta malik <shveta.malik@gmail.com> wrote:
>
> On Sat, Jul 19, 2025 at 4:17 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Mon, 30 Jun 2025 at 16:25, shveta malik <shveta.malik@gmail.com> 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.gmail.com

Thanks,
Shlok Kyal