RE: Time delayed LR (WAS Re: logical replication restrictions)

Dear Peter,

> 1.
> +-- fail - utilizing streaming = parallel with time-delayed
> replication is not supported
> +CREATE SUBSCRIPTION regress_testsub CONNECTION
> 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect =
> false, streaming = parallel, min_send_delay = 123);
>
> "utilizing" --> "specifying"

Fixed.

> 2.
> +-- success -- min_send_delay value without unit is take as milliseconds
> +CREATE SUBSCRIPTION regress_testsub CONNECTION
> 'dbname=regress_doesnotexit' PUBLICATION testpub WITH (connect =
> false, min_send_delay = 123);
> +\dRs+
>
> "without unit is take as" --> "without units is taken as"

Fixed.

> 3.
> +-- success -- min_send_delay value with unit is converted into ms and
> stored as an integer
> +ALTER SUBSCRIPTION regress_testsub SET (min_send_delay = '1 d');
> +\dRs+
>
>
> "with unit is converted into ms" --> "with units other than ms is
> converted to ms"

Fixed.

> 4. Missing tests?
>
> Why have the previous ALTER SUBSCRIPTION tests been removed? AFAIK,
> currently, there are no regression tests for error messages like:
>
> test_sub=# ALTER SUBSCRIPTION sub1 SET (min_send_delay = 123);
> ERROR: cannot set min_send_delay for subscription in parallel streaming mode

These tests were missed while changing the basic design.
Added.

> src/test/subscription/t/001_rep_changes.pl
>
> 5.
> +# This test is successful if and only if the LSN has been applied with at least
> +# the configured apply delay.
> +ok( time() - $publisher_insert_time >= $delay,
> + "subscriber applies WAL only after replication delay for
> non-streaming transaction"
> +);
>
> It's not strictly an "apply delay". Maybe this comment only needs to
> say like below:
>
> SUGGESTION
> # This test is successful only if at least the configured delay has elapsed.

Changed.

New patch is available on [1].

[1]: https://www.postgresql.org/message-id/TYAPR01MB5866C6BCA4D9386D9C486033F5A59%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED