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

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. :)

--
Thank you,
Amit