On Fri, May 6, 2022 4:56 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are my review comments for v5-0002 (TAP tests)
>
> Your changes followed a similar pattern of refactoring so most of my
> comments below is repeated for all the files.
>
Thanks for your comments.
> ======
>
> 1. Commit message
>
> For the tap tests about streaming option in logical replication, test both
> 'on' and 'apply' option.
>
> SUGGESTION
> Change all TAP tests using the PUBLICATION "streaming" option, so they
> now test both 'on' and 'apply' values.
>
OK. But "streaming" is a subscription option, so I modified it to:
Change all TAP tests using the SUBSCRIPTION "streaming" option, so they
now test both 'on' and 'apply' values.
> ~~~
>
> 4. src/test/subscription/t/015_stream.pl
>
> +# Test streaming mode apply
> +$node_publisher->safe_psql('postgres', "DELETE FROM test_tab WHERE (a > 2)");
> $node_publisher->wait_for_catchup($appname);
>
> I think those 2 lines do not really belong after the "# Test streaming
> mode apply" comment. IIUC they are really just doing cleanup from the
> prior test part so I think they should
>
> a) be *above* this comment (and say "# cleanup the test data") or
> b) maybe it is best to put all the cleanup lines actually inside the
> 'test_streaming' function so that the last thing the function does is
> clean up after itself.
>
> option b seems tidier to me.
>
I also think option b seems better, so I put them inside test_streaming().
The rest of the comments are fixed as suggested.
Besides, I noticed that we didn't free the background worker after preparing a
transaction in the main patch, so made some small changes to fix it.
Attach the updated patches.
Regards,
Shi yu
