RE: Data is copied twice when specifying both child and parent table in publication
Wei Wang (Fujitsu) <wangw.fnst@fujitsu.com>
From: "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>
To: Amit Kapila <amit.kapila16@gmail.com>, "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>
Cc: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, Amit Langote <amitlangote09@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>, Dilip Kumar <dilipbalaut@gmail.com>, Greg Nancarrow <gregn4422@gmail.com>, vignesh C <vignesh21@gmail.com>
Date: 2022-04-24T06:16:04Z
Lists: pgsql-hackers
Attachments
- HEAD_v1-0001-Fix-data-replicated-twice-when-specifying-PUBLISH.patch (application/octet-stream) patch v1-0001
- REL14_v3-0001-Fix-data-replicated-twice-when-specifying-PUBLISH.patch (application/octet-stream) patch v3-0001
On Thur, Apr 21, 2022 at 5:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
Thanks for your comments.
> On Tue, Apr 19, 2022 at 2:23 PM shiy.fnst@fujitsu.com <shiy.fnst@fujitsu.com>
> wrote:
> >
> > On Tue, Apr 19, 2022 3:05 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> > >
> > > One suggestion is that can we simplify the code by moving the logic
> > > of checking the ancestor into the SQL ?. For example, we could
> > > filter the outpout of pg_publication_tables by adding A WHERE clause
> > > which checks whether the table is a partition and if its ancestor is
> > > also in the output. I think we can also filter the needless partition in this
> approach.
> > >
> >
> > I agreed with you and I tried to fix this problem in a simpler way.
> > What we want is to exclude the partitioned table whose ancestor is
> > also need to be replicated, so how about implementing that by using
> > the following SQL when getting the table list from publisher?
> >
> > SELECT DISTINCT ns.nspname, c.relname
> > FROM pg_catalog.pg_publication_tables t JOIN pg_catalog.pg_namespace
> > ns ON ns.nspname = t.schemaname JOIN pg_catalog.pg_class c ON
> > c.relname = t.tablename AND c.relnamespace = ns.oid WHERE t.pubname IN
> > ('p0','p2') AND (c.relispartition IS FALSE OR NOT EXISTS (SELECT 1
> > FROM pg_partition_ancestors(c.oid) WHERE relid IN ( SELECT DISTINCT
> > (schemaname||'.'||tablename)::regclass::oid
> > FROM pg_catalog.pg_publication_tables t WHERE t.pubname IN ('p0','p2')
> > ) AND relid != c.oid));
> >
> > Please find the attached patch which used this approach, I also merged
> > the test in Wang's patch into it.
> >
>
> I think this will work but do we need "... relid != c.oid" at the end of the query? If
> so, why? Please use an alias for pg_partition_ancestors to make the statement
> understandable.
I think we need this (relid != c.oid). Because when we use function
pg_partition_ancestors(c.oid), its return value not only has ancestors, but
also the input table. That is to say, when we use the table (c.oid) of the
outer query to filter in the sub-query, the table of the outer query will also
appear in the result of the sub-query.
So, I think we need this condition to prevent filtering out itself.
> Now, this solution will work but I find this query a bit complex and will add some
> overhead as we are calling pg_publication_tables multiple times. So, I was
> wondering if we can have a new function pg_get_publication_tables which
> takes multiple publications as input and return the list of qualified tables? I think
> for back branches we need something on the lines of what you have proposed
> but for HEAD we can have a better solution.
Yes, it sounds reasonable to me. Now, to fix this bug:
In the patch for HEAD, add a new function pg_get_publications_tables to get
tables info from a publications array.
In the patch for back-branch (now just share the patch for REL14), modify the
SQL to get tables info.
> IIRC, the column list and row filter also have some issues exactly due to this
> reason, so, I would like those cases to be also mentioned here and probably
> include the tests for them in the patch for HEAD.
Improve the test case about the column list and row filter to cover this bug.
Attach the new patches.[suggestions by Amit-San]
The patch for HEAD:
1. Add a new function to get tables info by a publications array.
The patch for REL14:
1. Use an alias to make the statement understandable. BTW, I adjusted the alignment.
2. Improve the test cast about the column list and row filter to cover this bug.
Regards,
Wang wei
Commits
-
Avoid syncing data twice for the 'publish_via_partition_root' option.
- 062a84442424 16.0 landed
-
Fix partition table's REPLICA IDENTITY checking on the subscriber.
- 52d5ea9adb01 14.5 cited
-
Prohibit combining publications with different column lists.
- fd0b9dcebda7 15.0 cited
-
Fix double publish of child table's data.
- 3f06c00cf6dc 13.6 landed
- 614b77d65a38 14.2 landed
- 5e97905a2c76 15.0 landed
-
Support adding partitioned tables to publication
- 17b9e7f9fe23 13.0 cited