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

On 2020/03/19 0:37, Magnus Hagander wrote:
> On Wed, Mar 11, 2020 at 5:53 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>>
>>
>> On 2020/03/11 3:39, Magnus Hagander wrote:
>>> 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?
> 
> Did you miss this comment, or not agree? :)

Oh, I forgot to attached the patch... Patch attached.
This patch needs to be applied after applying
add_no_estimate_size_v3.patch.

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

Thanks for the review!

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters