Re: pg_stat_progress_basebackup - progress reporting forpg_basebackup, in the server side

On 2020/02/03 16:28, Amit Langote wrote:
> On Mon, Feb 3, 2020 at 1:17 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> On 2020/02/02 14:59, Masahiko Sawada wrote:
>>> On Fri, 31 Jan 2020 at 02:29, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>> On 2020/01/30 12:58, Kyotaro Horiguchi wrote:
>>>>> +            WHEN 3 THEN 'stopping backup'::text
>>>>>
>>>>> I'm not sure, but the "stop" seems suggesting the backup is terminated
>>>>> before completion. If it is following the name of the function
>>>>> pg_stop_backup, I think the name is suggesting to stop "the state for
>>>>> performing backup", not a backup.
>>>>>
>>>>> In the first place, the progress is about "backup" so it seems strange
>>>>> that we have another phase after the "stopping backup" phase.  It
>>>>> might be better that it is "finishing file transfer" or such.
>>>>>
>>>>>       "initializing"
>>>>> -> "starting file transfer"
>>>>> -> "transferring files"
>>>>> -> "finishing file transfer"
>>>>> -> "transaferring WAL"
>>>>
>>>> Better name is always welcome! If "stopping back" is confusing,
>>>> what about "performing pg_stop_backup"? So
>>>>
>>>> initializing
>>>> performing pg_start_backup
>>>> streaming database files
>>>> performing pg_stop_backup
>>>> transfering WAL files
>>>
>>> Another idea I came up with is to show steps that take time instead of
>>> pg_start_backup/pg_stop_backup, for better understanding the
>>> situation. That is, "performing checkpoint" and "performing WAL
>>> archive" etc, which engage the most of the time of these functions.
>>
>> Yeah, that's an idea. ISTM that "waiting for WAL archiving" sounds
>> better than "performing WAL archive". Thought?
>> I've not applied this change in the patch yet, but if there is no
>> other idea, I'd like to adopt this.
> 
> If we are trying to "pg_stop_backup" in phase name, maybe we should
> avoid "pg_start_backup"?  Then maybe:
> 
> initializing
> starting backup / waiting for [ backup start ] checkpoint to finish
> transferring database files
> finishing backup / waiting for WAL archiving to finish
> transferring WAL files

So we now have the following ideas about the phase names for pg_basebackup.

1.
initializing

2.
2-1. starting backup
2-2. starting file transfer
2-3. performing pg_start_backup
2-4. performing checkpoint
2-5. waiting for [ backup start ] checkpoint to finish

3.
3-1. streaming backup
3-2. transferring database files
3-3. streaming database files
3-4. transferring files

4.
4-1. stopping backup
4-2. finishing file transfer
4-3. performing pg_stop_backup
4-4. finishing backup
4-5. waiting for WAL archiving to finish
4-6. performing WAL archive

5.
1. transferring wal
2. transferring WAL files

What conbination of these do you prefer?
> Some comments on documentation changes in v2 patch:
> 
> +      Amount of data already streamed.

Ok, fixed.

> "already" may be redundant.  For example, in pg_start_progress_vacuum,
> heap_blks_scanned is described as "...blocks scanned", not "...blocks
> already scanned".
> 
> +     <entry><structfield>tablespace_total</structfield></entry>
> +     <entry><structfield>tablespace_streamed</structfield></entry>
> 
> Better to use plural tablespaces_total and tablespaces_streamed for consistency?

Fixed.

> +       The WAL sender process is currently performing
> +       <function>pg_start_backup</function> and setting up for
> +       making a base backup.
> 
> How about "taking" instead of "making" in the above sentence?

Fixed. Attached is the updated version of the patch.

> 
> -  <varlistentry>
> +  <varlistentry id="protocol-replication-base-backup" xreflabel="BASE_BACKUP">
> 
> I don't see any new text in the documentation patch that uses above
> xref, so no need to define it?

The following description that I added uses this.

     certain commands during command execution.  Currently, the only commands
     which support progress reporting are <command>ANALYZE</command>,
     <command>CLUSTER</command>,
-   <command>CREATE INDEX</command>, and <command>VACUUM</command>.
+   <command>CREATE INDEX</command>, <command>VACUUM</command>,
+   and <xref linkend="protocol-replication-base-backup"/> (i.e., replication
+   command that <xref linkend="app-pgbasebackup"/> issues to take
+   a base backup).

Regards,


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