Re: Skipping logical replication transactions on subscriber side

On Mon, Mar 14, 2022 at 6:50 PM shiy.fnst@fujitsu.com
<shiy.fnst@fujitsu.com> wrote:
>
> On Fri, Mar 11, 2022 4:20 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached an updated version patch. This patch can be applied on
> > top of the latest disable_on_error patch[1].
> >
>
> Thanks for your patch. Here are some comments for the v13 patch.

Thank you for the comments!

>
> 1. doc/src/sgml/ref/alter_subscription.sgml
> +          Specifies the transaction's finish LSN of the remote transaction whose changes
>
> Could it be simplified to "Specifies the finish LSN of the remote transaction
> whose ...".

Fixed.

>
> 2.
> I met a failed assertion, the backtrace is attached. This is caused by the
> following code in maybe_start_skipping_changes().
>
> +               /*
> +                * It's a rare case; a past subskiplsn was left because the server
> +                * crashed after preparing the transaction and before clearing the
> +                * subskiplsn. We clear it without a warning message so as not confuse
> +                * the user.
> +                */
> +               if (unlikely(MySubscription->skiplsn < lsn))
> +               {
> +                       clear_subscription_skip_lsn(MySubscription->skiplsn, InvalidXLogRecPtr, 0,
> +                                                                               false);
> +                       Assert(!IsTransactionState());
> +               }
>
> We want to clear subskiplsn in the case mentioned in comment. But if the next
> transaction is a steaming transaction and this function is called by
> apply_spooled_messages(), we are inside a transaction here. So, I think this
> assertion is not suitable for streaming transaction. Thoughts?

Good catch. After more thought, I realized that the assumption of this
if statement is wrong and we don't necessarily need to do here since
the left skip-LSN will eventually be cleared when the next transaction
is finished. So removed this part.

>
> 3.
> +       XLogRecPtr      subskiplsn;             /* All changes which committed at this LSN are
> +                                                                * skipped */
>
> To be consistent, should the comment be changed to "All changes which finished
> at this LSN are skipped"?

Fixed.

>
> 4.
> +      After logical replication worker successfully skips the transaction or commits
> +      non-empty transaction, the LSN (stored in
> +      <structname>pg_subscription</structname>.<structfield>subskiplsn</structfield>)
> +      is cleared.
>
> Besides "commits non-empty transaction", subskiplsn would also be cleared in
> some two-phase commit cases I think. Like prepare/commit/rollback a transaction,
> even if it is an empty transaction. So, should we change it for these cases?

Fixed.

>
> 5.
> + * Clear subskiplsn of pg_subscription catalog with origin state update.
>
> Should "with origin state update" modified to "with origin state updated"?

Fixed.

I'll submit an updated patch soon.

Regards,

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