Re: Skipping logical replication transactions on subscriber side

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

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)? That way, we can always advance the
origin by replorigin_advance() and don’t need to worry about a complex
case like the server crashes during preparing the transaction. I’ve
not considered the downside yet enough, though.

Regards,

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