Warn if initdb's --sync-only option is mixed with other options
When reading through code for my previous patch [1]Slightly improve initdb --sync-only option's help message /messages/by-id/CABwTF4U6hbNNE1bv=LxQdJybmUdZ5NJQ9rKY9tN82NXM8QH+iQ@mail.gmail.com I realized that
initdb does *not* warn users that it ignores all other options (except
-D/--pgdata) if the --sync-only option is used.
I'm not able to come up with an exact situation to prove this, but
this behaviour seems potentially dangerous. The user might mix the
--sync-only option with other options, but would be extremely
surprised if those other options didn't take effect.
I _think_ we should throw an error if the user specifies any options
that are being ignored. But an error might break someone's automation
(perhaps for their own good), since the current behaviour has been in
place for a very long time, so I'm willing to settle for at least a
warning in such a case.
[1]: Slightly improve initdb --sync-only option's help message /messages/by-id/CABwTF4U6hbNNE1bv=LxQdJybmUdZ5NJQ9rKY9tN82NXM8QH+iQ@mail.gmail.com
Slightly improve initdb --sync-only option's help message
/messages/by-id/CABwTF4U6hbNNE1bv=LxQdJybmUdZ5NJQ9rKY9tN82NXM8QH+iQ@mail.gmail.com
Best regards,
--
Gurjeet Singh http://gurjeet.singh.im/
Attachments:
v1-0001-Warn-if-sync-only-is-used-with-other-options.patchapplication/octet-stream; name=v1-0001-Warn-if-sync-only-is-used-with-other-options.patchDownload
From 5624055762af88cf8e8e844d83959ffcf2be2ab4 Mon Sep 17 00:00:00 2001
From: Gurjeet Singh <gurjeet@singh.im>
Date: Wed, 7 Jul 2021 01:50:41 +0000
Subject: [PATCH v1 2/2] Warn if --sync-only is used with other options
---
src/bin/initdb/initdb.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 05a98b9ade..fe2c9684bf 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2960,6 +2960,7 @@ main(int argc, char *argv[])
char *effective_user;
PQExpBuffer start_db_cmd;
char pg_ctl_path[MAXPGPATH];
+ bool pg_data_opt_used = false;
/*
* Ensure that buffering behavior of stdout matches what it is in
@@ -3014,6 +3015,7 @@ main(int argc, char *argv[])
break;
case 'D':
pg_data = pg_strdup(optarg);
+ pg_data_opt_used = true;
break;
case 'E':
encoding = pg_strdup(optarg);
@@ -3127,6 +3129,18 @@ main(int argc, char *argv[])
/* If we only need to fsync, just do it and exit */
if (sync_only)
{
+
+ // 2 => 1 for command itself, and one for --sync-data option
+ int valid_argc = 2 + pg_data_opt_used + (pg_data != NULL);
+
+ // Ensure that there were no other options specified
+ if (argc > valid_argc)
+ {
+ // Maybe we should _error_ and exit?!
+ pg_log_warning("too many command-line arguments (--sync-only ignores all other options)");
+ //exit(1);
+ }
+
setup_pgdata();
/* must check that directory is readable */
--
2.30.0
On 7 Jul 2021, at 04:23, Gurjeet Singh <gurjeet@singh.im> wrote:
I'm not able to come up with an exact situation to prove this, but
this behaviour seems potentially dangerous. The user might mix the
--sync-only option with other options, but would be extremely
surprised if those other options didn't take effect.
Is if there is a plausible real world situation where a user runs --sync-only
together with other arguments and also miss the fact that the other arguments
didn't take effect, and have bad consequences?
I _think_ we should throw an error if the user specifies any options
that are being ignored. But an error might break someone's automation
(perhaps for their own good), since the current behaviour has been in
place for a very long time, so I'm willing to settle for at least a
warning in such a case.
We typically don't issue warnings for incompatible arguments, but rather error
out, and I'm not convinced this warrants breaking that. If we are going to do
anything I think we should error out; if we decide to do something then we
consider the scripts that will break to already be broken.
A slightly confusing aspect of this is however the error message for sync-only
when -D or PGDATA isn't set says "will reside" when in fact it should say "is
residing" (or something along those lines):
$ ./bin/initdb --sync-only
initdb: error: no data directory specified
You must identify the directory where the data for this database system
will reside. Do this with either the invocation option -D or the
environment variable PGDATA.
I doubt it's worth complicating the code for this fringe case though.
--
Daniel Gustafsson https://vmware.com/
On 7 Jul 2021, at 15:25, Daniel Gustafsson <daniel@yesql.se> wrote:
I doubt it's worth complicating the code for this fringe case though.
This thread has stalled, and with the updated docs/help output done for this
option I don't think this is worth pursuing (especially given the lack of
complaints over behavior which has existed for a very long time). I'm marking
this returned with feedback.
--
Daniel Gustafsson https://vmware.com/