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

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?

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?

and a few docs nitpicks:

<para>
Whether this is enabled or not, the
<structname>pg_stat_progress_basebackup</structname> view
- report the progress of the backup in the server side. But note
- that the total amount of data that will be streamed is estimated
- and reported only when this option is enabled. In other words,
- <literal>backup_total</literal> column in the view always
- indicates <literal>0</literal> if this option is disabled.
+ report the progress of the backup in the server side.
+ </para>
+ <para>
+ This option is not allowed when using
+ <option>--no-estimate-size</option>.
</para>

I think you should just remove that whole paragraph. The details are
now listed under the disable parameter.

+ 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 indicates <literal>0</literal> if this option is enabled.

I suggest just "This option prevents the server from estimating the
total amount of backup data that will be streamed, resulting in the
ackup_total column in pg_stat_progress_basebackup to be (zero or NULL
depending on above)".

(Markup needed on that of course ,but you get the idea)

+ When this is disabled, the backup will start by enumerating

I'd try to avoid the double negation, with something "without this
parameter, the backup will start..."

+ <para>
+ <application>pg_basebackup</application> asks the server to estimate
+ the total amount of data that will be streamed by default (unless
+ <option>--no-estimate-size</option> is specified) in version 13 or later,
+ and does that only when <option>--progress</option> is specified in
+ the older versions.
+ </para>

That's an item for the release notes, not for the reference page, I
think. It's already explained under the --disable parameter, so I
suggest removing this paragraph as well.

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/