RE: Perform streaming logical transactions by background workers and parallel apply

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