Re: Data is copied twice when specifying both child and parent table in publication
Amit Kapila <amit.kapila16@gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
To: Jacob Champion <jchampion@timescale.com>
Cc: "wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>,
Andres Freund <andres@anarazel.de>, vignesh C <vignesh21@gmail.com>, Peter Smith <smithpb2250@gmail.com>, "Takamichi Osumi (Fujitsu)" <osumi.takamichi@fujitsu.com>, "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>,
"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>
Date: 2023-03-18T04:44:56Z
Lists: pgsql-hackers
On Sat, Mar 18, 2023 at 5:06 AM Jacob Champion <jchampion@timescale.com> wrote:
>
> On Thu, Mar 16, 2023 at 11:28 PM wangw.fnst@fujitsu.com
> <wangw.fnst@fujitsu.com> wrote:
> > Attach the new patch set.
>
> Hi,
>
> I ran into this problem while hacking on [1], so thank you for tackling
> it! I have no strong opinions on the implementation itself; I just want
> to register a concern that the tests have not kept up with the
> implementation complexity.
>
> For example, the corner case mentioned in 0003, with multiple
> publications having conflicting pubviaroot settings, isn't tested as far
> as I can see. (I checked manually, and it appears to work as intended.)
> And the related pub_lower_level test currently only covers the case
> where multiple publications have pubviaroot=true, so the following test
> comment is now misleading:
>
> > # for tab4, we publish changes through the "middle" partitioned table
> > $node_publisher->safe_psql('postgres',
> > "CREATE PUBLICATION pub_lower_level FOR TABLE tab4_1 WITH (publish_via_partition_root = true)"
> > );
>
> ...since the changes are now in fact published via the tab4 root after
> this patchset is applied.
>
> > I think the aim of joining it with pg_publication before is to exclude
> > non-existing publications. Otherwise, we would get an error because of the call
> > to function GetPublicationByName (with 'missing_ok = false') in function
> > pg_get_publication_tables.
>
> In the same vein, I don't think that case is covered anywhere.
>
We can have a test case to cover this scenario.
> There are a bunch of moving parts and hidden subtleties here, and I fell
> into a few traps when I was working on my patch, so it'd be nice to have
> additional coverage. I'm happy to contribute effort in that area if it's
> helpful.
>
I think it depends on what tests you have in mind. I suggest you can
propose a patch to cover tests for this are in a separate thread. We
can then evaluate those separately.
--
With Regards,
Amit Kapila.
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