On Thu, Sep 30, 2021 at 3:45 PM Masahiko Sawada <sawada.mshk@gmail.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