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

Dear Amit,

Thank you for reviewing! PSA new version.

> 1.
> +      <para>
> +       The minimum delay for publisher sends data, in milliseconds
> +      </para></entry>
> +     </row>
> 
> It would probably be better to write it as "The minimum delay, in
> milliseconds, by the publisher to send changes"

Fixed.

> 2. The subminsenddelay is placed inconsistently in the patch. In the
> docs (catalogs.sgml), system_views.sql, and in some places in the
> code, it is after subskiplsn, but in the catalog table and
> corresponding structure, it is placed after subowner. It should be
> consistently placed after the subscription owner.

Basically moved. Note that some parts were not changed like
maybe_reread_subscription() because the ordering had been already broken.

> 3.
> +     <row>
> +      <entry><literal>WalSenderSendDelay</literal></entry>
> +      <entry>Waiting for sending changes to subscriber in WAL sender
> +      process.</entry>
> 
> How about writing it as follows: "Waiting while sending changes for
> time-delayed logical replication in the WAL sender process."?

Fixed.

> 4.
> +         <para>
> +          Any delay becomes effective only after all initial table
> +          synchronization has finished and occurs before each transaction
> +          starts to get applied on the subscriber. The delay does not take into
> +          account the overhead of time spent in transferring the transaction,
> +          which means that the arrival time at the subscriber may be delayed
> +          more than the given time.
> +         </para>
> 
> This needs to change based on a new approach. It should be something
> like: "The delay is effective only when the publisher decides to send
> a particular transaction downstream."

Right, the first sentence is partially changed as you said.

> 5.
> + * allowed. This is because in parallel streaming mode, we start applying
> + * the transaction stream as soon as the first change arrives without
> + * knowing the transaction's prepare/commit time. Always waiting for the
> + * full 'min_send_delay' period might include unnecessary delay.
> + *
> + * The other possibility was to apply the delay at the end of the parallel
> + * apply transaction but that would cause issues related to resource bloat
> + * and locks being held for a long time.
> + */
> 
> This part of the comments seems to imply more of a subscriber-side
> delay approach. I think we should try to adjust these as per the
> changed approach.

Adjusted.

> 6.
> @@ -666,6 +666,10 @@ DecodeCommit(LogicalDecodingContext *ctx,
> XLogRecordBuffer *buf,
>   buf->origptr, buf->endptr);
>   }
> 
> + /* Delay given time if the context has 'delay' callback */
> + if (ctx->delay)
> + ctx->delay(ctx, commit_time);
> +
> 
> I think we should invoke delay functionality only when
> ctx->min_send_delay > 0. Otherwise, there will be some unnecessary
> overhead. We can change the comment along the lines of: "Delay sending
> the changes if required. For streaming transactions, this means a
> delay in sending the last stream but that is okay because on the
> downstream the changes will be applied only after receiving the last
> stream."

Changed accordingly.

> 7. For 2PC transactions, I think we should add the delay in
> DecodePrerpare. Because after receiving the PREPARE, the downstream
> will apply the xact. In this case, we shouldn't add a delay for the
> commit_prepared.

Right, the transaction will be end when it receive PREPARE. Fixed. 
I've tested locally and the delay seemed to be occurred at PREPARE phase.

> 8.
> +#
> +# If the subscription sets min_send_delay parameter, the logical replication
> +# worker will delay the transaction apply for min_send_delay milliseconds.
> 
> I think here also comments should be updated as per the changed
> approach for applying the delay on the publisher side.

Fixed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED