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

On 2020/02/18 16:02, Amit Langote wrote:
> On Mon, Feb 17, 2020 at 10:00 PM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
>> On 2020/02/06 11:07, Amit Langote wrote:
>>> On Thu, Feb 6, 2020 at 9:51 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>>>> I thought of "establishing checkpoint" or "running a checkpoint" as
>>>> other candidates.
>>>
>>> Okay, I understand.  I am fine with "running checkpoint", although I
>>> think "waiting for checkpoint" isn't totally wrong either.
>>
>> Yeah, but if "waiting for XXX" sounds a bit confusing to some people,
>> I'm OK to back to "waiting for XXX to finish" that you originally
>> proposed.
>>
>> Attached the updated version of the patch. This patch uses the following
>> descriptions of the phases.
>>
>>     waiting for checkpoint to finish
>>     estimating backup size
>>     streaming database files
>>     waiting for wal archiving to finish
>>     transferring wal files
> 
> Thanks for the new patch.

Thanks for reviewing the patch!

> I noticed that there is missing </para> tag in the documentation changes:

Could you tell me where I should add </para> tag?

> +     <row>
> +      <entry><literal>waiting for checkpoint to finish</literal></entry>
> +      <entry>
> +       The WAL sender process is currently performing
> +       <function>pg_start_backup</function> to set up for
> +       taking a base backup, and waiting for backup start
> +       checkpoint to finish.
> +      </entry>
> +     <row>
> 
> There should be a </row> between </entry> and <row> at the end of the
> hunk shown above.

Will fix. Thanks!

> Sorry for not saying it before, but the following text needs revisiting:

Of course, no problem. I'm happy to improve the patch!

> +  <para>
> +   Whenever <application>pg_basebackup</application> is taking a base
> +   backup, the <structname>pg_stat_progress_basebackup</structname>
> +   view will contain a row for each WAL sender process that is currently
> +   running <command>BASE_BACKUP</command> replication command
> +   and streaming the backup.
> 
> I understand that you wrote "Whenever pg_basebackup is taking a
> backup...", because description of other views contains a similar
> starting line.  But, it may not only be pg_basebackup that would be
> served by this view, no?  It could be any tool that speaks Postgres'
> replication protocol and thus be able to send a BASE_BACKUP command.
> If that is correct, I would write something like "When an application
> is taking a backup" or some such without specific reference to
> pg_basebackup.  Thoughts?

Yeah, there may be some such applications. But most users would
use pg_basebackup, so getting rid of the reference to pg_basebackup
would make the description a bit difficult-to-read. Also I can imagine
that an user of those backup applications would get to know
the progress reporting view from their documents. So I prefer
the existing one or something like "Whenever an application like
  pg_basebackup ...". Thought?

Regards,

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