On Tue, Sep 28, 2021 at 1:54 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Sep 28, 2021 at 7:25 AM osumi.takami...@fujitsu.com > <osumi.takami...@fujitsu.com> wrote: > > > > On Monday, September 27, 2021 1:42 PM Amit Kapila <amit.kapil...@gmail.com> > > wrote: > > > On Wed, Sep 22, 2021 at 10:10 AM osumi.takami...@fujitsu.com > > > <osumi.takami...@fujitsu.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/
