On Tue, Oct 12, 2021 at 4:00 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I've attached updated patches.
>
Some comments for the v16-0001 patch:
src/backend/postmaster/pgstat.c
(1) pgstat_vacuum_subworker_stat()
Remove "the" from beginning of the following comment line:
+ * the all the dead subscription worker statistics.
(2) pgstat_reset_subscription_error_stats()
This function would be better named "pgstat_reset_subscription_subworker_error".
(3) pgstat_report_subworker_purge()
Improve comment:
BEFORE:
+ * Tell the collector about dead subscriptions.
AFTER:
+ * Tell the collector to remove dead subscriptions.
(4) pgstat_get_subworker_entry()
I notice that in the following code:
+ if (create && !found)
+ pgstat_reset_subworker_error(wentry, 0);
The newly-created PgStat_StatSubWorkerEntry doesn't get the "dbid"
member set, so I think it's a junk value in this case, yet the caller
of pgstat_get_subworker_entry(..., true) is referencing it:
+ /* Get the subscription worker stats */
+ wentry = pgstat_get_subworker_entry(msg->m_subid, msg->m_subrelid, true);
+ Assert(wentry);
+
+ /*
+ * Update only the counter and timestamp if we received the same error
+ * again
+ */
+ if (wentry->dbid == msg->m_dbid &&
+ wentry->relid == msg->m_relid &&
+ wentry->command == msg->m_command &&
+ wentry->xid == msg->m_xid &&
+ strcmp(wentry->message, msg->m_message) == 0)
+ {
+ wentry->count++;
+ wentry->timestamp = msg->m_timestamp;
+ return;
+ }
Maybe the cheapest solution is to just set dbid in
pgstat_reset_subworker_error()?
src/backend/replication/logical/worker.c
(5) Fix typo
synchroniztion -> synchronization
+ * type for table synchroniztion.
Regards,
Greg Nancarrow
Fujitsu Australia
