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.
> 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.
II. If we create publication on all tables with EXCEPT part1,
data for all tables t1, part1 and part2 is replicated.
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.
II. If we create publication on all tables with EXCEPT part1,
data for part1 is not replicated
Is this behaviour fine?
I checked for other databases such as MySQL, SQL Server. In that we do
not have such cases as either we replicate the whole partitioned table
or we not replicated at all. We do not have partition level control.
For Oracle, I found that we can include or exclude partitions using
'PARTITIONEXCLUDE' [2], but did not find something similar to
publish_via_partition_root or where partitions are published as
separate tables.
What are your thoughts on the above behaviour?
I have addressed the comments and added the changes in the latest v16 patch [1].
[1]:https://www.postgresql.org/message-id/CANhcyEW2LK4diNeCG862DE40yQoV3VAgf59kXUq2TuR8fnw5vQ%40mail.gmail.com
[2]:
https://docs.oracle.com/en/middleware/goldengate/core/23/reference/partition-partitionexclude.html
Thanks,
Shlok Kyal
