Re: Skipping logical replication transactions on subscriber side

On Tue, Jan 18, 2022 at 10:36 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Mon, Jan 17, 2022 at 5:18 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached an updated patch. Please review it.
> >
>
> Some review comments for the v6 patch:

Thank you for the comments!

>
>
> doc/src/sgml/logical-replication.sgml
>
> (1) Expanded output
>
> Since the view output is shown in "expanded output" mode, perhaps the
> doc should say that, or alternatively add the following lines prior to
> it, to make it clear:
>
>   postgres=# \x
>   Expanded display is on.

I'm not sure it's really necessary. A similar example would be
perform.sgml but it doesn't say "\x".

>
>
> (2) Message output in server log
>
> The actual CONTEXT text now just says "at ..." instead of "with commit
> timestamp ...", so the doc needs to be updated as follows:
>
> BEFORE:
> +CONTEXT:  processing remote data during "INSERT" for replication
> target relation "public.test" in transaction 716 with commit timestamp
> 2021-09-29 15:52:45.165754+00
> AFTER:
> +CONTEXT:  processing remote data during "INSERT" for replication
> target relation "public.test" in transaction 716 at 2021-09-29
> 15:52:45.165754+00

Will fix.

>
> (3)
> The wording "the change" doesn't seem right here, so I suggest the
> following update:
>
> BEFORE:
> +   Skipping the whole transaction includes skipping the change that
> may not violate
> AFTER:
> +   Skipping the whole transaction includes skipping changes that may
> not violate
>
>
> doc/src/sgml/ref/alter_subscription.sgml

Will fix.

>
> (4)
> I have a number of suggested wording improvements:
>
> BEFORE:
> +      Skips applying changes of the particular transaction.  If incoming data
> +      violates any constraints the logical replication will stop until it is
> +      resolved.  The resolution can be done either by changing data on the
> +      subscriber so that it doesn't conflict with incoming change or
> by skipping
> +      the whole transaction.  The logical replication worker skips all data
> +      modification changes within the specified transaction including
> the changes
> +      that may not violate the constraint, so, it should only be used as a last
> +      resort. This option has no effect on the transaction that is already
> +      prepared by enabling <literal>two_phase</literal> on subscriber.
>
> AFTER:
> +      Skips applying all changes of the specified transaction.  If
> incoming data
> +      violates any constraints, logical replication will stop until it is
> +      resolved.  The resolution can be done either by changing data on the
> +      subscriber so that it doesn't conflict with incoming change or
> by skipping
> +      the whole transaction.  Using the SKIP option, the logical
> replication worker skips all data
> +      modification changes within the specified transaction, including changes
> +      that may not violate the constraint, so, it should only be used as a last
> +      resort. This option has no effect on transactions that are already
> +      prepared by enabling <literal>two_phase</literal> on the subscriber.
>

Will fix.

>
> (5)
> change -> changes
>
> BEFORE:
> +      subscriber so that it doesn't conflict with incoming change or
> by skipping
> AFTER:
> +      subscriber so that it doesn't conflict with incoming changes or
> by skipping

Will fix.

>
>
> src/backend/replication/logical/worker.c
>
> (6) Missing word?
> The following should say "worth doing" or "worth it"?
>
> + * doesn't seem to be worth since it's a very minor case.
>

WIll fix

>
> src/test/regress/sql/subscription.sql
>
> (7) Misleading test case
> I think the following test case is misleading and should be removed,
> because the "1.1" xid value is only regarded as invalid because "1" is
> an invalid xid (and there's already a test case for a "1" xid) - the
> fractional part gets thrown away, and doesn't affect the validity
> here.
>
>    +ALTER SUBSCRIPTION regress_testsub SKIP (xid = 1.1);
>

Good point. Will remove.

Regards,

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