On Fri, Mar 24, 2023 at 7:19 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> Hi Wang-san. I looked at the v21-0001 patch.
>
> I don't have any new review comments -- only follow-ups for some of my
> previous v20 comments that were rejected.
>
> On Thu, Mar 23, 2023 at 8:11 PM wangw.f...@fujitsu.com
> <wangw.f...@fujitsu.com> wrote:
> >
> > On Thu, Mar 23, 2023 at 12:27 PM Peter Smith <smithpb2...@gmail.com> wrote:
> > > Here are some review comments for patch v20-0001.
> >
> ...
> > > ======
> > > src/test/subscription/t/013_partition.pl
> > >
> > > 5.
> > > I think maybe you could have another test scenario where you INSERT
> > > something into tab4_1_1 while only the publication for tab4_1 has
> > > publish_via_partition_root=true
> >
> > I'm not sure if this scenario is necessary.
> >
>
> Please see my reply for #7 below.
>
> > > ~~~
> > >
> > > 6.
> > > AFAICT the tab4 tests are only testing the initial sync, but are not
> > > testing normal replication. Maybe some more normal (post sync) INSERTS
> > > are needed to tab4, tab4_1, tab4_1_1 ?
> >
> > Since I think the scenario we fixed is sync and not replication, it doesn't
> > seem
> > like we should extend the test you mentioned.
> >
>
> Maybe you are right. I only thought it would be better to have testing
> which verifies that the sync phase and the normal replication phase
> are using the same rules.
>
Yeah, we could extend such tests if we want but I think it is not a
must as the patch didn't change this behavior.
> > > ======
> > > src/test/subscription/t/028_row_filter.pl
> > >
> > > 7.
> > > +# insert data into partitioned table.
> > > +$node_publisher->safe_psql('postgres',
> > > + "INSERT INTO tab_rowfilter_viaroot_part(a) VALUES(13), (17)");
> > > +
> > > $node_subscriber->safe_psql('postgres',
> > > "CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr
> > > application_name=$appname' PUBLICATION tap_pub_1, tap_pub_2,
> > > tap_pub_3, tap_pub_4a, tap_pub_4b, tap_pub_5a, tap_pub_5b,
> > > tap_pub_toast, tap_pub_inherits, tap_pub_viaroot_2, tap_pub_viaroot_1"
> > > );
> > > @@ -707,13 +711,17 @@ is($result, qq(t|1), 'check replicated rows to
> > > tab_rowfilter_toast');
> > > # the row filter for the top-level ancestor:
> > > #
> > > # tab_rowfilter_viaroot_part filter is: (a > 15)
> > > +# - INSERT (13) NO, 13 < 15
> > > # - INSERT (14) NO, 14 < 15
> > > # - INSERT (15) NO, 15 = 15
> > > # - INSERT (16) YES, 16 > 15
> > > +# - INSERT (17) YES, 17 > 15
> > > $result =
> > > $node_subscriber->safe_psql('postgres',
> > > - "SELECT a FROM tab_rowfilter_viaroot_part");
> > > -is($result, qq(16), 'check replicated rows to
> > > tab_rowfilter_viaroot_part');
> > > + "SELECT a FROM tab_rowfilter_viaroot_part ORDER BY 1");
> > > +is( $result, qq(16
> > > +17),
> > > + 'check replicated rows to tab_rowfilter_viaroot_part');
> > >
> > > ~
> > >
> > > I'm not 100% sure this is testing quite what you want to test. AFAICT
> > > the subscription is created like:
> > >
> > > "CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr
> > > application_name=$appname' PUBLICATION tap_pub_1, tap_pub_2,
> > > tap_pub_3, tap_pub_4a, tap_pub_4b, tap_pub_5a, tap_pub_5b,
> > > tap_pub_toast, tap_pub_inherits, tap_pub_viaroot_2, tap_pub_viaroot_1"
> >
> > I think this is the scenario we fixed : Simultaneously subscribing to two
> > publications that publish the parent and child respectively, then want to
> > use
> > the parent's identity and schema).
> >
>
> Yeah, but currently BOTH the tap_pub_viaroot_2, tap_pub_viaroot_1 are
> using "WITH (publish_via_partition_root)", so IMO the user would
> surely expect that only the root table would be published even when a
> subscription combines those publications. OTOH, I thought a subtle
> point of this patch is that now the same result will happen even if
> only ONE of the publications was using "WITH
> (publish_via_partition_root)". So it’s that scenario of “only ONE
> publication is using the option” that I thought ought to be explicitly
> tested.
>
The current change to existing tests is difficult to understand. I
suggest writing a separate test for row filter and then cover the
scenario you have suggested.
--
With Regards,
Amit Kapila.
