Re: Skipping logical replication transactions on subscriber side

On Tue, Jan 18, 2022 at 10:36 AM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
> On Mon, Jan 17, 2022 at 5:18 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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/