Re: adding partitioned tables to publications

On Mon, Dec 16, 2019 at 2:50 PM Amit Langote <[email protected]> wrote:
>
> Thanks for checking.
>
> On Thu, Dec 12, 2019 at 12:48 AM Peter Eisentraut
> <[email protected]> wrote:
> > On 2019-12-06 08:48, Amit Langote wrote:
> > > 0001:  Adding a partitioned table to a publication implicitly adds all
> > > its partitions.  The receiving side must have tables matching the
> > > published partitions, which is typically the case, because the same
> > > partition tree is defined on both nodes.
> >
> > This looks pretty good to me now.  But you need to make all the changed
> > queries version-aware so that you can still replicate from and to older
> > versions.  (For example, pg_partition_tree is not very old.)
>
> True, fixed that.
>
> > This part looks a bit fishy:
> >
> > +       /*
> > +        * If either table is partitioned, skip copying.  Individual
> > partitions
> > +        * will be copied instead.
> > +        */
> > +       if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
> > +               remote_relkind == RELKIND_PARTITIONED_TABLE)
> > +       {
> > +               logicalrep_rel_close(relmapentry, NoLock);
> > +               return;
> > +       }
> >
> > I don't think you want to filter out a partitioned table on the local
> > side, since (a) COPY can handle that, and (b) it's (as of this patch) an
> > error to have a partitioned table in the subscription table set.
>
> Yeah, (b) is true, so copy_table() should only ever see regular tables
> with only patch 0001 applied.
>
> > I'm not a fan of the new ValidateSubscriptionRel() function.  It's too
> > obscure, especially the return value.  Doesn't seem worth it.
>
> It went through many variants since I first introduced it, but yeah I
> agree we don't need it if only because of the weird interface.
>
> It occurred to me that, *as of 0001*, we should indeed disallow
> replicating from a regular table on publisher node into a partitioned
> table of the same name on subscriber node (as the earlier patches
> did), because 0001 doesn't implement tuple routing support that would
> be needed to apply such changes.
>
> Attached updated patches.
>
I am planning to review this patch.  Currently, it is not applying on
the head so can you rebase it?


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com