Hi Peter
,
Thanks for the review and sorry it took me a while to get back.
On Wed, Jan 8, 2020 at 7:54 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Looking through 0001, I think perhaps there is a better way to structure
> some of the API changes.
>
> Instead of passing the root_target_rel to CheckValidResultRel() and
> CheckCmdReplicaIdentity(), which we only need to check the publication
> actions of the root table, how about changing
> GetRelationPublicationActions() to automatically include the publication
> information of the root table. Then we have that information in the
> relcache once and don't need to check the base table and the partition
> root separately at each call site (of which there is only one right
> now). (Would that work correctly with relcache invalidation?)
>
> Similarly, couldn't GetRelationPublications() just automatically take
> partitioning into account? We don't need the separation between
> GetRelationPublications() and GetRelationAncestorPublications(). This
> would also avoid errors of omission, for example the
> GetRelationPublications() call in ATPrepChangePersistence() doesn't take
> GetRelationAncestorPublications() into account.
I have addressed these comments in the attached updated patch.
Other than that, the updated patch contains following significant changes:
* Changed pg_publication.c: GetPublicationRelations() so that any
published partitioned tables are expanded as needed
* Since the pg_publication_tables view is backed by
GetPublicationRelations(), that means subscriptioncmds.c:
fetch_table_list() no longer needs to craft a query to include
partitions when needed, because partitions are included at source.
That seems better, because it allows to limit the complexity
surrounding publication of partitioned tables to the publication side.
* Fixed the publication table DDL to spot more cases of tables being
added to a publication in a duplicative manner. For example,
partition being added to a publication which already contains its
ancestor and a partitioned tables being added to a publication
(implying all of its partitions are added) which already contains a
partition
Only attaching 0001. Will send the rest after polishing them a bit more.
Thanks,
Amit
