From: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
---|---|
To: | Magnus Hagander <magnus(at)hagander(dot)net> |
Cc: | Amit Langote <amitlangote09(at)gmail(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, "Shinoda, Noriyoshi (PN Japan A&PS Delivery)" <noriyoshi(dot)shinoda(at)hpe(dot)com>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: pg_stat_progress_basebackup - progress reporting for pg_basebackup, in the server side |
Date: | 2020-03-11 04:53:47 |
Message-ID: | e4d5215f-eeaa-e7f4-81c2-eec788e8aebe@oss.nttdata.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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?
>
> 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.
> 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.
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 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)
Yes, fixed.
> + 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..."
Fixed. I used "Without using this option ...".
> + <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.
Fixed.
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
Attachment | Content-Type | Size |
---|---|---|
add_no_estimate_size_v3.patch | text/plain | 5.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Masahiko Sawada | 2020-03-11 05:42:34 | Re: Do we need to handle orphaned prepared transactions in the server? |
Previous Message | Fujii Masao | 2020-03-11 04:51:47 | Re: pg_stat_progress_basebackup - progress reporting for pg_basebackup, in the server side |