RE: Resetting spilled txn statistics in pg_stat_replication

Hi, thank you for the awesome feature.

As it may have been discussed, I think the 'name' column in pg_stat_replication_slots is more consistent with the column name and data type matched to the pg_replication_slots catalog.
The attached patch changes the name and data type of the 'name' column to slot_name and 'name' type, respectively.
Also, the macro name PG_STAT_GET_..._CLOS has been changed to PG_STAT_GET_..._COLS.

Regards,
Noriyoshi Shinoda
-----Original Message-----
From: Masahiko Sawada [mailto:masahiko(dot)sawada(at)2ndquadrant(dot)com]
Sent: Monday, October 12, 2020 3:22 PM
To: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Cc: Dilip Kumar <dilipbalaut(at)gmail(dot)com>; Magnus Hagander <magnus(at)hagander(dot)net>; Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>; PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>; Ajin Cherian <itsajin(at)gmail(dot)com>
Subject: Re: Resetting spilled txn statistics in pg_stat_replication

On Thu, 8 Oct 2020 at 22:57, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Oct 8, 2020 at 7:46 AM Masahiko Sawada
> <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
> >
> > On Wed, 7 Oct 2020 at 17:52, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> >
> > > I think after we are done with this the next step would be to
> > > finish the streaming stats work [1]. We probably need to review
> > > and add the test case in that patch. If nobody else shows up I
> > > will pick it up and complete it.
> >
> > +1
> > I can review that patch.
> >
>
> I have rebased the stream stats patch and made minor modifications. I
> haven't done a detailed review but one thing that I think is not
> correct is:
> @@ -3496,10 +3499,18 @@ ReorderBufferStreamTXN(ReorderBuffer *rb,
> ReorderBufferTXN *txn)
> txn->snapshot_now = NULL;
> }
>
> +
> + rb->streamCount += 1;
> + rb->streamBytes += txn->total_size;
> +
> + /* Don't consider already streamed transaction. */
> + rb->streamTxns += (rbtxn_is_streamed(txn)) ? 0 : 1;
> +
> /* Process and send the changes to output plugin. */
> ReorderBufferProcessTXN(rb, txn, InvalidXLogRecPtr, snapshot_now,
> command_id, true);
>
> I think we should update the stream stats after
> ReorderBufferProcessTXN rather than before because any error in
> ReorderBufferProcessTXN can lead to an unnecessary update of stats.
> But OTOH, the txn flags, and other data can be changed after
> ReorderBufferProcessTXN so we need to save them in a temporary
> variable before calling the function.

Thank you for updating the patch!

I've not looked at the patch in-depth yet but RBTXN_IS_STREAMED could be cleared after ReorderBUfferProcessTXN()?

BTW maybe it's better to start a new thread for this patch as the title is no longer relevant.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services