Re: Skipping logical replication transactions on subscriber side

Sorry for the late reply. I was on vacation.

On Tue, Sep 14, 2021 at 11:27 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> From Thur, Sep 9, 2021 10:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > Sorry for the late response. I've attached the updated patches that incorporate
> > all comments unless I missed something. Please review them.
>
> Thanks for the new version patches.
> Here are some comments for the v13-0001 patch.

Thank you for the comments!

>
> 1)
>
> +                                       pgstat_setheader(&errmsg.m_hdr, PGSTAT_MTYPE_SUBSCRIPTIONERRPURGE);
> +                                       pgstat_send(&errmsg, len);
> +                                       errmsg.m_nentries = 0;
> +                               }
>
> It seems we can invoke pgstat_setheader once before the loop like the
> following:
>
> +                       errmsg.m_nentries = 0;
> +                       errmsg.m_subid = subent->subid;
> +                       pgstat_setheader(&errmsg.m_hdr, PGSTAT_MTYPE_SUBSCRIPTIONERRPURGE);
>
> 2)
> +                                       pgstat_setheader(&submsg.m_hdr, PGSTAT_MTYPE_SUBSCRIPTIONPURGE);
> +                                       pgstat_send(&submsg, len);
>
> Same as 1), we can invoke pgstat_setheader once before the loop like:
> +               submsg.m_nentries = 0;
> +               pgstat_setheader(&submsg.m_hdr, PGSTAT_MTYPE_SUBSCRIPTIONPURGE);
>

But if we do that, we set the header even if there is no message to
send, right? Looking at other similar code in pgstat_vacuum_stat(), we
set the header just before sending the message. So I'd like to leave
them since it's cleaner.

>
> 3)
>
> +/* ----------
> + * PgStat_MsgSubscriptionErrPurge      Sent by the autovacuum to purge the subscription
> + *                                                                     errors.
>
> The comments said it's sent by autovacuum, would the manual vacuum also send
> this message ?

Right. Fixed.

>
>
> 4)
> +
> +       pgstat_send(&msg, offsetof(PgStat_MsgSubscriptionErr, m_reset) + sizeof(bool));
> +}
>
> Does it look cleaner that we use the offset of m_relid here like the following ?
>
> pgstat_send(&msg, offsetof(PgStat_MsgSubscriptionErr, m_relid));

Thank you for the suggestion. After more thought, it was a bit odd to
use PgStat_MsgSubscriptionErr to both report and reset the stats by
sending the part or the full struct. So in the latest version, I've
added a new message struct type to reset the subscription error
statistics.

I've attached the updated version patches. Please review them.

Regards,

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