Re: Replication slot stats misgivings

On Mon, Apr 12, 2021 at 4:34 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Apr 12, 2021 at 6:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Apr 12, 2021 at 10:27 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Sat, Apr 10, 2021 at 9:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > >
> > > > It seems Vignesh has changed patches based on the latest set of
> > > > comments so you might want to rebase.
> > >
> > > I've merged my patch into the v6 patch set Vignesh submitted.
> > >
> > > I've attached the updated version of the patches. I didn't change
> > > anything in the patch that changes char[NAMEDATALEN] to NameData (0001
> > > patch) and patches that add tests.
> > >
> >
> > I think we can push 0001. What do you think?
>
> +1
>
> >
> > > In 0003 patch I reordered the
> > > output parameters of pg_stat_replication_slots; showing total number
> > > of transactions and total bytes followed by statistics for spilled and
> > > streamed transactions seems appropriate to me.
> > >
> >
> > I am not sure about this because I think we might want to add some
> > info of stream/spill bytes in total_bytes description (something like
> > stream/spill bytes are not in addition to total_bytes).
>
> Okay.
>
> > So probably
> > keeping these new counters at the end makes more sense to me.
>
> But I think all of those counters are new for users since
> pg_stat_replication_slots view will be introduced to PG14, no?
>
> >
> > > Since my patch resolved
> > > the issue of writing stats beyond the end of the array, I've removed
> > > the patch that writes the number of stats into the stats file
> > > (v6-0004-Handle-overwriting-of-replication-slot-statistic-.patch).
> > >
> >
> > Okay, but I think it might be better to keep 0001, 0002, 0003 as
> > Vignesh had because those are agreed upon changes and are
> > straightforward. We can push those and then further review HTAB
> > implementation and also see if Andres has any suggestions on the same.
>
> Makes sense. Maybe it should have written my patch as 0004 (i.g.,
> applied on top of the patch that adds total_txn and tota_bytes).
>
> >
> > > Apart from the above updates, the
> > > contrib/test_decoding/001_repl_stats.pl add wait_for_decode_stats()
> > > function during testing but I think we can use poll_query_until()
> > > instead.
> >
> > +1. Can you please change it in the next version?
>
> Sure, I'll update the patches.

I had started working on poll_query_until comment, I will test and
post a patch for that comment shortly.

Regards,
Vignesh