Re: Skipping logical replication transactions on subscriber side

On Wed, Oct 13, 2021 at 10:59 AM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
> On Tue, Oct 12, 2021 at 4:00 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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/