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

On Thursday, January 19, 2023 10:49 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> On Wed, Jan 18, 2023 at 6:06 PM Peter Smith <smithpb2250(at)gmail(dot)com>
> wrote:
> >
> > Here are my review comments for the latest patch v16-0001. (excluding
> > the test code)
> >
>
> And here are some review comments for the v16-0001 test code.
Hi, thanks for your review !

> ======
>
> src/test/regress/sql/subscription.sql
>
> 1. General
> For all comments
>
> "time delayed replication" -> "time-delayed replication" maybe is better?
Fixed.

> ~~~
>
> 2.
> -- fail - utilizing streaming = parallel with time delayed replication is not
> supported.
>
> For readability please put a blank line before this test.
Fixed.

> ~~~
>
> 3.
> -- success -- value without unit is taken as milliseconds
>
> "value" -> "min_apply_delay value"
Fixed.

> ~~~
>
> 4.
> -- success -- interval is converted into ms and stored as integer
>
> "interval" -> "min_apply_delay interval"
>
> "integer" -> "an integer"
Both are fixed.

> ~~~
>
> 5.
> You could also add another test where min_apply_delay is 0
>
> Then the following combination can be confirmed OK -- success create
> subscription with (streaming=parallel, min_apply_delay=0)
This combination is added with the modification for #6.

> ~~
>
> 6.
> -- fail - alter subscription with min_apply_delay should fail when streaming =
> parallel is set.
> CREATE SUBSCRIPTION regress_testsub CONNECTION
> 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false,
> streaming = parallel);
>
> There is another way to do this test without creating a brand-new subscription.
> You could just alter the existing subscription like:
> ALTER ... SET (min_apply_delay = 0)
> then ALTER ... SET (parallel = streaming) then ALTER ... SET (min_apply_delay
> = 123)
Fixed.

> ======
>
> src/test/subscription/t/032_apply_delay.pl
>
> 7. sub check_apply_delay_log
>
> my ($node_subscriber, $message, $expected) = @_;
>
> Why pass in the message text? I is always the same so can be hardwired in this
> function, right?
Fixed.

> ~~~
>
> 8.
> # Get the delay time in the server log
>
> "int the server log" -> "from the server log" (?)
Fixed.

> ~~~
>
> 9.
> qr/$message: (\d+) ms/
> or die "could not get delayed time";
> my $logged_delay = $1;
>
> # Is it larger than expected?
> cmp_ok($logged_delay, '>', $expected,
> "The wait time of the apply worker is long enough expectedly"
> );
>
> 9a.
> "could not get delayed time" -> "could not get the apply worker wait time"
>
> 9b.
> "The wait time of the apply worker is long enough expectedly" -> "The apply
> worker wait time has expected duration"
Both are fixed.

> ~~~
>
> 10.
> sub check_apply_delay_time
>
>
> Maybe a brief explanatory comment for this function is needed to explain the
> unreplicated column c.
Added.

> ~~~
>
> 11.
> $node_subscriber->safe_psql('postgres',
> "CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr
> application_name=$appname' PUBLICATION tap_pub WITH (streaming = on,
> min_apply_delay = '3s')"
>
>
> I think there should be a comment here highlighting that you are setting up a
> subscriber time delay of 3 seconds, and then later you can better describe the
> parameters for the checking functions...
Added a comment for CREATE SUBSCRIPTION command.

> e.g. (add this comment)
> # verifies that the subscriber lags the publisher by at least 3 seconds
> check_apply_delay_time($node_publisher, $node_subscriber, '5', '3');
>
> e.g.
> # verifies that the subscriber lags the publisher by at least 3 seconds
> check_apply_delay_time($node_publisher, $node_subscriber, '8', '3');
Added.

> ~~~
>
> 12.
> # Test whether ALTER SUBSCRIPTION changes the delayed time of the apply
> worker # (1 day 1 minute).
> $node_subscriber->safe_psql('postgres',
> "ALTER SUBSCRIPTION tap_sub SET (min_apply_delay = 86460000)"
> );
>
> Update the comment with another note.
> # Note - The extra 1 min is to account for any decoding/network overhead.
Okay, added the comment. In general, TAP tests
fail if we wait for more than 3 minutes. Then,
we should think setting the maximum consumed time
more than 3 minutes is safe. For example, if
(which should not happen usually, but)
we consumed more than 1 minutes between this ALTER SUBSCRIPTION SET
and below check_apply_delay_log() then, the test will fail.

So made the extra time bigger.
> ~~~
>
> 13.
> # Make sure we have long enough min_apply_delay after the ALTER command
> check_apply_delay_log($node_subscriber, "logical replication apply delay",
> "80000000");
>
> IMO the expectation of 1 day (86460000 ms) wait time might be a better number
> for your "expected" value.
>
> So update the comment/call like this:
>
> # Make sure the apply worker knows to wait for more than 1 day (86400000 ms)
> check_apply_delay_log($node_subscriber, "logical replication apply delay",
> "86400000");
Updated the comment and the function call.

Kindly have a look at the updated patch v17.

Best Regards,
Takamichi Osumi