Re: pg_stat_progress_basebackup - progress reporting for pg_basebackup, in the server side

On Wed, Mar 11, 2020 at 5:53 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>
>
>
> On 2020/03/11 3:39, Magnus Hagander wrote:
> > On Tue, Mar 10, 2020 at 6:19 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> >>
> >>
> >>
> >> On 2020/03/10 22:43, Amit Langote wrote:
> >>> On Tue, Mar 10, 2020 at 6:09 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> >>>>> So, I will make the patch adding support for --no-estimate-size option
> >>>>> in pg_basebackup.
> >>>>
> >>>> Patch attached.
> >>>
> >>> Like the idea and the patch looks mostly good.
> >>
> >> Thanks for reviewing the patch!
> >>
> >>> + total size. If the estimation is disabled in
> >>> + <application>pg_basebackup</application>
> >>> + (i.e., <literal>--no-estimate-size</literal> option is specified),
> >>> + this is always <literal>0</literal>.
> >>>
> >>> "always" seems unnecessary.
> >>
> >> Fixed.
> >>
> >>> + This option prevents the server from estimating the total
> >>> + amount of backup data that will be streamed. In other words,
> >>> + <literal>backup_total</literal> column in the
> >>> + <structname>pg_stat_progress_basebackup</structname>
> >>> + view always indicates <literal>0</literal> if this option is enabled.
> >>>
> >>> Here too.
> >>
> >> Fixed.
> >>
> >> Attached is the updated version of the patch.
> >
> > Would it perhaps be better to return NULL instead of 0 in the
> > statistics view if there is no data?

Did you miss this comment, or not agree? :)

> > Also, should it really be the server version that decides how this
> > feature behaves, and not the pg_basebackup version? Given that the
> > implementation is entirely in the client, it seems that's more
> > logical?
>
> Yeah, you're right. I changed the patch that way.
> Attached is the updated version of the patch.

The other changes in it look good!

//Magnus