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.
>
>
> > 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?
>
Thank You for the details. I will review this behaviour soon and will
let you know my comments. Meanwhile, please find a few comments on
v16-0001:
1)
we do LockSchemaList() everywhere before we call
PublicationDropSchemas() to prevent concurrent schema deletion. Do we
need that in reset flow as well?
2)
+ /* Drop the schemas associated with the publication */
+ schemas = GetPublicationSchemas(pubid);
+ PublicationDropSchemas(pubid, schemas, true);
+
+ /* Get all relations associated with the publication */
+ relids = GetPublicationRelations(pubid, PUBLICATION_PART_ROOT);
We can rename schemas to schemaids similar to relids, as
GetPublicationSchemas return oids.
3)
+ /* Drop the relations associated with the publication */
+ PublicationDropTables(pubform->oid, rels, true);
we can pass 'pubid' here instead of pubform->oid
4)
Shall we modify the comments:
'Drop the relations associated with the publication' to 'Remove the
associated relations from the publication'
'Drop the schemas associated with the publication' to 'Remove the
associated schemas from the publication'
Similar changes can be done in test file's comments as well
--Verify that tables associated with the publication are dropped after
RESET
--Verify that schemas associated with the publication are dropped after RESET
thanks
Shveta
