Re: Replication slot stats misgivings

On Mon, Apr 12, 2021 at 10:27 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Sat, Apr 10, 2021 at 9:53 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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?

> 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). So probably
keeping these new counters at the end makes more sense to me.

> 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.

> 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?

--
With Regards,
Amit Kapila.