Re: pg_get_publication_tables() output duplicate relid

On Fri, Nov 19, 2021 at 7:19 AM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Thu, Nov 18, 2021 at 1:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > 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.
>

Hmm, I don't think we can assume that current or future users of
pg_publication_tables() will use it with DISTINCT unless we specify in
the specs/docs. However, I guess we might want to do it only for HEAD
as there is no direct field report for this and anyway we have some
workaround for this.

>  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?
>

Yeah, we can add tests for this and the other case.

>  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?
>

The latter one but I think either we should document this or change it
as we can't assume users will follow what subscriber-side code does.

> > > 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.
> >
>
> 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.
>

Okay, I see this could be a problem but I haven't tried to reproduce it.

> > 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.
>

Okay, so we have four known problems in the same area. The first has
been reported at the start of this thread, then the two as summarized
by Hou-San in his email [1] and the fourth one is the attach of
partitions after initial sync as you mentioned. It makes sense to have
some high-level idea on how to solve each of the problems before
pushing a fix for any one particular problem as one fix could have an
impact on other fixes. I'll think over it but please do let me know if
you have any ideas.


[1] -
https://www.postgresql.org/message-id/OS0PR01MB5716C756312959F293A822C794869%40OS0PR01MB5716.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.