Re: Skipping logical replication transactions on subscriber side

On Fri, Oct 8, 2021 at 8:17 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
> 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:

Thank you for the comments!

>
> (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.

Fixed.

>
> (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".
>
>

Fixed.

> doc/src/sgml/monitoring.sgml
>
> (3)
> "Message of the error" doesn't sound right. I suggest just saying "The
> error message".

Fixed.

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

Okay, fixed.

>
>
> 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

Fixed.

>
> (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.
>

Fixed.

>
> (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

Fixed.

>
> (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.

It's better to use strcmp() in this case. Fixed.

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

Fixed.

I've attached updated patches.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/