Re: Skipping logical replication transactions on subscriber side

On Sat, Dec 11, 2021 at 3:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Dec 10, 2021 at 11:14 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Dec 9, 2021 at 6:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Dec 9, 2021 at 2:24 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Thu, Dec 9, 2021 at 11:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > > I am thinking that we can start a transaction, update the catalog,
> > > > > commit that transaction. Then start a new one to update
> > > > > origin_lsn/timestamp, finishprepared, and commit it. Now, if it
> > > > > crashes after the first transaction, only commit prepared will be
> > > > > resent again and this time we don't need to update the catalog as that
> > > > > entry would be already cleared.
> > > >
> > > > Sounds good. In the crash case, it should be fine since we will just
> > > > commit an empty transaction. The same is true for the case where
> > > > skip_xid has been changed after skipping and preparing the transaction
> > > > and before handling commit_prepared.
> > > >
> > > > Regarding the case where the user specifies XID of the transaction
> > > > after it is prepared on the subscriber (i.g., the transaction is not
> > > > empty), we won’t skip committing the prepared transaction. But I think
> > > > that we don't need to support skipping already-prepared transaction
> > > > since such transaction doesn't conflict with anything regardless of
> > > > having changed or not.
> > > >
> > >
> > > Yeah, this makes sense to me.
> > >
> >
> > I've attached an updated patch. The new syntax is like "ALTER
> > SUBSCRIPTION testsub SKIP (xid = '123')".
> >
> > I’ve been thinking we can do something safeguard for the case where
> > the user specified the wrong xid. For example, can we somewhat use the
> > stats in pg_stat_subscription_workers? An idea is that logical
> > replication worker fetches the xid from the stats when reading the
> > subscription and skips the transaction if the xid matches to
> > subskipxid. That is, the worker checks the error reported by the
> > worker previously working on the same subscription. The error could
> > not be a conflict error (e.g., connection error etc.) or might have
> > been cleared by the reset function, But given the worker is in an
> > error loop, the worker can eventually get xid in question. We can
> > prevent an unrelated transaction from being skipped unexpectedly. It
> > seems not a stable solution though. Or it might be enough to warn
> > users when they specified an XID that doesn’t match to last_error_xid.
> >
>
> I think the idea is good but because it is not predictable as pointed
> by you so we might want to just issue a LOG/WARNING. If not already
> mentioned, then please do mention in docs the possibility of skipping
> non-errored transactions.
>
> Few comments/questions:
> =====================
> 1.
> +          Specifies the ID of the transaction whose application is to
> be skipped
> +          by the logical replication worker. Setting -1 means to reset the
> +          transaction ID.
>
> Can we change it to something like: "Specifies the ID of the
> transaction whose changes are to be skipped by the logical replication
> worker. ...."
>

Agreed.

> 2.
> @@ -104,6 +104,16 @@ GetSubscription(Oid subid, bool missing_ok)
>   Assert(!isnull);
>   sub->publications = textarray_to_stringlist(DatumGetArrayTypeP(datum));
>
> + /* Get skip XID */
> + datum = SysCacheGetAttr(SUBSCRIPTIONOID,
> + tup,
> + Anum_pg_subscription_subskipxid,
> + &isnull);
> + if (!isnull)
> + sub->skipxid = DatumGetTransactionId(datum);
> + else
> + sub->skipxid = InvalidTransactionId;
>
> Can't we assign it as we do for other fixed columns like subdbid,
> subowner, etc.?
>

Yeah, I think we can use InvalidTransactionId as the initial value
instead of setting NULL. Then, we can change this code.

> 3.
> + * Also, we don't skip receiving the changes in streaming cases,
> since we decide
> + * whether or not to skip applying the changes when starting to apply changes.
>
> But why so? Can't we even skip streaming (and writing to file all such
> messages)? If we can do this then we can avoid even collecting all
> messages in a file.

IIUC in streaming cases, a transaction can be sent to the subscriber
while splitting into multiple chunks of changes. In the meanwhile,
skip_xid can be changed. If the user changed or cleared skip_xid after
the subscriber skips some streamed changes, the subscriber won't able
to have complete changes of the transaction.

>
> 4.
> + * Also, one might think that we can skip preparing the skipped transaction.
> + * But if we do that, PREPARE WAL record won’t be sent to its physical
> + * standbys, resulting in that users won’t be able to find the prepared
> + * transaction entry after a fail-over.
> + *
> ..
> + */
> + if (skipping_changes)
> + stop_skipping_changes(false);
>
> Why do we need such a Prepare's entry either at current subscriber or
> on its physical standby? I think it is to allow Commit-prepared. If
> so, how about if we skip even commit prepared as well? Even on
> physical standby, we would be having the value of skip_xid which can
> help us to skip there as well after failover.

It's true that skip_xid would be set also on physical standby. When it
comes to preparing the skipped transaction on the current subscriber,
if we want to skip commit-prepared I think we need protocol changes in
order for subscribers to know prepare_lsn and preppare_timestampso
that it can lookup the prepared transaction when doing
commit-prepared. I proposed this idea before. This change would be
benefical as of now since the publisher sends even empty transactions.
But considering the proposed patch[1] that makes the puslisher not
send empty transaction, this protocol change would be an optimization
only for this feature.

Regards,

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