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

On 2020/03/19 1:13, Fujii Masao wrote:
> 
> 
> 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!

Pushed! Thanks!

Regards,

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