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.
Thanks,
Amit