On Tue, Jul 5, 2022 at 2:12 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for your v4-0001 patch. I hope they are
> useful for you.
>
> ======
>
> 1. General
>
> This thread name "logical replication restrictions" seems quite
> unrelated to the patch here. Maybe it's better to start a new thread
> otherwise nobody is going to recognise what this thread is really
> about.
>
+1.
>
> 17. src/backend/replication/logical/worker.c - apply_handle_stream_prepare
>
> @@ -1090,6 +1146,19 @@ apply_handle_stream_prepare(StringInfo s)
>
> elog(DEBUG1, "received prepare for streamed transaction %u",
> prepare_data.xid);
>
> + /*
> + * Should we delay the current prepared transaction?
> + *
> + * Although the delay is applied in BEGIN PREPARE messages, streamed
> + * prepared transactions apply the delay in a STREAM PREPARE message.
> + * That's ok because no changes have been applied yet
> + * (apply_spooled_messages() will do it).
> + * The STREAM START message does not contain a prepare time (it will be
> + * available when the in-progress prepared transaction finishes), hence, it
> + * was not possible to apply a delay at that time.
> + */
> + apply_delay(prepare_data.prepare_time);
> +
>
> It seems to rely on the spooling happening at the end. But won't this
> cause a problem later when/if the "parallel apply" patch [1] is pushed
> and the stream bgworkers are doing stuff on the fly instead of
> spooling at the end?
>
I wonder why we don't apply the delay on commit/commit_prepared
records only similar to physical replication. See recoveryApplyDelay.
One more advantage would be then we don't need to worry about
transactions that we are going to skip due SKIP feature for
subscribers.
One more thing that might be worth discussing is whether introducing a
new subscription parameter for this feature is a better idea or can we
use guc (either an existing or a new one). Users may want to set this
only for a particular subscription or set of subscriptions in which
case it is better to have this as a subscription level parameter.
OTOH, I was slightly worried that if this will be used for all
subscriptions on a subscriber then it will be burdensome for users.
--
With Regards,
Amit Kapila.