Re: Skipping logical replication transactions on subscriber side

On Mon, Dec 27, 2021 at 9:54 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Thu, Dec 16, 2021 at 2:42 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Thu, Dec 16, 2021 at 2:21 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Dec 16, 2021 at 10:37 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > > > On Thu, Dec 16, 2021 at 11:43 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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?

> 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?

> 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.

--
With Regards,
Amit Kapila.