RE: Time delayed LR (WAS Re: logical replication restrictions) - Mailing list pgsql-hackers

Dear Peter,

Thank you for reviewing! PSA new version.

> ======
> doc/src/sgml/glossary.sgml
> 
> 1.
> +     <para>
> +      Replication setup that applies time-delayed copy of the data.
> +    </para>
> 
> That sentence seemed a bit strange to me.
> 
> SUGGESTION
> Replication setup that delays the application of changes by a
> specified minimum time-delay period.

Fixed.

> ======
> 
> src/backend/replication/logical/worker.c
> 
> 2. maybe_apply_delay
> 
> + if (wal_receiver_status_interval > 0 &&
> + diffms > wal_receiver_status_interval * 1000L)
> + {
> + WaitLatch(MyLatch,
> +   WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
> +   wal_receiver_status_interval * 1000L,
> +   WAIT_EVENT_RECOVERY_APPLY_DELAY);
> + send_feedback(last_received, true, false, true);
> + }
> 
> I felt that introducing another variable like:
> 
> long statusinterval_ms = wal_receiver_status_interval * 1000L;
> 
> would help here by doing 2 things:
> 1) The condition would be easier to read because the ms units would be the same
> 2) Won't need * 1000L repeated in two places.
> 
> Only, do take care to assign this variable in the right place in this
> loop in case the configuration is changed.

Fixed. Calculations are done on two lines - first one is the entrance of the loop,
and second one is the after SIGHUP is detected.

> ======
> src/test/subscription/t/001_rep_changes.pl
> 
> 3.
> +# Test time-delayed logical replication
> +#
> +# If the subscription sets min_apply_delay parameter, the logical replication
> +# worker will delay the transaction apply for min_apply_delay milliseconds. We
> +# look the time duration between tuples are inserted on publisher and then
> +# changes are replicated on subscriber.
> 
> This comment and the other one appearing later in this test are both
> explaining the same test strategy. I think both comments should be
> combined into one big one up-front, like this:
> 
> SUGGESTION
> If the subscription sets min_apply_delay parameter, the logical
> replication worker will delay the transaction apply for
> min_apply_delay milliseconds. We verify this by looking at the time
> difference between a) when tuples are inserted on the publisher, and
> b) when those changes are replicated on the subscriber. Even on slow
> machines, this strategy will give predictable behavior.

Changed.

> 4.
> +my $delay = 3;
> +
> +# Set min_apply_delay parameter to 3 seconds
> +$node_subscriber->safe_psql('postgres',
> + "ALTER SUBSCRIPTION tap_sub_renamed SET (min_apply_delay =
> '${delay}s')");
> 
> IMO that "my $delay = 3;" assignment should be *after* the comment:
> 
> e.g.
> +
> +# Set min_apply_delay parameter to 3 seconds
> +my $delay = 3;
> +$node_subscriber->safe_psql('postgres',
> + "ALTER SUBSCRIPTION tap_sub_renamed SET (min_apply_delay =
> '${delay}s')");

Right, changed.

> 5.
> +# Make new content on publisher and check its presence in subscriber
> depending
> +# on the delay applied above. Before doing the insertion, get the
> +# current timestamp that will be used as a comparison base. Even on slow
> +# machines, this allows to have a predictable behavior when comparing the
> +# delay between data insertion moment on publisher and replay time on
> subscriber.
> 
> Most of this comment is now redundant because this was already
> explained in the big comment up-front (see #3). Only one useful
> sentence is left.
> 
> SUGGESTION
> Before doing the insertion, get the current timestamp that will be
> used as a comparison base.

Removed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED