On Fri, Jul 30, 2021 at 6:25 PM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:
>
> On Friday, July 30, 2021 12:02 PM Peter Smith <smithpb2250@gmail.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
seecases for "PREPARE / ROLLBACK PREPARED". Is it needless or just missing?
>
Yes, that test used to exist but it was removed in response to a
previous review (see [1] comment #10, Amit said there were too many
tests).
> 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.
>
Good catch. Fixed in v101.
> 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?
>
Oops. Thanks for finding this! Fixed in v101 by adding the missing tests.
> 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
> ));
>
OK. In v101 I changed the config as you suggested for both the 022 and
023 TAP tests.
------
[1] https://www.postgresql.org/message-id/CAHut%2BPts_bWx_RrXu%2BYwbiJva33nTROoQQP5H4pVrF%2BNcCMkRA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia.