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

On Fri, Apr 29, 2022 at 3:22 PM shiy(dot)fnst(at)fujitsu(dot)com
<shiy(dot)fnst(at)fujitsu(dot)com> wrote:
...
> Thanks for your patch.
>
> The patch modified streaming option in logical replication, it can be set to
> 'on', 'off' and 'apply'. The new option 'apply' haven't been tested in the tap test.
> Attach a patch which modified the subscription tap test to cover both 'on' and
> 'apply' option. (The main patch is also attached to make cfbot happy.)
>

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.

======

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.

~~~

2. src/test/subscription/t/015_stream.pl

+sub test_streaming
+{

I think the function should have a comment to say that its purpose is
to encapsulate all the common (stream related) test steps so the same
code can be run both for the streaming=on and streaming=apply cases.

~~~

3. src/test/subscription/t/015_stream.pl

+
+# Test streaming mode on

+# Test streaming mode apply

These comments fell too small. IMO they should both be more prominent like:

################################
# Test using streaming mode 'on'
################################

###################################
# Test using streaming mode 'apply'
###################################

~~~

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.

~~~

5. src/test/subscription/t/016_stream_subxact.pl

sub test_streaming should be commented. (same as comment #2)

~~~

6. src/test/subscription/t/016_stream_subxact.pl

The comments for the different streaming nodes should be more
prominent. (same as comment #3)

~~~

7. src/test/subscription/t/016_stream_subxact.pl

+# Test streaming mode apply
+$node_publisher->safe_psql('postgres', "DELETE FROM test_tab WHERE (a > 2)");
$node_publisher->wait_for_catchup($appname);

These don't seem to belong here. They are clean up from the prior
test. (same as comment #4)

~~~

8. src/test/subscription/t/017_stream_ddl.pl

sub test_streaming should be commented. (same as comment #2)

~~~

9. src/test/subscription/t/017_stream_ddl.pl

The comments for the different streaming nodes should be more
prominent. (same as comment #3)

~~~

10. src/test/subscription/t/017_stream_ddl.pl

+# Test streaming mode apply
$node_publisher->safe_psql(
'postgres', q{
-BEGIN;
-INSERT INTO test_tab VALUES (2001, md5(2001::text), -2001, 2*2001);
-ALTER TABLE test_tab ADD COLUMN e INT;
-SAVEPOINT s1;
-INSERT INTO test_tab VALUES (2002, md5(2002::text), -2002, 2*2002, -3*2002);
-COMMIT;
+DELETE FROM test_tab WHERE (a > 2);
+ALTER TABLE test_tab DROP COLUMN c, DROP COLUMN d, DROP COLUMN e,
DROP COLUMN f;
});

$node_publisher->wait_for_catchup($appname);

These don't seem to belong here. They are clean up from the prior
test. (same as comment #4)

~~~

11. .../t/018_stream_subxact_abort.pl

sub test_streaming should be commented. (same as comment #2)

~~~

12. .../t/018_stream_subxact_abort.pl

The comments for the different streaming nodes should be more
prominent. (same as comment #3)

~~~

13. .../t/018_stream_subxact_abort.pl

+# Test streaming mode apply
+$node_publisher->safe_psql('postgres', "DELETE FROM test_tab WHERE (a > 2)");
$node_publisher->wait_for_catchup($appname);

These don't seem to belong here. They are clean up from the prior
test. (same as comment #4)

~~~

14. .../t/019_stream_subxact_ddl_abort.pl

sub test_streaming should be commented. (same as comment #2)

~~~

15. .../t/019_stream_subxact_ddl_abort.pl

The comments for the different streaming nodes should be more
prominent. (same as comment #3)

~~~

16. .../t/019_stream_subxact_ddl_abort.pl

+test_streaming($node_publisher, $node_subscriber, $appname);
+
+# Test streaming mode apply
$node_publisher->safe_psql(
'postgres', q{
-BEGIN;
-INSERT INTO test_tab SELECT i, md5(i::text) FROM generate_series(3,500) s(i);
-ALTER TABLE test_tab ADD COLUMN c INT;
-SAVEPOINT s1;
-INSERT INTO test_tab SELECT i, md5(i::text), -i FROM
generate_series(501,1000) s(i);
-ALTER TABLE test_tab ADD COLUMN d INT;
-SAVEPOINT s2;
-INSERT INTO test_tab SELECT i, md5(i::text), -i, 2*i FROM
generate_series(1001,1500) s(i);
-ALTER TABLE test_tab ADD COLUMN e INT;
-SAVEPOINT s3;
-INSERT INTO test_tab SELECT i, md5(i::text), -i, 2*i, -3*i FROM
generate_series(1501,2000) s(i);
+DELETE FROM test_tab WHERE (a > 2);
ALTER TABLE test_tab DROP COLUMN c;
-ROLLBACK TO s1;
-INSERT INTO test_tab SELECT i, md5(i::text), i FROM
generate_series(501,1000) s(i);
-COMMIT;
});
-
$node_publisher->wait_for_catchup($appname);

These don't seem to belong here. They are clean up from the prior
test. (same as comment #4)

~~~

17. .../subscription/t/022_twophase_cascade.

+# ---------------------
+# 2PC + STREAMING TESTS
+# ---------------------
+sub test_streaming
+{

I think maybe that 2PC comment should not have been moved. IMO it
belongs in the main test body...

~~~

18. .../subscription/t/022_twophase_cascade.

sub test_streaming should be commented. (same as comment #2)

~~~

19. .../subscription/t/022_twophase_cascade.

+sub test_streaming
+{
+ my ($node_A, $node_B, $node_C, $appname_B, $appname_C, $streaming) = @_;

If you called that '$streaming' param something more like
'$streaming_mode' it would read better I think.

~~~

20. .../subscription/t/023_twophase_stream.pl

sub test_streaming should be commented. (same as comment #2)

~~~

21. .../subscription/t/023_twophase_stream.pl

The comments for the different streaming nodes should be more
prominent. (same as comment #3)

~~~

22. .../subscription/t/023_twophase_stream.pl

+# Test streaming mode apply
$node_publisher->safe_psql('postgres', "DELETE FROM test_tab WHERE a > 2;");
-
-# Then insert, update and delete enough rows to exceed the 64kB limit.
-$node_publisher->safe_psql('postgres', q{
- BEGIN;
- INSERT INTO test_tab SELECT i, md5(i::text) FROM generate_series(3,
5000) s(i);
- UPDATE test_tab SET b = md5(b) WHERE mod(a,2) = 0;
- DELETE FROM test_tab WHERE mod(a,3) = 0;
- PREPARE TRANSACTION 'test_prepared_tab';});
-
-$node_publisher->wait_for_catchup($appname);
-
-# check that transaction is in prepared state on subscriber
-$result = $node_subscriber->safe_psql('postgres', "SELECT count(*)
FROM pg_prepared_xacts;");
-is($result, qq(1), 'transaction is prepared on subscriber');
-
-# 2PC transaction gets aborted
-$node_publisher->safe_psql('postgres', "ROLLBACK PREPARED
'test_prepared_tab';");
-
$node_publisher->wait_for_catchup($appname);

These don't seem to belong here. They are clean up from the prior
test. (same as comment #4)

------
Kind Regards,
Peter Smith.
Fujitsu Australia