On Fri, Mar 24, 2023 at 9:49 AM Peter Smith <smithpb2250@gmail.com> wrote:
> On Thu, Mar 23, 2023 at 8:11 PM wangw.fnst@fujitsu.com
> <wangw.fnst@fujitsu.com> wrote:
> >
> > On Thu, Mar 23, 2023 at 12:27 PM Peter Smith <smithpb2250@gmail.com>
> wrote:
> > > Here are some review comments for patch v20-0001.
> >
> ...
> > > ======
> > > src/test/subscription/t/031_column_list.pl
> > >
> > > 8.
> > > - CREATE PUBLICATION pub_root_true FOR TABLE test_root (a) WITH
> > > (publish_via_partition_root = true);
> > > + CREATE PUBLICATION pub_root_true_1 FOR TABLE test_root (a) WITH
> > > (publish_via_partition_root = true);
> > > + CREATE PUBLICATION pub_root_true_2 FOR TABLE test_root_1 (a, b) WITH
> > > (publish_via_partition_root = true);
> > >
> > > -- initial data
> > > INSERT INTO test_root VALUES (1, 2, 3);
> > > INSERT INTO test_root VALUES (10, 20, 30);
> > > ));
> > >
> > > +# Subscribe to pub_root_true_1 and pub_root_true_2 at the same time,
> which
> > > +# means that the initial data will be synced once, and only the column list of
> > > +# the parent table (test_root) in the publication pub_root_true_1 will be
> used
> > > +# for both table sync and data replication.
> > > $node_subscriber->safe_psql(
> > > 'postgres', qq(
> > > - CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr'
> PUBLICATION
> > > pub_root_true;
> > > + CREATE
> > >
> > > ~
> > >
> > > (This is similar to the previous review comment #7 above)
> > >
> > > Won't it be a better test of the "At least one" code when only the
> > > publication of partition (test_root_1) is using "WITH
> > > (publish_via_partition_root = true)".
> > >
> > > e.g
> > > CREATE PUBLICATION pub_root_true_1 FOR TABLE test_root (a);
> > > CREATE PUBLICATION pub_root_true_2 FOR TABLE test_root_1 (a, b) WITH
> > > (publish_via_partition_root = true);
> >
> > I think specifying one or both is the same scenario here.
> > But it seemed clearer if only the "via_root" option is specified in the
> > publication that publishes the parent, so I changed this point in
> > "031_column_list.pl". Since the publications in "028_row_filter.pl" were
> > introduced by other commits, I didn't change it.
> >
>
> In hindsight, I think those publications should be renamed to
> something more appropriate. The name "pub_root_true_2" seems
> misleading now since the publish_via_partition_root = false
>
> e.g.1. pub_test_root, pub_test_root_1
> or
> e.g.2. pub_root_true, pub_root_1_false
> etc.
I prefer your first suggestion.
Changed.
Regards,
Wang Wei
