Re: Skipping logical replication transactions on subscriber side

On Wed, Jan 5, 2022 at 12:31 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Dec 27, 2021 at 9:54 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Dec 16, 2021 at 2:42 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Thu, Dec 16, 2021 at 2:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Thu, Dec 16, 2021 at 10:37 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > > >
> > > > > On Thu, Dec 16, 2021 at 11:43 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > > >
> > > > > > I thought we just want to lock before clearing the skip_xid something
> > > > > > like take the lock, check if the skip_xid in the catalog is the same
> > > > > > as we have skipped, if it is the same then clear it, otherwise, leave
> > > > > > it as it is. How will that disallow users to change skip_xid when we
> > > > > > are skipping changes?
> > > > >
> > > > > Oh I thought we wanted to keep holding the lock while skipping changes
> > > > > (changing skip_xid requires acquiring the lock).
> > > > >
> > > > > So if skip_xid is already changed, the apply worker would do
> > > > > replorigin_advance() with WAL logging, instead of committing the
> > > > > catalog change?
> > > > >
> > > >
> > > > Right. BTW, how are you planning to advance the origin? Normally, a
> > > > commit transaction would do it but when we are skipping all changes,
> > > > the commit might not do it as there won't be any transaction id
> > > > assigned.
> > >
> > > I've not tested it yet but replorigin_advance() with wal_log = true
> > > seems to work for this case.
> >
> > I've tested it and realized that we cannot use replorigin_advance()
> > for this purpose without changes. That is, the current
> > replorigin_advance() doesn't allow to advance the origin by the owner:
> >
> >         /* Make sure it's not used by somebody else */
> >         if (replication_state->acquired_by != 0)
> >         {
> >             ereport(ERROR,
> >                     (errcode(ERRCODE_OBJECT_IN_USE),
> >                      errmsg("replication origin with OID %d is already
> > active for PID %d",
> >                             replication_state->roident,
> >                             replication_state->acquired_by)));
> >         }
> >
> > So we need to change it so that the origin owner can advance its
> > origin, which makes sense to me.
> >
> > Also, when we have to update the origin instead of committing the
> > catalog change while updating the origin, we cannot record the origin
> > timestamp.
> >
>
> Is it because we currently update the origin timestamp with commit record?

Yes.

>
> > This behavior makes sense to me because we skipped the
> > transaction. But ISTM it’s not good if we emit the origin timestamp
> > only when directly updating the origin. So probably we need to always
> > omit origin timestamp.
> >
>
> Do you mean to say that you want to omit it even when we are
> committing the changes?

Yes, it would be better to record only origin lsn in terms of consistency.

>
> > Apart from that, I'm vaguely concerned that the logic seems to be
> > getting complex. Probably it comes from the fact that we store
> > skip_xid in the catalog and update the catalog to clear/set the
> > skip_xid. It might be worth revisiting the idea of storing skip_xid on
> > shmem (e.g., ReplicationState)?
> >
>
> IIRC, the problem with that idea was that we won't remember skip_xid
> information after server restart and the user won't even know that it
> has to set it again.

Right, I agree that it’s not convenient when the server restarts or
crashes, but these problems could not be critical in the situation
where users have to use this feature; the subscriber already entered
an error loop so they can know xid again and it’s an uncommon case
that they need to restart during skipping changes.

Anyway, I'll submit an updated patch soon so we can discuss complexity
vs. convenience.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/