Re: adding partitioned tables to publications

Thanks for the review.

On Tue, Apr 7, 2020 at 6:01 PM Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> On 2020-04-07 08:44, Amit Langote wrote:
> > I updated the patch to make the following changes:
> >
> > * Rewrote the tests to match in style with those committed yesterday
> > * Renamed all variables that had pubasroot in it to have pubviaroot
> > instead to match the publication parameter
> > * Updated pg_publication catalog documentation
>
> Thanks. I have some further questions:
>
> The change in nodeModifyTable.c to add CheckValidResultRel() is unclear.
> It doesn't seem to do anything, and it's not clear how it's related to
> this patch.

CheckValidResultRel() checks that replica identity is present for
replicating given update/delete, which I think, it's better to perform
on the root table itself, rather than some partition that would be
affected. The latter already occurs by way of CheckValidResultRel()
being called on partitions to be updated. I think we get a more
helpful message if the root parent is flagged instead of a partition.

update prt1 set b = b + 1 where a = 578;
ERROR: cannot update table "prt1" because it does not have a replica
identity and publishes updates
HINT: To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.

vs.

-- checking the partition
update prt1 set b = b + 1 where a = 578;
ERROR: cannot update table "prt1_p3" because it does not have a
replica identity and publishes updates
HINT: To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.

I am okay to get rid of the check on root table if flagging invidual
partitions seems good enough.

> The changes in GetRelationPublications() are confusing to me:
>
> + if (published_rels)
> + {
> + num = list_length(result);
> + for (i = 0; i < num; i++)
> + *published_rels = lappend_oid(*published_rels, relid);
> + }
>
> This adds relid to the output list "num" times, where num is the number
> of publications found. Shouldn't "i" be used in the loop somehow?
> Similarly later in the function.

published_rels contains an *OID* for each publication that will be in
result. Callers should iterate the two lists together and for each
publication found in result, it will know which relation it is
associated with using the OID found in published_rels being scanned in
parallel. If publishing through an ancestor's publication, we need to
know which ancestor, so the whole dance.

I have thought this to be a bit ugly before, but after having to
explain it, I think it's better to use some other approach for this.
I have updated the patch so that GetRelationPublications no longer
considers a relation's ancestors. That way, it doesn't have to
second-guess what other information will be needed by the caller.

I hope that's clearer, because all the logic is in one place and that
is get_rel_sync_entry().

> The descriptions of the new fields in RelationSyncEntry don't seem to
> match the code accurately, or at least it's confusing.
> replicate_as_relid is always filled in with an ancestor, even if
> pubviaroot is not set.

Given this confusion, I have changed how replicate_as_relid works so
that it's now always set -- if different from the relation's own OID,
the code for "publishing via root" kicks in in various places.

> I think the pubviaroot field is actually not necessary. We only need
> replicate_as_relid.

Looking through the code, I agree. I guess I only kept it around to
go with pubupdate, etc.

I guess it might also be a good idea to call it publish_as_relid
instead of replicate_as_relid for consistency.

> There is a markup typo in logical-replication.sgml:
>
> <xref linkend=="sql-createpublication"/>

Oops, fixed.

> In pg_dump, you missed updating a branch for an older version. See
> attached patch.
>
> Also attached a patch to rephrase the psql output a bit to make it not
> so long.

Thank you, merged.

Attached updated patch with above changes.

--
Thank you,

Amit Langote
EnterpriseDB: http://www.enterprisedb.com