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