On Mon, Mar 27, 2023 at 7:03 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
>
> 1.
> +# two publications, one publishing through ancestor and another one directly
> +# publsihing the partition, with different row filters
> +$node_publisher->safe_psql('postgres',
> + "CREATE PUBLICATION tap_pub_viaroot_sync_1 FOR TABLE
> tab_rowfilter_viaroot_part_sync WHERE (a > 15) WITH
> (publish_via_partition_root)"
> +);
> +$node_publisher->safe_psql('postgres',
> + "CREATE PUBLICATION tap_pub_viaroot_sync_2 FOR TABLE
> tab_rowfilter_viaroot_part_sync_1 WHERE (a < 15)"
> +);
> +
>
> 1a.
> Typo "publsihing"
>
> ~
>
> 1b.
> IMO these table and publication names could be better.
>
> I thought it was confusing to have the word "sync" in these table
> names and publication names. To the casual reader, it looks like these
> are synchronous replication tests but they are not.
>Hmm, sync here is for initial sync, so I don't think it is too much of a problem to understand if one is aware that these are logical replication related tests. > Similarly, I thought it was confusing that 2nd publication and table > have names with the word "viaroot" when the option > publish_via_partition_root is not even true. > I think the better names for tables could be "tab_rowfilter_parent_sync, tab_rowfilter_child_sync" and for publications "tap_pub_parent_sync_1, tap_pub_child_sync_1" > ~~~ > > 2. > > # The following commands are executed after CREATE SUBSCRIPTION, so these SQL > # commands are for testing normal logical replication behavior. > # > > ~ > > I think you should add a couple of INSERTS for the newly added table/s > also. IMO it is not only better for test completeness, but it causes > readers to question why there are INSERTS for every other table except > these ones. > The purpose of the test is to test the initial sync's interaction with 'publish_via_partition_root' option. So, adding Inserts after that for replication doesn't serve any purpose and it also consumes test cycles without any additional benefit. So, -1 for extending it further. -- With Regards, Amit Kapila.
