On Sat, Jul 19, 2025 at 4:17 PM Shlok Kyal <shlok.kyal....@gmail.com> wrote:
>
> On Mon, 30 Jun 2025 at 16:25, shveta malik <shveta.ma...@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?
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?
> 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?
> II. If we create publication on all tables with EXCEPT part1,
> data for part1 is not replicated
>
Agreed.
thanks
Shveta
