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(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> From Thur, Sep 9, 2021 10:33 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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/