Re: adding partitioned tables to publications

Hi Amit,

Once again I went through this patch set and here are my few comments,

On Thu, 23 Jan 2020 at 11:10, Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Wed, Jan 22, 2020 at 2:38 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > 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
>
> On second thought, this seems like an overkill.  It might be OK after
> all for both a partitioned table and its partitions to be explicitly
> added to a publication without complaining of duplication. IOW, it's
> the user's call whether it makes sense to do that or not.
>
> > Only attaching 0001.
>
> Attached updated 0001 considering the above and the rest of the
> patches that add support for replicating partitioned tables using
> their own identity and schema.  I have reorganized the other patches
> as follows:
>
> 0002: refactoring of logical/worker.c without any functionality
> changes (contains much less churn than in earlier versions)
>
> 0003: support logical replication into partitioned tables on the
> subscription side (allows replicating from a non-partitioned table on
> publisher node into a partitioned table on subscriber node)
>
> 0004: support optionally replicating partitioned table changes (and
> changes directly made to partitions) using root partitioned table
> identity and schema

+     cannot replicate from a regular table into a partitioned able or vice
Here is a missing t from table.

+     <para>
+      When a partitioned table is added to a publication, all of its existing
+      and future partitions are also implicitly considered to be part of the
+      publication.  So, even operations that are performed directly on a
+      partition are also published via its ancestors' publications.

Now this is confusing, does it mean that when partitions are later
added to the table they will be replicated too, I think not, because
you need to first create them manually at the replication side, isn't
it...?

+ /* Must be a regular or partitioned table */
+ if (RelationGetForm(targetrel)->relkind != RELKIND_RELATION &&
+ RelationGetForm(targetrel)->relkind != RELKIND_PARTITIONED_TABLE)
  ereport(ERROR,
  (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
  errmsg("\"%s\" is not a table",

IMHO the error message and details should be modified here to
something along the lines of 'is neither a regular or partitioned
table'

+ * published via an ancestor and when a partitioned tables's partitions
tables's --> tables'

+ if (get_rel_relispartition(relid))
+ {
+ List    *ancestors = get_partition_ancestors(relid);

Now, this is just for my understanding, why the ancestors have to be a
list, I always assumed that a partition could only have one ancestor
-- the root table. Is there something more to it that I am totally
missing here or is it to cover the scenario of having partitions of
partitions.

Here I also want to clarify one thing, does it also happen like if a
partitioned table is dropped from a publication then all its
partitions are also implicitly dropped? As far as my understanding
goes that doesn't happen, so shouldn't there be some notice about it.

-GetPublicationRelations(Oid pubid)
+GetPublicationRelations(Oid pubid, bool include_partitions)

How about having an enum here with INCLUDE_PARTITIONS,
INCLUDE_PARTITIONED_REL, and SKIP_PARTITIONS to address the three
possibilities and avoiding reiterating through the list in
pg_get_publication_tables().

-- 
Regards,
Rafia Sabih