Slightly improve initdb --sync-only option's help message
When reading the output of `initdb --help` I could not clearly
understand what the purpose of the --sync-only option was, until I
read the documentation of initdb.
-S, --sync-only only sync data directory
Perhaps the confusion was caused by the fact that sync(hronization)
means different things in different contexts, and many of those
contexts apply to databases, and to data directories; time sync, data
sync, replica sync, etc.
I think it would be helpful if the help message was slightly more
descriptive. Some options:
Used in patch:
only sync data directory; does not modify any data
To match the wording of --sync-only option:
write contents of data directory to disk; helpful after --no-sync option
Clearly specify the system operation used for the option
perform fsync on data directory; helpful after --no-sync option
Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/
Attachments:
v1-0001-Explicitly-state-that-sync-only-does-not-modify-d.patchapplication/octet-stream; name=v1-0001-Explicitly-state-that-sync-only-does-not-modify-d.patchDownload
From 53cf677bfcc520e824295fb06231e545cf21cd6c Mon Sep 17 00:00:00 2001
From: Gurjeet Singh <gurjeet@singh.im>
Date: Wed, 7 Jul 2021 01:33:28 +0000
Subject: [PATCH v1 1/2] Explicitly state that --sync-only does not modify data
---
src/bin/initdb/initdb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 0945d70061..05a98b9ade 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2273,7 +2273,7 @@ usage(const char *progname)
printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
printf(_(" --no-instructions do not print instructions for next steps\n"));
printf(_(" -s, --show show internal settings\n"));
- printf(_(" -S, --sync-only only sync data directory\n"));
+ printf(_(" -S, --sync-only only sync data directory; does not modify any data\n"));
printf(_("\nOther options:\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
--
2.30.0
On 7/6/21, 7:02 PM, "Gurjeet Singh" <gurjeet@singh.im> wrote:
I think it would be helpful if the help message was slightly more
descriptive. Some options:Used in patch:
only sync data directory; does not modify any dataTo match the wording of --sync-only option:
write contents of data directory to disk; helpful after --no-sync optionClearly specify the system operation used for the option
perform fsync on data directory; helpful after --no-sync option
I think the help message should say exactly what the option does and
should avoid saying what it does not do or how it may be useful. I
would suggest the following to match the initdb docs [0]https://www.postgresql.org/docs/devel/app-initdb.html:
-S, --sync-only safely write all database files to disk and exit
IMO the note about the option being helpful after using the --no-sync
option would fit better in the docs, but I'm struggling to think of a
use case for using --no-sync and then calling initdb again with
--sync-only. Why wouldn't you just leave out --no-sync the first
time?
Nathan
On Thu, Jul 22, 2021 at 10:32:18PM +0000, Bossart, Nathan wrote:
On 7/6/21, 7:02 PM, "Gurjeet Singh" <gurjeet@singh.im> wrote:
I think it would be helpful if the help message was slightly more
descriptive. Some options:Used in patch:
only sync data directory; does not modify any dataTo match the wording of --sync-only option:
write contents of data directory to disk; helpful after --no-sync optionClearly specify the system operation used for the option
perform fsync on data directory; helpful after --no-sync optionI think the help message should say exactly what the option does and
should avoid saying what it does not do or how it may be useful. I
would suggest the following to match the initdb docs [0]:-S, --sync-only safely write all database files to disk and exit
IMO the note about the option being helpful after using the --no-sync
option would fit better in the docs, but I'm struggling to think of a
use case for using --no-sync and then calling initdb again with
--sync-only. Why wouldn't you just leave out --no-sync the first
time?
It's to allow safely running bulk loading with fsync=off - if the bulk load
fails, you can wipe out the partially-loaded cluster and start over.
But then transitioning to a durable state requires not just setting fsync=on,
which enables future fsync calls. It also requires syncing all dirty buffers.
doc/src/sgml/config.sgml- <para>
doc/src/sgml/config.sgml- For reliable recovery when changing <varname>fsync</varname>
doc/src/sgml/config.sgml- off to on, it is necessary to force all modified buffers in the
doc/src/sgml/config.sgml- kernel to durable storage. This can be done while the cluster
doc/src/sgml/config.sgml- is shutdown or while <varname>fsync</varname> is on by running <command>initdb
doc/src/sgml/config.sgml: --sync-only</command>, running <command>sync</command>, unmounting the
doc/src/sgml/config.sgml- file system, or rebooting the server.
doc/src/sgml/config.sgml- </para>
--
Justin
On 7/22/21, 6:31 PM, "Justin Pryzby" <pryzby@telsasoft.com> wrote:
On Thu, Jul 22, 2021 at 10:32:18PM +0000, Bossart, Nathan wrote:
IMO the note about the option being helpful after using the --no-sync
option would fit better in the docs, but I'm struggling to think of a
use case for using --no-sync and then calling initdb again with
--sync-only. Why wouldn't you just leave out --no-sync the first
time?It's to allow safely running bulk loading with fsync=off - if the bulk load
fails, you can wipe out the partially-loaded cluster and start over.
But then transitioning to a durable state requires not just setting fsync=on,
which enables future fsync calls. It also requires syncing all dirty buffers.
Right. Perhaps the documentation for --sync-only could mention this
use-case instead.
Safely write all database files to disk and exit. This does
not perform any of the normal initdb operations. Generally,
this option is useful for ensuring reliable recovery after
changing fsync from off to on.
Nathan
On 7/23/21, 9:09 AM, "Bossart, Nathan" <bossartn@amazon.com> wrote:
On 7/22/21, 6:31 PM, "Justin Pryzby" <pryzby@telsasoft.com> wrote:
On Thu, Jul 22, 2021 at 10:32:18PM +0000, Bossart, Nathan wrote:
IMO the note about the option being helpful after using the --no-sync
option would fit better in the docs, but I'm struggling to think of a
use case for using --no-sync and then calling initdb again with
--sync-only. Why wouldn't you just leave out --no-sync the first
time?It's to allow safely running bulk loading with fsync=off - if the bulk load
fails, you can wipe out the partially-loaded cluster and start over.
But then transitioning to a durable state requires not just setting fsync=on,
which enables future fsync calls. It also requires syncing all dirty buffers.Right. Perhaps the documentation for --sync-only could mention this
use-case instead.Safely write all database files to disk and exit. This does
not perform any of the normal initdb operations. Generally,
this option is useful for ensuring reliable recovery after
changing fsync from off to on.
Here are my suggestions in patch form.
Nathan
Attachments:
v2-0001-improve-initdb-sync-only-help-message-and-docs.patchapplication/octet-stream; name=v2-0001-improve-initdb-sync-only-help-message-and-docs.patchDownload
From 508437cd9750eca15c3deb35af1d48559b77ab31 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Mon, 26 Jul 2021 18:01:44 +0000
Subject: [PATCH v2 1/1] improve initdb --sync-only help message and docs
---
doc/src/sgml/ref/initdb.sgml | 3 +++
src/bin/initdb/initdb.c | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
index e62742850a..3d88838712 100644
--- a/doc/src/sgml/ref/initdb.sgml
+++ b/doc/src/sgml/ref/initdb.sgml
@@ -306,6 +306,9 @@ PostgreSQL documentation
<para>
Safely write all database files to disk and exit. This does not
perform any of the normal <application>initdb</application> operations.
+ Generally, this option is useful for ensuring reliable recovery after
+ changing <xref linkend="guc-fsync"/> from <literal>off</literal> to
+ <literal>on</literal>.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 994bf07f3b..56528c8221 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2200,7 +2200,7 @@ usage(const char *progname)
printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
printf(_(" --no-instructions do not print instructions for next steps\n"));
printf(_(" -s, --show show internal settings\n"));
- printf(_(" -S, --sync-only only sync data directory\n"));
+ printf(_(" -S, --sync-only safely write all database files to disk and exit\n"));
printf(_("\nOther options:\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
--
2.16.6
On Mon, Jul 26, 2021 at 11:05 AM Bossart, Nathan <bossartn@amazon.com> wrote:
Here are my suggestions in patch form.
+ printf(_(" -S, --sync-only safely write all database
files to disk and exit\n"));
Not your patch's fault, but the word "write" does not seem to convey
the true intent of the option, because generally a "write" operation
is still limited to dirtying the OS buffers, and does not guarantee
sync-to-disk.
It'd be better if the help message said, either "flush all database
files to disk and exit",or "sync all database files to disk and exit".
Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/
On 26 Jul 2021, at 20:05, Bossart, Nathan <bossartn@amazon.com> wrote:
Here are my suggestions in patch form.
- printf(_(" -S, --sync-only only sync data directory\n"));
+ printf(_(" -S, --sync-only safely write all database files to disk and exit\n"));I think removing the word "only" here is a net loss, since it IMO clearly
conveyed that this option isn't doing any of the other initdb duties. The
documentation states it in more words, but the help output must be brief so I
think "only" is a good keyword.
--
Daniel Gustafsson https://vmware.com/
On 7/29/21, 2:11 AM, "Daniel Gustafsson" <daniel@yesql.se> wrote:
I think removing the word "only" here is a net loss, since it IMO clearly
conveyed that this option isn't doing any of the other initdb duties. The
documentation states it in more words, but the help output must be brief so I
think "only" is a good keyword.
I've attached a new version of the patch in which I've attempted to
address both sets of feedback.
Nathan
Attachments:
v3-0001-improve-initdb-sync-only-help-message-and-docs.patchapplication/octet-stream; name=v3-0001-improve-initdb-sync-only-help-message-and-docs.patchDownload
From b85980c333b940273d8dd7f7f469f33c8cca2c79 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Thu, 29 Jul 2021 23:32:20 +0000
Subject: [PATCH v3 1/1] improve initdb --sync-only help message and docs
---
doc/src/sgml/ref/initdb.sgml | 3 +++
src/bin/initdb/initdb.c | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
index e62742850a..3d88838712 100644
--- a/doc/src/sgml/ref/initdb.sgml
+++ b/doc/src/sgml/ref/initdb.sgml
@@ -306,6 +306,9 @@ PostgreSQL documentation
<para>
Safely write all database files to disk and exit. This does not
perform any of the normal <application>initdb</application> operations.
+ Generally, this option is useful for ensuring reliable recovery after
+ changing <xref linkend="guc-fsync"/> from <literal>off</literal> to
+ <literal>on</literal>.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 994bf07f3b..28c6eac903 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2200,7 +2200,7 @@ usage(const char *progname)
printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
printf(_(" --no-instructions do not print instructions for next steps\n"));
printf(_(" -s, --show show internal settings\n"));
- printf(_(" -S, --sync-only only sync data directory\n"));
+ printf(_(" -S, --sync-only only sync database files to disk and exit\n"));
printf(_("\nOther options:\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
--
2.16.6
On 30 Jul 2021, at 01:37, Bossart, Nathan <bossartn@amazon.com> wrote:
On 7/29/21, 2:11 AM, "Daniel Gustafsson" <daniel@yesql.se> wrote:
I think removing the word "only" here is a net loss, since it IMO clearly
conveyed that this option isn't doing any of the other initdb duties. The
documentation states it in more words, but the help output must be brief so I
think "only" is a good keyword.I've attached a new version of the patch in which I've attempted to
address both sets of feedback.
LGTM. I took the liberty to rephrase the "and exit" part of the initdb help
output match the other exiting options in there. Barring objections, I think
this is ready.
--
Daniel Gustafsson https://vmware.com/
Attachments:
v4-0001-Clarify-initdb-sync-only-help-message-and-docs.patchapplication/octet-stream; name=v4-0001-Clarify-initdb-sync-only-help-message-and-docs.patch; x-unix-mode=0644Download
From be9a91a0e3a2bd2a8c78087df6adc0adf6f43b98 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Fri, 30 Jul 2021 11:10:14 +0200
Subject: [PATCH v4] Clarify initdb --sync-only help message and docs
Make it clear that initdb --sync-only will exit after syncing, and
expand the docs with a note on when the option can be useful.
Author: Nathan Bossart, Gurjeet Singh
Discussion: https://postgr.es/m/CABwTF4U6hbNNE1bv=LxQdJybmUdZ5NJQ9rKY9tN82NXM8QH+iQ@mail.gmail.com
---
doc/src/sgml/ref/initdb.sgml | 3 +++
src/bin/initdb/initdb.c | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
index e62742850a..3d88838712 100644
--- a/doc/src/sgml/ref/initdb.sgml
+++ b/doc/src/sgml/ref/initdb.sgml
@@ -306,6 +306,9 @@ PostgreSQL documentation
<para>
Safely write all database files to disk and exit. This does not
perform any of the normal <application>initdb</application> operations.
+ Generally, this option is useful for ensuring reliable recovery after
+ changing <xref linkend="guc-fsync"/> from <literal>off</literal> to
+ <literal>on</literal>.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 994bf07f3b..2db65e09e9 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2200,7 +2200,7 @@ usage(const char *progname)
printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
printf(_(" --no-instructions do not print instructions for next steps\n"));
printf(_(" -s, --show show internal settings\n"));
- printf(_(" -S, --sync-only only sync data directory\n"));
+ printf(_(" -S, --sync-only only sync database files to disk, then exit\n"));
printf(_("\nOther options:\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
--
2.30.1 (Apple Git-130)
On 7/30/21, 2:22 AM, "Daniel Gustafsson" <daniel@yesql.se> wrote:
LGTM. I took the liberty to rephrase the "and exit" part of the initdb help
output match the other exiting options in there. Barring objections, I think
this is ready.
LGTM. Thanks!
Nathan
On 30 Jul 2021, at 18:27, Bossart, Nathan <bossartn@amazon.com> wrote:
On 7/30/21, 2:22 AM, "Daniel Gustafsson" <daniel@yesql.se> wrote:
LGTM. I took the liberty to rephrase the "and exit" part of the initdb help
output match the other exiting options in there. Barring objections, I think
this is ready.LGTM. Thanks!
Pushed to master, thanks!
--
Daniel Gustafsson https://vmware.com/
On Mon, Aug 16, 2021 at 4:42 AM Daniel Gustafsson <daniel@yesql.se> wrote:
On 30 Jul 2021, at 18:27, Bossart, Nathan <bossartn@amazon.com> wrote:
On 7/30/21, 2:22 AM, "Daniel Gustafsson" <daniel@yesql.se> wrote:
LGTM. I took the liberty to rephrase the "and exit" part of the initdb help
output match the other exiting options in there. Barring objections, I think
this is ready.LGTM. Thanks!
Pushed to master, thanks!
Thank you Daniel and Nathan! Much appreciated.
Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/