Re: Skipping logical replication transactions on subscriber side

On Fri, Oct 29, 2021 at 4:24 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> I've attached a new version patch. Since the syntax of skipping
> transaction id is under the discussion I've attached only the error
> reporting patch for now.
>

I have some comments on the v19-0001 patch:

v19-0001

(1) doc/src/sgml/monitoring.sgml
Seems to be missing the word "information":

BEFORE:
+ <entry>At least one row per subscription, showing about errors that
+ occurred on subscription.
AFTER:
+ <entry>At least one row per subscription, showing information about
+ errors that occurred on subscription.

(2) pg_stat_reset_subscription_worker(subid Oid, relid Oid)
First of all, I think that the documentation for this function should
make it clear that a non-NULL "subid" parameter is required for both
reset cases (tablesync and apply).
Perhaps this could be done by simply changing the first sentence to say:
"Resets statistics of a single subscription worker error, for a worker
running on subscription with <parameter>subid</parameter>."
(and then can remove " running on the subscription with
<parameter>subid</parameter>" from the last sentence)

I think that the documentation for this function should say that it
should be used in conjunction with the "pg_stat_subscription_workers"
view in order to obtain the required subid/relid values for resetting.
(and should provide a link to the documentation for that view)
Also, I think that the function documentation should make it clear
that the tablesync error case is indicated by a NULL "command" in the
information returned from the "pg_stat_subscription_workers" view
(otherwise the user needs to look at the server log in order to
determine whether the error is for the apply/tablesync worker).

Finally, there are currently no tests for this new function.

(3) pg_stat_subscription_workers
In the documentation for this, the description for the "command"
column says: "This field is always NULL if the error was reported
during the initial data copy."
Some users may not realise that this refers to "tablesync", so perhaps
add " (tablesync)" to the end of this sentence, or similar.

Regards,
Greg Nancarrow
Fujitsu Australia