Re: pg_stat_progress_basebackup - progress reporting forpg_basebackup, in the server side - Mailing list pgsql-hackers

On Tue, Mar 10, 2020 at 6:19 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2020/03/10 22:43, Amit Langote wrote:
> > On Tue, Mar 10, 2020 at 6:09 PM Fujii Masao <masao.fujii@oss.nttdata.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/