RE: [HACKERS] logical decoding of two-phase transactions

On Friday, July 30, 2021 12:02 PM Peter Smith <smithpb2250(at)gmail(dot)com>wrote:
>
> Please find attached the latest patch set v100*
>
> v99-0002 --> v100-0001
>

Thanks for your patch. A few comments on the test file:

1. src/test/subscription/t/022_twophase_cascade.pl

1.1
I saw your test cases for "PREPARE / COMMIT PREPARED" and "PREPARE with a nested ROLLBACK TO SAVEPOINT", but didn't see cases for "PREPARE / ROLLBACK PREPARED". Is it needless or just missing?

1.2
+# check inserts are visible at subscriber(s).
+# All the streamed data (prior to the SAVEPOINT) should be rolled back.
+# (3, 'foobar') should be committed.

I think it should be (9999, 'foobar') here.

1.3
+$result = $node_B->safe_psql('postgres', "SELECT count(*) FROM test_tab where b = 'foobar';");
+is($result, qq(1), 'Rows committed are present on subscriber B');
+$result = $node_B->safe_psql('postgres', "SELECT count(*) FROM test_tab;");
+

It seems the test is not finished yet. We didn't check the value of 'result'. Besides, maybe we should also check node_C, right?

1.4
+$node_B->append_conf('postgresql.conf', qq(max_prepared_transactions = 10));
+$node_B->append_conf('postgresql.conf', qq(logical_decoding_work_mem = 64kB));

You see, the first line uses a TAB but the second line uses a space.
Also, we could use only one statement to append these two settings to run tests a bit faster. Thoughts?
Something like:

$node_B->append_conf(
'postgresql.conf', qq(
max_prepared_transactions = 10
logical_decoding_work_mem = 64kB
));

Regards
Tang