RE: Data is copied twice when specifying both child and parent table in publication

On Thu, Mar 23, 2023 at 12:27 PM Peter Smith <smithpb2250@gmail.com> wrote:
> Here are some review comments for patch v20-0001.

Thanks for your comments.

> ======
> src/backend/commands/subscriptioncmds.c
> 
> 3. fetch_table_list
> 
> + /* Get the list of tables from the publisher. */
> + if (server_version >= 160000)
> + {
> + StringInfoData pub_names;
> 
> - appendStringInfoString(&cmd, "FROM pg_catalog.pg_publication_tables t\n"
> -    " WHERE t.pubname IN (");
> - get_publications_str(publications, &cmd, true);
> - appendStringInfoChar(&cmd, ')');
> + initStringInfo(&pub_names);
> + get_publications_str(publications, &pub_names, true);
> +
> + /*
> + * From version 16, we allowed passing multiple publications to the
> + * function pg_get_publication_tables. This helped to filter out the
> + * partition table whose ancestor is also published in this
> + * publication array.
> + *
> + * Join pg_get_publication_tables with pg_publication to exclude
> + * non-existing publications.
> + */
> + appendStringInfo(&cmd, "SELECT DISTINCT N.nspname, C.relname,\n"
> + "              ( SELECT array_agg(a.attname ORDER BY a.attnum)\n"
> + "                FROM pg_attribute a\n"
> + "                WHERE a.attrelid = GPT.relid AND\n"
> + "                      a.attnum = ANY(GPT.attrs)\n"
> + "              ) AS attnames\n"
> + " FROM pg_class C\n"
> + "   JOIN pg_namespace N ON N.oid = C.relnamespace\n"
> + "   JOIN ( SELECT (pg_get_publication_tables(VARIADIC
> array_agg(pubname::text))).*\n"
> + "          FROM pg_publication\n"
> + "          WHERE pubname IN ( %s )) as GPT\n"
> + "       ON GPT.relid = C.oid\n",
> + pub_names.data);
> +
> + pfree(pub_names.data);
> + }
> + else
> + {
> + appendStringInfoString(&cmd, "SELECT DISTINCT t.schemaname, t.tablename
> \n");
> +
> + /* Get column lists for each relation if the publisher supports it */
> + if (check_columnlist)
> + appendStringInfoString(&cmd, ", t.attnames\n");
> +
> + appendStringInfoString(&cmd, "FROM pg_catalog.pg_publication_tables t\n"
> +    " WHERE t.pubname IN (");
> + get_publications_str(publications, &cmd, true);
> + appendStringInfoChar(&cmd, ')');
> + }
> 
> I noticed the SQL "if" part is using uppercase aliases, but the SQL in
> the "else" part is using lowercase aliases. I think it would be better
> to be consistent (pick one).

Unified them into lowercase aliases.

> ======
> src/test/subscription/t/013_partition.pl
> 
> 4.
> -# for tab4, we publish changes through the "middle" partitioned table
> +# If we subscribe only to pub_lower_level, changes for tab4 will be published
> +# through the "middle" partition table. However, since we will be subscribing
> +# to both pub_lower_level and pub_all (see subscription sub2 below), we will
> +# publish changes via the root table (tab4).
>  $node_publisher->safe_psql('postgres',
>   "CREATE PUBLICATION pub_lower_level FOR TABLE tab4_1 WITH
> (publish_via_partition_root = true)"
>  );
> 
> ~
> 
> This comment seemed a bit overkill IMO. I don't think you need to say
> much here except maybe:
> 
> # Note that subscription "sub2" will later subscribe simultaneously to
> both pub_lower_level (i.e. just table tab4_1) and  pub_all.

Changed.

> ~~~
> 
> 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.

> ~~~
> 
> 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.

> ======
> 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).

> Notice in this case  BOTH the partitioned table and the partition had
> been published using "WITH (publish_via_partition_root)". But, IIUC
> won't it be better to test when only the partition's publication was
> using that option?
> 
> For example, I think then it would be a better test of this "At least one" code:
> 
> /* At least one publication is using publish_via_partition_root. */
> if (pub_elem->pubviaroot)
>     viaroot = true;
>
> ======
> 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 simlar 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.

Attach the new patch set.

Regards,
Wang Wei