RE: Skipping logical replication transactions on subscriber side

On Monday, January 17, 2022 5:03 PM I wrote:
> Hi, thank you for sharing a new patch.
> Few comments on the v6.
> 
> (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".
> 
> (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"
> 
> (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 ?
> 
> 
> (4) typo
> 
> + * the subscription if hte user has specified skip_xid. Once we start
> + skipping
> 
> "hte" should "the" ?
> 
> (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.
> 
> (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 ?
> 
> (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.
Few more minor comments

(8) another full-width char in apply_handle_commit_prepared


+                * PREPARED won't be resent but subskipxid is left.

Kindly check "won't" ?

(9) the header comments of clear_subscription_skip_xid

+/* clear subskipxid of pg_subscription catalog */

Should start with an upper letter ?

(10) some variable declarations and initialization of clear_subscription_skip_xid

There's no harm in moving below codes into a condition case
where the user didn't change the subskipxid before
apply worker clearing it.

+       bool            nulls[Natts_pg_subscription];
+       bool            replaces[Natts_pg_subscription];
+       Datum           values[Natts_pg_subscription];
+
+       memset(values, 0, sizeof(values));
+       memset(nulls, false, sizeof(nulls));
+       memset(replaces, false, sizeof(replaces));


Best Regards,
    Takamichi Osumi