Re: Skipping logical replication transactions on subscriber side

On Thu, Sep 30, 2021 at 3:45 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> I've attached updated patches that incorporate all comments I got so
> far. Please review them.
>

Some comments about the v15-0001 patch:

(1) patch adds a whitespace error

Applying: Add a subscription errors statistics view
"pg_stat_subscription_errors".
.git/rebase-apply/patch:1656: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

(2) Patch comment says "This commit adds a new system view
pg_stat_logical_replication_errors ..."
BUT this is the wrong name, it should be "pg_stat_subscription_errors".

doc/src/sgml/monitoring.sgml

(3)
"Message of the error" doesn't sound right. I suggest just saying "The
error message".

(4) view column "last_failed_time"
I think it would be better to name this "last_error_time".

src/backend/postmaster/pgstat.c

(5) pgstat_vacuum_subworker_stats()

Spelling mistake in the following comment:

/* Create a map for mapping subscriptoin OID and database OID */

subscriptoin -> subscription

(6)
In the following functions:

pgstat_read_statsfiles
pgstat_read_db_statsfile_timestamp

The following comment should say "... struct describing subscription
worker statistics."
(i.e. need to remove the "a")

+ * 'S' A PgStat_StatSubWorkerEntry struct describing a
+ * subscription worker statistics.

(7) pgstat_get_subworker_entry

Suggest comment change:

BEFORE:
+ * Return the entry of subscription worker entry with the subscription
AFTER:
+ * Return subscription worker entry with the given subscription

(8) pgstat_recv_subworker_error

+ /*
+ * Update only the counter and timestamp if we received the same error
+ * again
+ */
+ if (wentry->relid == msg->m_relid &&
+ wentry->command == msg->m_command &&
+ wentry->xid == msg->m_xid &&
+ strncmp(wentry->message, msg->m_message, strlen(wentry->message)) == 0)
+ {

Is there a reason that the above check uses strncmp() with
strlen(wentry->message), instead of just strcmp()?
msg->m_message is treated as the same error message if it is the same
up to strlen(wentry->message)?
Perhaps if that is intentional, then the comment should be updated.

src/tools/pgindent/typedefs.list

(9)
The added "PgStat_SubWorkerError" should be removed from the
typedefs.list (as there is no such new typedef).

Regards,
Greg Nancarrow
Fujitsu Australia