Horton

Re: Skipping logical replication transactions on subscriber side

Greg Nancarrow <gregn4422@gmail.com>

View thread View on postgr.es
From: Greg Nancarrow <gregn4422@gmail.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>, "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>, Alexey Lesovsky <lesovsky@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2021-11-02T03:51:34Z
Lists: pgsql-hackers 
On Fri, Oct 29, 2021 at 4:24 PM Masahiko Sawada <sawada.mshk@gmail.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

Commits

  1. Test ALIGNOF_DOUBLE==4 compatibility under ALIGNOF_DOUBLE==8.

  2. Reorder subskiplsn in pg_subscription to avoid alignment issues.

  3. Add ALTER SUBSCRIPTION ... SKIP.

  4. Optionally disable subscriptions on error.

  5. Update docs of logical replication for commit 8d74fc96db.

  6. Respect permissions within logical replication.

  7. Fix regression test failure caused by commit 8d74fc96db.

  8. Add a view to show the stats of subscription workers.

  9. Add logical change details to logical replication worker errcontext.

  10. Rename LOGICAL_REP_MSG_STREAM_END to LOGICAL_REP_MSG_STREAM_STOP.

  11. Fix typo in protocol.sgml.

  12. Remove unused argument in apply_handle_commit_internal().

  13. Fix replication of in-progress transactions in tablesync worker.

  14. Reorder pg_sequence columns to avoid alignment issue