On Wed, Oct 13, 2021 at 10:59 AM Greg Nancarrow <gregn4...@gmail.com> wrote:
>
> On Tue, Oct 12, 2021 at 4:00 PM Masahiko Sawada <sawada.m...@gmail.com> wrote:
> >
> > I've attached updated patches.
> >
>
> Some comments for the v16-0001 patch:
>
Thank you for the comments!
>
> 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.
Fixed.
>
>
> (2) pgstat_reset_subscription_error_stats()
>
> This function would be better named
> "pgstat_reset_subscription_subworker_error".
'subworker' contains an abbreviation of 'subscription'. So it seems
redundant to me. No?
>
>
> (3) pgstat_report_subworker_purge()
>
> Improve comment:
>
> BEFORE:
> + * Tell the collector about dead subscriptions.
> AFTER:
> + * Tell the collector to remove dead subscriptions.
Fixed.
>
>
> (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()?
I've change the code to reset dbid in pgstat_reset_subworker_error().
>
>
> src/backend/replication/logical/worker.c
>
> (5) Fix typo
>
> synchroniztion -> synchronization
>
> + * type for table synchroniztion.
Fixed.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
