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

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?  If you need help, please feel free to poke me. I
think that this should be fixed first, before adding the new event.

>           <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).

> +		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.
--
Michael