On Thu, Mar 19, 2020 at 11:45 AM Fujii Masao <masao.fu...@oss.nttdata.com> wrote: > On 2020/03/19 11:32, Amit Langote wrote: > > On Thu, Mar 19, 2020 at 11:24 AM Alvaro Herrera > > <alvhe...@2ndquadrant.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
