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

On Tue, Feb 21, 2023 at 3:31 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
>
> 2.
> The combination of parallel streaming mode and min_send_delay is not 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.
>
> ~
>
> Is there another reason not to support this?
>
> Even if streaming + min_send_delay incurs some extra delay, is that a
> reason to reject outright the combination? What difference will the
> potential of a few extra seconds overhead make when min_send_delay is
> more likely to be far greater (e.g. minutes or hours)?
>

I think the point is that we don't know the commit time at the start
of streaming and even the transaction can be quite long in which case
adding the delay is not expected.

>
> ======
> doc/src/sgml/catalogs.sgml
>
> 4.
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>subminsenddelay</structfield> <type>int4</type>
> +      </para>
> +      <para>
> +       The minimum delay, in milliseconds, by the publisher to send changes
> +      </para></entry>
> +     </row>
>
> "by the publisher to send changes" --> "by the publisher before sending changes"
>

For the streaming (=on) case, we may end up sending changes before we
start to apply delay.

> ======
> doc/src/sgml/monitoring.sgml
>
> 6.
> +     <row>
> +      <entry><literal>WalSenderSendDelay</literal></entry>
> +      <entry>Waiting while sending changes for time-delayed logical replication
> +      in the WAL sender process.</entry>
> +     </row>
>
> Should this say "Waiting before sending changes", instead of "Waiting
> while sending changes"?
>

In the streaming (non-parallel) case, we may have sent some changes
before wait as we wait only at commit/prepare time. The downstream
won't apply such changes till commit. So, this description makes sense
and this matches similar nearby descriptions.

>
> 8.
> +         <para>
> +          The delay is effective only when the initial table synchronization
> +          has been finished and the publisher decides to send a particular
> +          transaction downstream. 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>
>
> I'm not sure about this mention about only "effective only when the
> initial table synchronization has been finished"... Now that the delay
> is pub-side I don't know that it is true anymore.
>

This will still be true because we don't wait during the initial copy
(sync). The delay happens only when the replication starts.

> ======
> src/backend/commands/subscriptioncmds.c
> ======
> src/backend/replication/pgoutput/pgoutput.c
>
> 11.
> + errno = 0;
> + parsed = strtoul(strVal(defel->arg), &endptr, 10);
> + if (errno != 0 || *endptr != '\0')
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("invalid min_send_delay")));
> +
> + if (parsed > PG_INT32_MAX)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("min_send_delay \"%s\" out of range",
> + strVal(defel->arg))));
>
> Should the validation be also checking/asserting no negative numbers,
> or actually should the min_send_delay be defined as a uint32 in the
> first place?
>

I don't see the need to change the datatype of min_send_delay as
compared to what we have min_apply_delay.

> ======
> src/include/replication/logical.h
>
> 15.
> @@ -64,6 +68,7 @@ typedef struct LogicalDecodingContext
>   LogicalOutputPluginWriterPrepareWrite prepare_write;
>   LogicalOutputPluginWriterWrite write;
>   LogicalOutputPluginWriterUpdateProgress update_progress;
> + LogicalOutputPluginWriterDelay delay;
>
> ~
>
> 15a.
> Question: Is there some advantage to introducing another callback,
> instead of just doing the delay inline?
>

This is required because we need to check walsender's timeout and or
process replies during the delay.

-- 
With Regards,
Amit Kapila.