Re: Skipping logical replication transactions on subscriber side

On Mon, Jan 17, 2022 at 5:03 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> On Monday, January 17, 2022 3:18 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > I've attached an updated patch. Please review it.
> Hi, thank you for sharing a new patch.
> Few comments on the v6.

Thank you for the comments!

>
> (1) doc/src/sgml/ref/alter_subscription.sgml
>
> +      resort.  This option has no effect on the transaction that is already
>
> One TAB exists between "resort" and "This".

Will remove.

>
> (2) Minor improvement suggestion of comment in src/backend/replication/logical/worker.c
>
> + * reset during that.  Also, we don't skip receiving the changes in streaming
> + * cases, since we decide whether or not to skip applying the changes when
>
> I sugguest that you don't use 'streaming cases', because
> what "streaming cases" means sounds a bit broader than actual your implementation.
> We do skip transaction of streaming cases but not during the spooling phase, right ?
>
> I suggest below.
>
> "We don't skip receiving the changes at the phase to spool streaming transactions"

I might be missing your point but I think it's correct that we don't
skip receiving the change of the transaction that is sent via
streaming protocol. And it doesn't sound broader to me. Could you
elaborate on that?

>
> (3) in the comment of apply_handle_prepare_internal, two full-width characters.
>
> 3-1
> +        * won’t be resent in a case where the server crashes between them.
>
> 3-2
> +        * COMMIT PREPARED or ROLLBACK PREPARED. But that’s okay because this
>
> You have full-width characters for "won't" and "that's".
> Could you please check ?

Which characters in "won't" are full-width characters? I could not find them.

>
>
> (4) typo
>
> + * the subscription if hte user has specified skip_xid. Once we start skipping
>
> "hte" should "the" ?

Will fix.

>
> (5)
>
> I can miss something here but, in one of
> the past discussions, there seems a consensus that
> if the user specifies XID of a subtransaction,
> it would be better to skip only the subtransaction.
>
> This time, is it out of the range of the patch ?
> If so, I suggest you include some description about it
> either in the commit message or around codes related to it.

How can the user know subtransaction XID? I suppose you refer to
streaming protocol cases but while applying spooled changes we don't
report subtransaction XID neither in server log nor
pg_stat_subscription_workers.

>
> (6)
>
> I feel it's a better idea to include a test whether
> to skip aborted streaming transaction clears the XID
> in the TAP test for this feature, in a sense to cover
> various new code paths. Did you have any special reason
> to omit the case ?

Which code path is newly covered by this aborted streaming transaction
tests? I think that this patch is already covered even by the test for
a committed-and-streamed transaction. It doesn't matter whether the
streamed transaction is committed or aborted because an error occurs
while applying the spooled changes.

>
> (7)
>
> I want more explanation for the reason to restart the subscriber
> in the TAP test because this is not mandatory operation.
> (We can pass the TAP tests without this restart)
>
> From :
> # Restart the subscriber node to restart logical replication with no interval
>
> IIUC, below would be better.
>
> To :
> # As an optimization to finish tests earlier, restart the subscriber with no interval,
> # rather than waiting for new error to laucher a new apply worker.

I could not understand why the proposed sentence has more information.
Does it mean you want to mention "As an optimization to finish tests
earlier"?

Regards,

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