Re: Failed transaction statistics to measure the logical replication progress

On Tue, Sep 28, 2021 at 1:54 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Sep 28, 2021 at 7:25 AM osumi(dot)takamichi(at)fujitsu(dot)com
> <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> >
> > On Monday, September 27, 2021 1:42 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > On Wed, Sep 22, 2021 at 10:10 AM osumi(dot)takamichi(at)fujitsu(dot)com
> > > <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> > > >
> > > > Just conducted some cosmetic changes
> > > > and rebased my patch, using v14 patch-set in [1].
> > > >
> > >
> > > IIUC, this proposal will allow new xact stats for subscriptions via
> > > pg_stat_subscription. One thing that is not clear to me in this patch is that why
> > > you choose a different way to store these stats than the existing stats in that
> > > view? AFAICS, the other existing stats are stored in-memory in
> > > LogicalRepWorker whereas these new stats are stored/fetched via stats
> > > collector means these will persist. Isn't it better to be consistent here? I am not
> > > sure which is a more appropriate way to store these stats and I would like to
> > > hear your and other's thoughts on that matter but it appears a bit awkward to
> > > me that some of the stats in the same view are persistent and others are
> > > in-memory.
> > Yeah, existing stats values of pg_stat_subscription are in-memory.
> > I thought xact stats should survive over the restart,
> > to summarize and show all accumulative transaction values
> > on one subscription for user. But, your pointing out is reasonable,
> > mixing two types can be awkward and lack of consistency.

I remembered that we have discussed a similar thing when discussing
pg_stat_replication_slots view[1]. The slot statistics such as
spill_txn, spill_count, and spill_bytes were originally shown in
pg_stat_replication, mixing cumulative counters and dynamic counters.
But we concluded to separate these cumulative values from
pg_stat_replication view and introduce a new pg_stat_replication_slots
view.

> >
> > Then, if, we proceed in this direction,
> > the place to implement those stats
> > would be on the LogicalRepWorker struct, instead ?
> >
>
> Or, we can make existing stats persistent and then add these stats on
> top of it. Sawada-San, do you have any thoughts on this matter?

I think that making existing stats including received_lsn and
last_msg_receipt_time persistent by using stats collector could cause
massive reporting messages. We can report these messages with a
certain interval to reduce the amount of messages but we will end up
seeing old stats on the view. Another idea could be to have a separate
view, say pg_stat_subscription_xact but I'm not sure it's a better
idea.

>
> > On one hand, what confuses me is that
> > in another thread of feature to skip xid,
> > I wondered if Sawada-san has started to take
> > those xact stats into account (probably in his patch-set),
> > because the stats values this thread is taking care of are listed up in the thread.
> >
>
> I don't think the Skip Xid patch is going to take care of these
> additional stats but Sawada-San can confirm.

Yes, I don't take care of these additional stats discussed on this
thread. The reason why I recently mentioned these statistics on the
skip_xid thread is that since I thought that this patch will be built
on top of the subscription error reporting patch that I'm proposing I
discussed the extensibility of my patch.

Regards,

[1] https://www.postgresql.org/message-id/CABUevEwayASgA2GHAQx%3DVtCp6OQh5PVfem6JDQ97gFHka%3D6n1w%40mail.gmail.com

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