Thank Rafia for the review.
On Wed, Jan 29, 2020 at 3:55 PM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> On Thu, 23 Jan 2020 at 11:10, Amit Langote <amitlangote09@gmail.com> wrote:
> > v10 patches
> + cannot replicate from a regular table into a partitioned able or vice
> Here is a missing t from table.
Oops, fixed.
> + <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...?
Yes, it's upon the user to make sure that they have set up the
partitions correctly on the subscriber. I don't see how that's very
different from what needs to be done when tables are added to a
publication after-the-fact. Did I misunderstand you?
> + /* 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'
Hmm, this is simply following a convention that's used in most places
around the code, although I'm not really a fan of these "not a
<whatever>"-style messages to begin with. It's less ambiguous with a
"cannot perform <action> on <relkind>"-style message, which some
places already use.
In that view, I have changed the documentation too to say this:
+ Replication is only supported by tables, partitioned or not, although a
+ given table must either be partitioned on both servers or not partitioned
+ at all. Also, when replicating between partitioned tables, the actual
+ replication occurs between leaf partitions, so partitions on the two
+ servers must match one-to-one.
In retrospect, the confusion surrounding how we communicate the
various operations and properties that cannot be supported on a table
if partitioned, both in the error messages and the documentation,
could have been avoided if it wasn't based on relkind. I guess it's
too late now though. :(
> + * 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.
Yes, with multi-level partitioning.
> 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.
Actually, that is what happens, unless partitions were explicitly
added to the publication, in which case they will continue to be
published.
> -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().
I have done something similar in the updated patch, as I mentioned in
my earlier reply.
Please check the updated patches.
Thanks,
Amit
