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.sawada@2ndquadrant.com] 
Sent: Monday, October 12, 2020 3:22 PM
To: Amit Kapila 
Cc: Dilip Kumar ; Magnus Hagander ; Tomas Vondra ; PostgreSQL Hackers ; Ajin Cherian 
Subject: Re: Resetting spilled txn statistics in pg_stat_replication

On Thu, 8 Oct 2020 at 22:57, Amit Kapila  wrote:
>
> On Thu, Oct 8, 2020 at 7:46 AM Masahiko Sawada 
>  wrote:
> >
> > On Wed, 7 Oct 2020 at 17:52, Amit Kapila  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