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/
