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

  1. Avoid syncing data twice for the 'publish_via_partition_root' option.

  2. Fix partition table's REPLICA IDENTITY checking on the subscriber.

  3. Prohibit combining publications with different column lists.

  4. Fix double publish of child table's data.

  5. Support adding partitioned tables to publication