On Thu, Jun 23, 2022 at 9:41 AM Peter Smith <smithpb2250@gmail.com> wrote:
> Here are some review comments for v12-0002
Thanks for your comments.
> 3. .../subscription/t/022_twophase_cascade.pl
>
> For every test file in this patch the new function is passed $is_apply
> = 0/1 to indicate to use 'on' or 'apply' parameter value. But in this
> test file the parameter is passed as $streaming_mode = 'on'/'apply'.
>
> I was wondering if (for the sake of consistency) it might be better to
> use the same parameter kind for all of the test files. Actually, I
> don't care if you choose to do nothing and leave this as-is; I am just
> posting this review comment in case it was not a deliberate decision
> to implement them differently.
>
> e.g.
> + my ($node_publisher, $node_subscriber, $appname, $is_apply) = @_;
>
> versus
> + my ($node_A, $node_B, $node_C, $appname_B, $appname_C,
> $streaming_mode) =
> + @_;
This is because in 022_twophase_cascade.pl, altering subscription streaming
mode is inside test_streaming(), it would be more convenient to pass which
streaming mode we use (on or apply), we can directly use that in alter
subscription command.
In other files, we need to get the option because we only check the log in
apply mode, so I think it is sufficient to pass 'is_apply' (whose value is 0 or
1).
Because of these differences, I did not change it.
The rest of the comments are improved as suggested.
The new patches were attached in [1].
[1] -
https://www.postgresql.org/message-id/OS3PR01MB62758DBE8FA12BA72A43AC819EB89%40OS3PR01MB6275.jpnprd01.prod.outlook.com
Regards,
Wang wei