Re: Wait event that should be reported while waiting for WALarchiving to finish

On 2020/02/13 16:30, Michael Paquier wrote:
> On Thu, Feb 13, 2020 at 03:35:50PM +0900, Fujii Masao wrote:
>> I found that the wait events "LogicalRewriteTruncate" and
>> "GSSOpenServer" are not documented. I'm thinking to add
>> them into doc separately if ok.
> 
> Nice catch.  The ordering of the entries is not respected either for
> GSSOpenServer in pgstat.h.  The portion for the code and the docs can
> be fixed in back-branches, but not the enum list in WaitEventClient or
> we would have an ABI breakage.  But this can be fixed on HEAD.  Can
> you take care of it?
Yes. Patch attached.

logical_rewrite_truncate_v1.patch adds the description of
LogicalRewriteTruncate into the doc. This needs to be
back-patched to v10 where commit 249cf070e3 introduced
LogicalRewriteTruncate event.

gss_open_server_v1.patch adds the description of GSSOpenServer
into the doc and update the code in pgstat_get_wait_client().
This needs to be applied in v12 where commit b0b39f72b9 introduced
GSSOpenServer event.

gss_open_server_for_master_v1.patch does not only what the above
patch does but also update wait event enum into alphabetical order.
This needs to be applied in the master.

> 
>>            <entry><literal>SyncRep</literal></entry>
>>            <entry>Waiting for confirmation from remote server during synchronous replication.</entry>
>>           </row>
>> +        <row>
>> +         <entry><literal>BackupWaitWalArchive</literal></entry>
>> +         <entry>Waiting for WAL files required for the backup to be successfully archived.</entry>
>> +        </row>
> 
> The category IPC is adapted.  You forgot to update the markup morerows
> from "36" to "37", causing the table of the wait events to have a
> weird format (the bottom should be incorrect).

Fixed. Thanks for the review!

> 
>> +        pgstat_report_wait_start(WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE);
>>           while (XLogArchiveIsBusy(lastxlogfilename) ||
>>                  XLogArchiveIsBusy(histfilename))
>>           {
>> @@ -11120,6 +11121,7 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
>>                                    "but the database backup will not be usable without all the WAL segments.")));
>>               }
>>           }
>> +        pgstat_report_wait_end();
> 
> Okay, that position is right.
> 
>> @@ -3848,6 +3848,9 @@ pgstat_get_wait_ipc(WaitEventIPC w)
>>           case WAIT_EVENT_SYNC_REP:
>>               event_name = "SyncRep";
>>               break;
>> +        case WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE:
>> +            event_name = "BackupWaitWalArchive";
>> +            break;
>>               /* no default case, so that compiler will warn */
>> [...]
>> @@ -853,7 +853,8 @@ typedef enum
>>       WAIT_EVENT_REPLICATION_ORIGIN_DROP,
>>       WAIT_EVENT_REPLICATION_SLOT_DROP,
>>       WAIT_EVENT_SAFE_SNAPSHOT,
>> -    WAIT_EVENT_SYNC_REP
>> +    WAIT_EVENT_SYNC_REP,
>> +    WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE
>>   } WaitEventIPC;
> 
> It would be good to keep entries in alphabetical order in the header,
> the code and in the docs (see the effort from 5ef037c), and your patch
> is missing that concept for all three places where it matters for this
> new event.

Fixed. Patch attached.

Regards,

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