Re: Skipping logical replication transactions on subscriber side

On Thu, Sep 2, 2021 at 9:03 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
> On Mon, Aug 30, 2021 at 5:07 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > I've attached rebased patches. 0004 patch is not the scope of this
> > patch. It's borrowed from another thread[1] to fix the assertion
> > failure for newly added tests. Please review them.
> >
>

Thank you for the comments!

> Some initial comments for the v12-0003 patch:
>
> (1) Patch comment
> "This commit introduces another way to skip the transaction in question."
>
> I think it should further explain: "This commit introduces another way
> to skip the transaction in question, other than manually updating the
> subscriber's database or using pg_replication_origin_advance()."

Updated.

>
>
> doc/src/sgml/logical-replication.sgml
> (2)
>
> Suggested minor update:
>
> BEFORE:
> + transaction that conflicts with the existing data. When a conflict produce
> + an error, it is shown in
> <structname>pg_stat_subscription_errors</structname>
> + view as follows:
> AFTER:
> + transaction that conflicts with the existing data. When a conflict produces
> + an error, it is recorded in the
> <structname>pg_stat_subscription_errors</structname>
> + view as follows:

Fixed.

>
> (3)
> + found from those outputs (transaction ID 740 in the above case).
> The transaction
>
> Shouldn't it be transaction ID 716?

Right, fixed.

>
> (4)
> + can be skipped by setting <replaceable>skip_xid</replaceable> to
> the subscription
>
> Is it better to say here ... "on the subscription" ?

Okay, fixed.

>
> (5)
> Just skipping a transaction could make a subscriber inconsistent, right?
>
> Would it be better as follows?
>
> BEFORE:
> + In either way, those should be used as a last resort. They skip the whole
> + transaction including changes that may not violate any constraint and easily
> + make subscriber inconsistent if a user specifies the wrong transaction ID or
> + the position of origin.
>
> AFTER:
> + Either way, those transaction skipping methods should be used as a
> last resort.
> + They skip the whole transaction, including changes that may not violate any
> + constraint. They may easily make the subscriber inconsistent,
> especially if a
> + user specifies the wrong transaction ID or the position of origin.

Agreed, fixed.

>
> (6)
> The grammar is not great in the following description, so here's a
> suggested improvement:
>
> BEFORE:
> + incoming change or by skipping the whole transaction. This option
> + specifies transaction ID that logical replication worker skips to
> + apply. The logical replication worker skips all data modification
>
> AFTER:
> + incoming changes or by skipping the whole transaction. This option
> + specifies the ID of the transaction whose application is to
> be skipped
> + by the logical replication worker. The logical replication worker
> + skips all data modification

Fixed.

>
>
> src/backend/postmaster/pgstat.c
> (7)
> BEFORE:
> + * Tell the collector about clear the error of subscription.
> AFTER:
> + * Tell the collector to clear the subscription error.

Fixed.

>
>
> src/backend/replication/logical/worker.c
> (8)
> + * subscription is invalidated and* MySubscription->skipxid gets
> changed or reset.
>
> There is a "*" after "and".

Fixed.

>
> (9)
> Do these lines really need to be moved up?
>
> + /* extract XID of the top-level transaction */
> + stream_xid = logicalrep_read_stream_start(s, &first_segment);
> +

I had missed to revert this change, fixed.

>
> src/backend/postmaster/pgstat.c
> (10)
>
> + bool m_clear; /* clear all fields except for last_failure and
> + * last_errmsg */
>
> I think it should say: clear all fields except for last_failure,
> last_errmsg and stat_reset_timestamp.

Fixed.

Those comments including your comments on the v12-0001 and v12-0002
are incorporated into local branch. I'll submit the updated patches
after incorporating all other comments.

Regards,

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