On Mon, Dec 16, 2019 at 2:50 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Thanks for checking.
>
> On Thu, Dec 12, 2019 at 12:48 AM Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> 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
