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
