Date: Sun, 6 Jun 2021 21:13:48 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: "Bossart, Nathan" <bossartn@amazon.com>,
	Fujii Masao <masao.fujii@oss.nttdata.com>, k.jamison@fujitsu.com,
	Thomas Munro <thomas.munro@gmail.com>, pgsql-hackers@postgresql.org,
	Nitin Jadhav <nitinjadhavpostgres@gmail.com>
Subject: Re: please update ps display for recovery checkpoint
Message-ID: <20210607021348.GA14099@telsasoft.com>
References: <549f04f1-6362-b599-c5e6-87330e36f03d@oss.nttdata.com>
 <X9Brc31Jj5cemRUw@paquier.xyz>
 <B8C46DD1-557F-4D20-97DD-DBC16772457F@amazon.com>
 <X9Ltbgcqy18Xvizq@paquier.xyz>
 <20201211040210.GZ24052@telsasoft.com>
 <X9Ly3zWGtaIfaRYT@paquier.xyz>
 <59A309AA-87C4-457E-A9D4-922BC5A418BA@amazon.com>
 <X9QH9ztwiVt1Lk/N@paquier.xyz>
 <B08B7934-A7DA-4A4E-BBFC-3505667F2B7E@amazon.com>
 <X9bVjWjCb5yxgDwN@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="UHN/qo2QbUvPLonB"
Content-Disposition: inline
In-Reply-To: <X9bVjWjCb5yxgDwN@paquier.xyz>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20210607021348.GA14099%40telsasoft.com>
Precedence: bulk


--UHN/qo2QbUvPLonB
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Dec 14, 2020 at 12:01:33PM +0900, Michael Paquier wrote:
> On Sat, Dec 12, 2020 at 12:41:25AM +0000, Bossart, Nathan wrote:
> > On 12/11/20, 4:00 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> >> My counter-proposal is like the attached, with the set/reset part not
> >> reversed this time, and the code indented :p
> > 
> > Haha.  LGTM.
> 
> Thanks.  I have applied this one, then.

To refresh: commit df9274adf adds "checkpoint" info to "ps", which previously
continued to say "recovering NNNNN" even after finishing WAL replay, and
throughout the checkpoint.

Now, I wonder whether the startup process should also include some detail about
"syncing data dir".  It's possible to strace the process to see what it's
doing, but most DBA would probably not know that, and it's helpful to know the
status of recovery and what part of recovery is slow: sync, replay, or
checkpoint.  commit df9274adf improved the situation between replay and
ckpoint, but it's still not clear what "postgres: startup" means before replay
starts.

There's some interaction between Thomas' commit 61752afb2 and
recovery_init_sync_method - if we include a startup message, it should
distinguish between "syncing data dirs (fsync)" and (syncfs).

Putting this into fd.c seems to assume that we can clobber "ps", which is fine
when called by StartupXLOG(), but it's a public interface, so I'm not sure if
it's okay to assume that's the only caller.  Maybe it should check if
MyAuxProcType == B_STARTUP.

-- 
Justin

--UHN/qo2QbUvPLonB
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-Show-syncing-data-directories-in-ps-display.patch"

From a143c40fb2bad96d45f5cc3e22f70dd0e2d6b5c6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 6 Jun 2021 16:28:15 -0500
Subject: [PATCH] Show "syncing data directories" in ps display..

See also: df9274adf, which shows checkpoint output in ps, and 61752afb2 which
adds syncfs.
---
 src/backend/storage/file/fd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index e8cd7ef088..692d7f16b7 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -99,6 +99,7 @@
 #include "storage/fd.h"
 #include "storage/ipc.h"
 #include "utils/guc.h"
+#include "utils/ps_status.h"
 #include "utils/resowner_private.h"
 
 /* Define PG_FLUSH_DATA_WORKS if we have an implementation for pg_flush_data */
@@ -3374,6 +3375,8 @@ SyncDataDirectory(void)
 		 * directories.
 		 */
 
+		set_ps_display("syncing data directory (syncfs)");
+
 		/* Sync the top level pgdata directory. */
 		do_syncfs(".");
 		/* If any tablespaces are configured, sync each of those. */
@@ -3392,10 +3395,13 @@ SyncDataDirectory(void)
 		/* If pg_wal is a symlink, process that too. */
 		if (xlog_is_symlink)
 			do_syncfs("pg_wal");
+
+		set_ps_display("");
 		return;
 	}
 #endif							/* !HAVE_SYNCFS */
 
+	set_ps_display("syncing data directory (fsync)");
 	/*
 	 * If possible, hint to the kernel that we're soon going to fsync the data
 	 * directory and its contents.  Errors in this step are even less
@@ -3421,6 +3427,8 @@ SyncDataDirectory(void)
 	if (xlog_is_symlink)
 		walkdir("pg_wal", datadir_fsync_fname, false, LOG);
 	walkdir("pg_tblspc", datadir_fsync_fname, true, LOG);
+
+	set_ps_display("");
 }
 
 /*
-- 
2.17.0


--UHN/qo2QbUvPLonB--