Re: pg_get_publication_tables() output duplicate relid

On Thu, Nov 18, 2021 at 1:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Nov 17, 2021 at 11:39 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > What IS problematic is what a subscriber sees in the
> > pg_publication_tables view and the problem occurs only in the initial
> > sync phase, where the partition is synced duplicatively because of
> > being found in the view along with the parent in this case, that is,
> > when publish_via_partiiton_root is true.  I was saying on the other
> > thread [1] that we should leave it up to the subscriber to decide what
> > to do when the view (duplicatively) returns both the parent and the
> > partition, citing the use case that a subscriber may want to replicate
> > the parent and the partition as independent tables.  Though I now tend
> > to agree with Amit K that that may be such a meaningful and all that
> > common use case, and the implementation on the subscriber side would
> > have to be unnecessarily complex.
> >
> > So maybe it makes sense to just do what has been proposed --
> > de-duplicate partitions out of the pg_publication_tables view,
>
> AFAICU, there are actually two problems related to
> pg_publication_tables view that are being discussed: (a) when
> 'publish_via_partition_root' is true then it returns both parent and
> child tables (provided both are part of publication), this can lead to
> duplicate data on the subscriber; the fix for this is being discussed
> in thread [1]. (b) when 'publish_via_partition_root' is false, then it
> returns duplicate entries for child tables (provided both are part of
> publication), this problem is being discussed in this thread.
>
> As per the proposed patches, it seems both need a separate fix, we can
> fix them together if we want but I am not sure. Is your understanding
> the same?

Actually, I'm seeing both (a) and (b) as more or less the same thing.
If publish_via_partition_root=true, we'd want to remove a partition
from the list if the root parent is also present.  If false, we'd want
to remove a partition from the list because it'd also be added by way
of expanding the root.

I know that (a) causes the partition-double-sync problem that could be
fixed by partition OID de-duplication as proposed in the other thread.
As a comment on that, it'd be nice to see a test case added to
src/test/subscription suite in that patch, because
partition-double-sync is the main problem statement I'd think.

Like Bharath said, I don't see (b) as much of a problem, because the
subscriber applies DISTINCT when querying pg_publication_tables()
anyway.  Other users, if any out there, may be doing the same by
following core subscription code's example.  That said, I am not
opposed to applying the patch being proposed here, though I guess we
don't have any src/test/subscription test case for this one?  As in,
do we know of any replication (initial/streaming) misbehavior caused
by the duplicate partition OIDs in this case or is the only problem
that pg_publication_tables output looks odd?

> > unless
> > we know of a bigger problem that requires us to hack the subscriber
> > side of things too.
> >
>
> There is yet another issue that might need subscriber side change. See
> the second issue summarized by Hou-San in the email[2]. I feel it is
> better to tackle that separately.
>
> >  Actually, I came to know of one such problem
> > while thinking about this today: when you ATTACH a partition to a
> > table that is present in a publish_via_partition_root=true
> > publication, it doesn't get copied via the initial sync even though
> > subsequent replication works just fine.  The reason for that is that
> > the subscriber only syncs the partitions that are known at the time
> > when the parent is first synced, that too via the parent (as SELECT
> > <columns..> FROM parent), and then marks the parent as sync-done.
> > Refreshing the subscription after ATTACHing doesn't help, because the
> > subscriber can't see any partitions to begin with.
> >
>
> Sorry, it is not clear to me how this can lead to a problem. Even if
> we just have the root table in the subscriber at the time of sync
> later shouldn't copy get the newly attached partition's data and
> replicate it to the required partition for
> "publish_via_partition_root=true" case?

Not sure if you thought otherwise but I am talking about attaching a
new partition into the partition tree on the *publication side*.

The problematic case is attaching the partition *after* the subscriber
has already marked the root parent as synced and/or ready for
replication.  Refreshing the subscription doesn't help it discover the
newly attached partition, because a publish_via_partition_root only
ever tells about the root parent, which would be already synced, so
the subscriber would think there's nothing to copy.

> Anyway, if this is a problem
> we need to figure the solution for this separately.

Sure, we might need to do that after all.  Though it might be a good
idea to be sure that we won't need to reconsider the fix we push for
the issue(s) being discussed here and elsewhere, because I suspect
that the solution to the problem I mentioned is likely to involve
tweaking pg_publication_tables view output.

--
Amit Langote
EDB: http://www.enterprisedb.com