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

On Mon, Feb 27, 2023 at 3:34 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Feb 27, 2023 at 11:11 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Thu, Feb 23, 2023 at 9:10 PM Hayato Kuroda (Fujitsu)
> > <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> > >
> > > Thank you for reviewing! PSA new version.
> > >
> > >
> >
> > Thank you for updating the patch. Here are some comments on v7 patch:
> >
> > + *
> > + * LOGICALREP_PROTO_MIN_SEND_DELAY_VERSION_NUM is the minimum protocol version
> > + * with support for delaying to send transactions. Introduced in PG16.
> > */
> > #define LOGICALREP_PROTO_MIN_VERSION_NUM 1
> > #define LOGICALREP_PROTO_VERSION_NUM 1
> > #define LOGICALREP_PROTO_STREAM_VERSION_NUM 2
> > #define LOGICALREP_PROTO_TWOPHASE_VERSION_NUM 3
> > #define LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM 4
> > -#define LOGICALREP_PROTO_MAX_VERSION_NUM
> > LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM
> > +#define LOGICALREP_PROTO_MIN_SEND_DELAY_VERSION_NUM 4
> > +#define LOGICALREP_PROTO_MAX_VERSION_NUM
> > LOGICALREP_PROTO_MIN_SEND_DELAY_VERSION_NUM
> >
> > What is the usecase of the old macro,
> > LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM, after adding
> > LOGICALREP_PROTO_MIN_SEND_DELAY_VERSION_NUM ? I think if we go this
> > way, we will end up adding macros every time when adding a new option,
> > which seems not a good idea. I'm really not sure we need to change the
> > protocol version or the macro. Commit
> > 366283961ac0ed6d89014444c6090f3fd02fce0a introduced the 'origin'
> > subscription parameter that is also sent to the publisher, but we
> > didn't touch the protocol version at all.
> >
>
> Right, I also don't see a reason to do anything for this. We have
> previously bumped the protocol version when we send extra/additional
> information from walsender but here that is not the requirement, so
> this change doesn't seem to be required.
>
> > ---
> > Why do we not to delay sending COMMIT PREPARED messages?
> >
>
> I think we need to either add delay for prepare or commit prepared as
> otherwise, it will lead to delaying the xact more than required.

Agreed.

> The
> patch seems to add a delay before sending a PREPARE as that is the
> time when the subscriber will apply the changes.

Considering the purpose of this feature mentioned in the commit
message "particularly to fix errors that might cause data loss",
delaying sending PREPARE would really help that situation? For
example, even after (mistakenly) executing PREPARE for a transaction
executing DELETE without WHERE clause on the publisher the user still
can rollback the transaction. They don't lose data on both nodes yet.
After executing (and replicating) COMMIT PREPARED for that
transaction, they lose the data on both nodes. IIUC the time-delayed
logical replication should help this situation by delaying sending
COMMIT PREPARED so that, for example, the user can stop logical
replication before COMMIT PREPARED message arrives to the subscriber.
So I think we should delay sending COMMIT PREPARED (and COMMIT)
instead of PREPARE. This would help users to correct data loss errors,
and would be more consistent with what recovery_min_apply_delay does.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com