On Fri, Mar 24, 2023 at 9:49 AM Peter Smith <smithpb2...@gmail.com> wrote: > 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/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
