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

Hello,

On Thu, Mar 19, 2020 at 1:47 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> On 2020/03/19 1:22, Magnus Hagander wrote:
> >>>>> 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.
> >
> > :)
> >
> > Hmm. I'm slightly irked by doing the -1 -> NULL conversion in the SQL
> > view. I wonder if it might be worth teaching
> > pg_stat_get_progress_info() about returning NULL?
>
> That's possible by
> - adding the boolean array like st_progress_null[PGSTAT_NUM_PROGRESS_PARAM]
> that indicates whether each column is NULL or not, into PgBackendStatus
> - extending pgstat_progress_update_param() and pgstat_progress_update_multi_param()
> so that they can update the boolean array for NULL
> - updating the progress reporting code so that the extended versions of
> function are used
>
> I didn't adopt this idea because it looks a bit overkill for the purpose.

I tend to agree that this would be too many changes for something only
one place currently needs to use.

> OTOH, this would be good improvement for the progress reporting
> infrastructure and I'm fine to implement it.

Magnus' idea of checking the values in pg_stat_get_progress_info() to
determine whether to return NULL seems fine to me. 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.

--
Thank you,
Amit