[patch] pg_basebackup: mention that spread checkpoints are the default in --help

Started by Michael Banckabout 2 years ago10 messages

mbanck@gmx.net

about 2 years ago

1 attachment(s)

Hi,

I believed that spread (not fast) checkpoints are the default in
pg_basebackup, but noticed that --help does not specify which is which -
contrary to the reference documentation.

So I propose the small attached patch to clarify that.

Michael

Attachments:

0001-pg_basebackup-Mention-that-spread-checkpoints-are-th.patchtext/x-diff; charset=us-asciiDownload

From 2fc49eae5ccc82e144c3f683689757e014e331bd Mon Sep 17 00:00:00 2001
From: Michael Banck <michael.banck@credativ.de>
Date: Thu, 19 Oct 2023 11:37:11 +0200
Subject: [PATCH] pg_basebackup: Mention that spread checkpoints are the
 default in --help

---
 src/bin/pg_basebackup/pg_basebackup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 1a8cef345d..f2cf38a773 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -407,7 +407,7 @@ usage(void)
 	printf(_("  -Z, --compress=none    do not compress tar output\n"));
 	printf(_("\nGeneral options:\n"));
 	printf(_("  -c, --checkpoint=fast|spread\n"
-			 "                         set fast or spread checkpointing\n"));
+			 "                         set fast or spread (default) checkpointing\n"));
 	printf(_("  -C, --create-slot      create replication slot\n"));
 	printf(_("  -l, --label=LABEL      set backup label\n"));
 	printf(_("  -n, --no-clean         do not clean up after errors\n"));
-- 
2.39.2

Aleksander Alekseev

aleksander@timescale.com

about 2 years ago

In reply to: Michael Banck (#1)

Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

Hi,

I believed that spread (not fast) checkpoints are the default in
pg_basebackup, but noticed that --help does not specify which is which -
contrary to the reference documentation.

So I propose the small attached patch to clarify that.

You are right and I believe this is a good change.

Maybe we should also display the defaults for -X,
--manifest-checksums, etc for consistency.

--
Best regards,
Aleksander Alekseev

Michael Banck

mbanck@gmx.net

about 2 years ago

In reply to: Aleksander Alekseev (#2)

Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

Hi,

On Thu, Oct 19, 2023 at 04:21:19PM +0300, Aleksander Alekseev wrote:

I believed that spread (not fast) checkpoints are the default in
pg_basebackup, but noticed that --help does not specify which is which -
contrary to the reference documentation.

So I propose the small attached patch to clarify that.

You are right and I believe this is a good change.

Maybe we should also display the defaults for -X,
--manifest-checksums, etc for consistency.

Hrm right, but those have multiple options and they do not enumerate
them in the help string as do -F and -c - not sure what general project
policy here is for mentioning defaults in --help, I will check some of
the other commands.

Michael

Michael Paquier

michael@paquier.xyz

about 2 years ago

In reply to: Michael Banck (#3)

Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

On Thu, Oct 19, 2023 at 10:30:04PM +0200, Michael Banck wrote:

Hrm right, but those have multiple options and they do not enumerate
them in the help string as do -F and -c - not sure what general project
policy here is for mentioning defaults in --help, I will check some of
the other commands.

Then comes the point that this bloats the --help output. A bunch of
system commands I use on a daily-basis outside Postgres don't do that,
so it's kind of hard to put a line on what's good or not in this area
while we have the SGML and man pages to do the job, with always more
details.
--
Michael

Aleksander Alekseev

aleksander@timescale.com

about 2 years ago

In reply to: Michael Paquier (#4)

Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

Hi,

On Thu, Oct 19, 2023 at 10:30:04PM +0200, Michael Banck wrote:

Hrm right, but those have multiple options and they do not enumerate
them in the help string as do -F and -c - not sure what general project
policy here is for mentioning defaults in --help, I will check some of
the other commands.

Then comes the point that this bloats the --help output. A bunch of
system commands I use on a daily-basis outside Postgres don't do that,
so it's kind of hard to put a line on what's good or not in this area
while we have the SGML and man pages to do the job, with always more
details.

Right. Then I suggest merging the patch as is.

--
Best regards,
Aleksander Alekseev

Peter Eisentraut

peter@eisentraut.org

about 2 years ago

In reply to: Michael Banck (#1)

Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

On 19.10.23 11:39, Michael Banck wrote:

Hi,

I believed that spread (not fast) checkpoints are the default in
pg_basebackup, but noticed that --help does not specify which is which -
contrary to the reference documentation.

So I propose the small attached patch to clarify that.

printf(_(" -c, --checkpoint=fast|spread\n"
- " set fast or spread

checkpointing\n"));

+ " set fast or spread (default)

checkpointing\n"));

Could we do like

-c, --checkpoint=fast|spread
set fast or spread checkpointing
(default: spread)

This seems to be easier to read.

Michael Banck

mbanck@gmx.net

about 2 years ago

In reply to: Peter Eisentraut (#6)

1 attachment(s)

Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

Hi,

On Wed, Oct 25, 2023 at 04:36:41PM +0200, Peter Eisentraut wrote:

On 19.10.23 11:39, Michael Banck wrote:

Hi,

I believed that spread (not fast) checkpoints are the default in
pg_basebackup, but noticed that --help does not specify which is which -
contrary to the reference documentation.

So I propose the small attached patch to clarify that.
printf(_("  -c, --checkpoint=fast|spread\n"
-          "                         set fast or spread checkpointing\n"));
+          "                         set fast or spread (default)
checkpointing\n"));

Could we do like

-c, --checkpoint=fast|spread
set fast or spread checkpointing
(default: spread)

This seems to be easier to read.

Yeah, we could do that. But then again the question pops up what to do
about the other option that mentions defaults (-F) and the others which
have a default but it is not spelt out yet (-X, -Z at least) (output is
still from v15, additional options have been added since):

-F, --format=p|t output format (plain (default), tar)
-X, --wal-method=none|fetch|stream
include required WAL files with specified method
-Z, --compress=0-9 compress tar output with given compression level

So, my personal opinion is that we should really document -c because it
is quite user-noticable compared to the others.

So attached is a new version with just your proposed change for now.

Michael

Attachments:

v2-0001-pg_basebackup-Mention-that-spread-checkpoints-are.patchtext/x-diff; charset=us-asciiDownload

From 817f71f2eaa8814a30320bc1ef97c1ec8a95f083 Mon Sep 17 00:00:00 2001
From: Michael Banck <michael.banck@credativ.de>
Date: Thu, 19 Oct 2023 11:37:11 +0200
Subject: [PATCH v2] pg_basebackup: Mention that spread checkpoints are the
 default in --help

---
 src/bin/pg_basebackup/pg_basebackup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 1a8cef345d..9957fb4f54 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -408,6 +408,7 @@ usage(void)
 	printf(_("\nGeneral options:\n"));
 	printf(_("  -c, --checkpoint=fast|spread\n"
 			 "                         set fast or spread checkpointing\n"));
+			 "                         (default: spread)\n"));
 	printf(_("  -C, --create-slot      create replication slot\n"));
 	printf(_("  -l, --label=LABEL      set backup label\n"));
 	printf(_("  -n, --no-clean         do not clean up after errors\n"));
-- 
2.39.2

Shlok Kyal

shlok.kyal.oss@gmail.com

about 2 years ago

In reply to: Michael Banck (#7)

Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

Hi,

On Thu, 26 Oct 2023 at 13:58, Michael Banck <mbanck@gmx.net> wrote:

Hi,

On Wed, Oct 25, 2023 at 04:36:41PM +0200, Peter Eisentraut wrote:
On 19.10.23 11:39, Michael Banck wrote:

Hi,

I believed that spread (not fast) checkpoints are the default in
pg_basebackup, but noticed that --help does not specify which is which -
contrary to the reference documentation.

So I propose the small attached patch to clarify that.
printf(_("  -c, --checkpoint=fast|spread\n"
-          "                         set fast or spread checkpointing\n"));
+          "                         set fast or spread (default)
checkpointing\n"));

Could we do like

-c, --checkpoint=fast|spread
set fast or spread checkpointing
(default: spread)

This seems to be easier to read.
Yeah, we could do that. But then again the question pops up what to do
about the other option that mentions defaults (-F) and the others which
have a default but it is not spelt out yet (-X, -Z at least) (output is
still from v15, additional options have been added since):

-F, --format=p|t output format (plain (default), tar)
-X, --wal-method=none|fetch|stream
include required WAL files with specified method
-Z, --compress=0-9 compress tar output with given compression level

So, my personal opinion is that we should really document -c because it
is quite user-noticable compared to the others.

So attached is a new version with just your proposed change for now.

Michael

I went through the Cfbot for this patch and found out that the build
is failing with the following error (Link:
https://cirrus-ci.com/task/4648506929971200?logs=build#L1217):

[08:34:47.625] FAILED: src/bin/pg_basebackup/pg_basebackup.p/pg_basebackup.c.o
[08:34:47.625] ccache cc -Isrc/bin/pg_basebackup/pg_basebackup.p
-Isrc/include -I../src/include -Isrc/interfaces/libpq
-I../src/interfaces/libpq -Isrc/include/catalog -Isrc/include/nodes
-Isrc/include/utils -fdiagnostics-color=always -pipe
-D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -g -fno-strict-aliasing
-fwrapv -fexcess-precision=standard -D_GNU_SOURCE -Wmissing-prototypes
-Wpointer-arith -Werror=vla -Wendif-labels -Wmissing-format-attribute
-Wimplicit-fallthrough=3 -Wcast-function-type
-Wshadow=compatible-local -Wformat-security
-Wdeclaration-after-statement -Wno-format-truncation
-Wno-stringop-truncation -pthread -MD -MQ
src/bin/pg_basebackup/pg_basebackup.p/pg_basebackup.c.o -MF
src/bin/pg_basebackup/pg_basebackup.p/pg_basebackup.c.o.d -o
src/bin/pg_basebackup/pg_basebackup.p/pg_basebackup.c.o -c
../src/bin/pg_basebackup/pg_basebackup.c
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c: In function ‘usage’:
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c:411:5:
warning: statement with no effect [-Wunused-value]
[08:34:47.625] 411 | " (default: spread)\n"));
[08:34:47.625] | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c:411:51: error:
expected ‘;’ before ‘)’ token
[08:34:47.625] 411 | " (default: spread)\n"));
[08:34:47.625] | ^
[08:34:47.625] | ;
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c:411:51: error:
expected statement before ‘)’ token
[08:34:47.625] ../src/bin/pg_basebackup/pg_basebackup.c:411:52: error:
expected statement before ‘)’ token
[08:34:47.625] 411 | " (default: spread)\n"));
[08:34:47.625] | ^
[08:34:47.629] [1210/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/parallel.c.o
[08:34:47.639] [1211/1832] Compiling C object
src/bin/pg_basebackup/pg_recvlogical.p/pg_recvlogical.c.o
[08:34:47.641] [1212/1832] Linking static target
src/bin/pg_basebackup/libpg_basebackup_common.a
[08:34:47.658] [1213/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/compress_io.c.o
[08:34:47.669] [1214/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/compress_lz4.c.o
[08:34:47.678] [1215/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/compress_zstd.c.o
[08:34:47.692] [1216/1832] Compiling C object
src/bin/pg_dump/libpgdump_common.a.p/dumputils.c.o
[08:34:47.692] ninja: build stopped: subcommand failed.

I also see that patch is marked 'Ready for Committer' on commitfest.

Just wanted to make sure, you are aware of this error.

Thanks,
Shlok Kumar Kyal

Michael Banck

mbanck@gmx.net

about 2 years ago

In reply to: Shlok Kyal (#8)

1 attachment(s)

Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

Hi,

On Tue, Oct 31, 2023 at 04:59:24PM +0530, Shlok Kyal wrote:

I went through the Cfbot for this patch and found out that the build
is failing with the following error (Link:
https://cirrus-ci.com/task/4648506929971200?logs=build#L1217):

Oops, sorry. Attached is a working third version of this patch.

Michael

Attachments:

v3-0001-pg_basebackup-Mention-that-spread-checkpoints-are.patchtext/x-diff; charset=us-asciiDownload

From bc9eb46a49ee514236aabe42d9689a7c35b5bcd7 Mon Sep 17 00:00:00 2001
From: Michael Banck <michael.banck@credativ.de>
Date: Thu, 19 Oct 2023 11:37:11 +0200
Subject: [PATCH v3] pg_basebackup: Mention that spread checkpoints are the
 default in --help

---
 src/bin/pg_basebackup/pg_basebackup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index f32684a8f2..b0ac77b4ce 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -407,7 +407,8 @@ usage(void)
 	printf(_("  -Z, --compress=none    do not compress tar output\n"));
 	printf(_("\nGeneral options:\n"));
 	printf(_("  -c, --checkpoint=fast|spread\n"
-			 "                         set fast or spread checkpointing\n"));
+			 "                         set fast or spread checkpointing\n"
+			 "                         (default: spread)\n"));
 	printf(_("  -C, --create-slot      create replication slot\n"));
 	printf(_("  -l, --label=LABEL      set backup label\n"));
 	printf(_("  -n, --no-clean         do not clean up after errors\n"));
-- 
2.39.2

#10

Magnus Hagander

magnus@hagander.net

about 2 years ago

In reply to: Michael Banck (#9)

Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

On Tue, Oct 31, 2023 at 8:01 PM Michael Banck <mbanck@gmx.net> wrote:

Hi,

On Tue, Oct 31, 2023 at 04:59:24PM +0530, Shlok Kyal wrote:

I went through the Cfbot for this patch and found out that the build
is failing with the following error (Link:
https://cirrus-ci.com/task/4648506929971200?logs=build#L1217):

Oops, sorry. Attached is a working third version of this patch.

While I think Peters argument about one reading better than the other
one, that does also increase the "help message bloat" mentioned by
Michael. So I think we're better off actually using the original
version, so I'm going to go ahead and push that one (and also to avoid
endless bikeshedding)-

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/