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