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

On 2020/03/19 17:22, Fujii Masao wrote:
>
>
> On 2020/03/19 12:02, Amit Langote wrote:
>> On Thu, Mar 19, 2020 at 11:45 AM Fujii Masao
>> <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>> On 2020/03/19 11:32, Amit Langote wrote:
>>>> On Thu, Mar 19, 2020 at 11:24 AM Alvaro Herrera
>>>> <alvherre(at)2ndquadrant(dot)com> wrote:
>>>>> On 2020-Mar-19, Amit Langote wrote:
>>>>>
>>>>>> Magnus' idea of checking the values in pg_stat_get_progress_info() to
>>>>>> determine whether to return NULL seems fine to me.
>>>
>>> So you think that the latest patch is good enough?
>>
>> I see that the latest patch modifies pg_stat_progress_basebackup view
>> to return NULL, so not exactly.  IIUC, Magnus seems to be advocating
>> to *centralize* this in pg_stat_get_progress_info(), which all views
>> are based on, which means we need to globally define a NULL param
>> value, as Alvaro also pointed out.
>>
>> But...
>>
>>>>>>   We will need to
>>>>>> update the documentation of st_progress_param, because it currently
>>>>>> says:
>>>>>>
>>>>>>        *  ...but the meaning of each element in the
>>>>>>        * st_progress_param array is command-specific.
>>>>>>        */
>>>>>>       ProgressCommandType st_progress_command;
>>>>>>       Oid         st_progress_command_target;
>>>>>>       int64       st_progress_param[PGSTAT_NUM_PROGRESS_PARAM];
>>>>>> } PgBackendStatus;
>>>>>>
>>>>>> If we are to define -1 in st_progress_param[] as NULL to the users,
>>>>>> that must be mentioned here.
>>>>>
>>>>> Hmm, why -1?  It seems like a value that we might want to use for other
>>>>> purposes in other params.  Maybe INT64_MIN is a better choice?
>>>>
>>>> Yes, maybe.
>>>
>>> I don't think that we need to define the specific value like -1 as NULL globally.
>>> Which value should be used for that purpose may vary by each command. Only for
>>> pg_stat_progress_basebackup.backup_total, IMO using -1 as special value for
>>> NULL is not so bad idea.
>>
>> This is the first instance of needing to display NULL in a progress
>> view, so a non-general solution may be enough for now.  IOW, your
>> latest patch is good enough for that. :)
>
> Ok, so barring any objection, I will commit the latest patch.

Pushed! Thanks all!

Regards,

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