Conflicting option checking in pg_restore

Started by Daniel Gustafssonabout 7 years ago9 messages

daniel@yesql.se

about 7 years ago

1 attachment(s)

Checking for conflicting options in pg_restore was mostly done in main() with
one check deferred until RestoreArchive(). Reading the git history makes it
seem like it simply happened, without the disjoint checking being intentional.
Am I reading it right that we can consolidate all the option checking to
main()? The attached patch does that, and also rewords the error message to
make it similar to the other option checks.

cheers ./daniel

Attachments:

pg_restore_options.patchapplication/octet-stream; name=pg_restore_options.patch; x-unix-mode=0644Download

From 9edff69d80ec3f8d4e54aafab357916a7d03ae22 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Sat, 27 Oct 2018 22:41:53 +0200
Subject: [PATCH] Consolidate option checking in pg_restore

Move the check for conflicting options from the archive restore code
to the main function where other similar checks are performed.  Also
reword the error message to be in line with other messages.
---
 src/bin/pg_dump/pg_backup_archiver.c |  9 ---------
 src/bin/pg_dump/pg_restore.c         | 11 +++++++++++
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index e976def42a..defa8a41b7 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -358,15 +358,6 @@ RestoreArchive(Archive *AHX)
 
 	AH->stage = STAGE_INITIALIZING;
 
-	/*
-	 * Check for nonsensical option combinations.
-	 *
-	 * -C is not compatible with -1, because we can't create a database inside
-	 * a transaction block.
-	 */
-	if (ropt->createDB && ropt->single_txn)
-		exit_horribly(modulename, "-C and -1 are incompatible options\n");
-
 	/*
 	 * If we're going to do parallel restore, there are some restrictions.
 	 */
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 501d7cea72..58b36b6bf7 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -331,6 +331,17 @@ main(int argc, char **argv)
 		exit_nicely(1);
 	}
 
+	/*
+	 * -C is not compatible with -1, because we can't create a database inside
+	 * a transaction block.
+	 */
+	if (opts->createDB && opts->single_txn)
+	{
+		fprintf(stderr, _("%s: options -C/--create and -1/--single-transaction cannot be used together\n"),
+				progname);
+		exit_nicely(1);
+	}
+
 	if (numWorkers <= 0)
 	{
 		fprintf(stderr, _("%s: invalid number of parallel jobs\n"), progname);
-- 
2.14.1.145.gb3622a4ee

Fabien COELHO

coelho@cri.ensmp.fr

about 7 years ago

In reply to: Daniel Gustafsson (#1)

Re: Conflicting option checking in pg_restore

Hallï¿œ Daniel,

Checking for conflicting options in pg_restore was mostly done in main() with
one check deferred until RestoreArchive(). Reading the git history makes it
seem like it simply happened, without the disjoint checking being intentional.
Am I reading it right that we can consolidate all the option checking to
main()? The attached patch does that, and also rewords the error message to
make it similar to the other option checks.

Patch applies cleanly, compiles, both global and local "make check" ok.

As there are no test changes, this is not tested. I'd suggest to add it to
"src/bin/pg_dump/t/001_basic.pl".

There is a possible catch:

Function RestoreArchive is called both from pg_dump & pg_restore, so now
the sanity check is not performed for the former (which does not have the
-1 option, though). Moreover, the function is noted "Public", which may
suggest that external tools could take advantage of it, and if so it
suggests that maybe it is not wise to remove the test. Any opinion around?

Maybe the check could be kept in RestoreArchive with a comment to say it
is redundant, but the check could be performed in pg_restore as well for
the sake of having an better and more homogeneous error message.

--
Fabien.

Narayanan V

vnarayanan.email@gmail.com

about 7 years ago

In reply to: Fabien COELHO (#2)

Re: Conflicting option checking in pg_restore

Hi
On Sun, Oct 28, 2018 at 12:25 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Hallå Daniel,

Checking for conflicting options in pg_restore was mostly done in main()

with

one check deferred until RestoreArchive(). Reading the git history

makes it

seem like it simply happened, without the disjoint checking being

intentional.

Am I reading it right that we can consolidate all the option checking to
main()? The attached patch does that, and also rewords the error

message to

make it similar to the other option checks.

Patch applies cleanly, compiles, both global and local "make check" ok.

As there are no test changes, this is not tested. I'd suggest to add it to
"src/bin/pg_dump/t/001_basic.pl".

There is a possible catch:

Function RestoreArchive is called both from pg_dump & pg_restore, so now
the sanity check is not performed for the former (which does not have the
-1 option, though). Moreover, the function is noted "Public", which may
suggest that external tools could take advantage of it, and if so it
suggests that maybe it is not wise to remove the test. Any opinion around?

The RestoreArchive Method extracts the RestoreOptions from the Archive
Handle.
Please see the relevant code below,

,----
| void
| RestoreArchive(Archive *AHX)
| {
| ArchiveHandle *AH = (ArchiveHandle *) AHX;
| RestoreOptions *ropt = AH->public.ropt;
`----

Wouldn't ropt->single_txn be undefined when called from pg_dump ?

Unless I missed something here, I think it is logical to just move the
relevant code to
pg_restore main.

- Tested the patch on a machine running -Ubuntu 18.04.1 LTS
- Patch applied cleanly.
- The source built post application of the patch.
- make check passed.
,----
| =======================
| All 187 tests passed.
| =======================
`----

+1 from me.

Thank you,
Narayanan

Show quoted text

Maybe the check could be kept in RestoreArchive with a comment to say it
is redundant, but the check could be performed in pg_restore as well for
the sake of having an better and more homogeneous error message.

--
Fabien.

Fabien COELHO

coelho@cri.ensmp.fr

about 7 years ago

In reply to: Narayanan V (#3)

Re: Conflicting option checking in pg_restore

Hello Narayanan,

There is a possible catch:

Function RestoreArchive is called both from pg_dump & pg_restore, so now
the sanity check is not performed for the former (which does not have the
-1 option, though). Moreover, the function is noted "Public", which may
suggest that external tools could take advantage of it, and if so it
suggests that maybe it is not wise to remove the test. Any opinion around?

[...]

Wouldn't ropt->single_txn be undefined when called from pg_dump ?

Yes, probably.

Unless I missed something here, I think it is logical to just move the
relevant code to pg_restore main.

My point is that given the "Public" comment and that some care is taken to
put everything in a special struct, I was wondering whether external tools
may use this function, in which case the check would be left out.

--
Fabien.

Daniel Gustafsson

daniel@yesql.se

about 7 years ago

In reply to: Fabien COELHO (#2)

1 attachment(s)

Re: Conflicting option checking in pg_restore

Could you add the patch to the CF app?

https://commitfest.postgresql.org/20/

Done.

Checking for conflicting options in pg_restore was mostly done in main() with
one check deferred until RestoreArchive(). Reading the git history makes it
seem like it simply happened, without the disjoint checking being intentional.
Am I reading it right that we can consolidate all the option checking to
main()? The attached patch does that, and also rewords the error message to
make it similar to the other option checks.

Patch applies cleanly, compiles, both global and local "make check" ok.

Thanks for reviewing!

As there are no test changes, this is not tested. I'd suggest to add it to "src/bin/pg_dump/t/001_basic.pl”.

Excellent point, I’ve added a test in the attached revision.

There is a possible catch:

Function RestoreArchive is called both from pg_dump & pg_restore, so now the sanity check is not performed for the former (which does not have the -1 option, though). Moreover, the function is noted "Public", which may suggest that external tools could take advantage of it, and if so it suggests that maybe it is not wise to remove the test. Any opinion around?

Maybe the check could be kept in RestoreArchive with a comment to say it is redundant, but the check could be performed in pg_restore as well for the sake of having an better and more homogeneous error message.

Perhaps, we don’t really test for all other potential misconfigurations of the
options so I can go either way. It’s also a cheap enough operation. Do you
think it should be a check like today or an assertion?

cheers ./daniel

Attachments:

pg_restore_options-v2.patchapplication/octet-stream; name=pg_restore_options-v2.patch; x-unix-mode=0644Download

From d03d959bf57e2244f2263c3ec1d71a3c8fd182ff Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Sat, 27 Oct 2018 22:41:53 +0200
Subject: [PATCH] Consolidate option checking in pg_restore

Move the check for conflicting options from the archive restore code
to the main function where other similar checks are performed.  Also
reword the error message to be in line with other messages.
---
 src/bin/pg_dump/pg_backup_archiver.c |  9 ---------
 src/bin/pg_dump/pg_restore.c         | 11 +++++++++++
 src/bin/pg_dump/t/001_basic.pl       |  8 +++++++-
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index e976def42a..defa8a41b7 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -358,15 +358,6 @@ RestoreArchive(Archive *AHX)
 
 	AH->stage = STAGE_INITIALIZING;
 
-	/*
-	 * Check for nonsensical option combinations.
-	 *
-	 * -C is not compatible with -1, because we can't create a database inside
-	 * a transaction block.
-	 */
-	if (ropt->createDB && ropt->single_txn)
-		exit_horribly(modulename, "-C and -1 are incompatible options\n");
-
 	/*
 	 * If we're going to do parallel restore, there are some restrictions.
 	 */
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 501d7cea72..58b36b6bf7 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -331,6 +331,17 @@ main(int argc, char **argv)
 		exit_nicely(1);
 	}
 
+	/*
+	 * -C is not compatible with -1, because we can't create a database inside
+	 * a transaction block.
+	 */
+	if (opts->createDB && opts->single_txn)
+	{
+		fprintf(stderr, _("%s: options -C/--create and -1/--single-transaction cannot be used together\n"),
+				progname);
+		exit_nicely(1);
+	}
+
 	if (numWorkers <= 0)
 	{
 		fprintf(stderr, _("%s: invalid number of parallel jobs\n"), progname);
diff --git a/src/bin/pg_dump/t/001_basic.pl b/src/bin/pg_dump/t/001_basic.pl
index 17edf444b2..de00dbca39 100644
--- a/src/bin/pg_dump/t/001_basic.pl
+++ b/src/bin/pg_dump/t/001_basic.pl
@@ -4,7 +4,7 @@ use warnings;
 use Config;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 70;
+use Test::More tests => 72;
 
 my $tempdir       = TestLib::tempdir;
 my $tempdir_short = TestLib::tempdir_short;
@@ -150,3 +150,9 @@ command_fails_like(
 	[ 'pg_dumpall', '--if-exists' ],
 	qr/\Qpg_dumpall: option --if-exists requires option -c\/--clean\E/,
 	'pg_dumpall: option --if-exists requires option -c/--clean');
+
+command_fails_like(
+	[ 'pg_restore', '-C', '-1' ],
+	qr/\Qpg_restore: options -C\/--create and -1\/--single-transaction cannot be used together\E/,
+	'pg_restore: options -C\/--create and -1\/--single-transaction cannot be used together'
+);
-- 
2.14.1.145.gb3622a4ee

Daniel Gustafsson

daniel@yesql.se

about 7 years ago

In reply to: Fabien COELHO (#4)

Re: Conflicting option checking in pg_restore

On 28 Oct 2018, at 19:42, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Function RestoreArchive is called both from pg_dump & pg_restore, so now
the sanity check is not performed for the former (which does not have the
-1 option, though). Moreover, the function is noted "Public", which may
suggest that external tools could take advantage of it, and if so it
suggests that maybe it is not wise to remove the test. Any opinion around?

[...]

Wouldn't ropt->single_txn be undefined when called from pg_dump ?

Yes, probably.

pg_dump creates the RestoreOptions struct with NewRestoreOptions() which
allocates it with pg_malloc0(), making single_txn false.

cheers ./daniel

Michael Paquier

michael@paquier.xyz

about 7 years ago

In reply to: Daniel Gustafsson (#6)

Re: Conflicting option checking in pg_restore

On Sun, Oct 28, 2018 at 10:02:02PM +0100, Daniel Gustafsson wrote:

On 28 Oct 2018, at 19:42, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Function RestoreArchive is called both from pg_dump & pg_restore, so now
the sanity check is not performed for the former (which does not have the
-1 option, though). Moreover, the function is noted "Public", which may
suggest that external tools could take advantage of it, and if so it
suggests that maybe it is not wise to remove the test. Any opinion around?

I would still need to see such a tool, and they would need most of the
stuff that pg_dump has already to get the object data. As far as I know
it is marked as public because plain-text mode of pg_dump uses it.

Wouldn't ropt->single_txn be undefined when called from pg_dump ?

Yes, probably.

pg_dump creates the RestoreOptions struct with NewRestoreOptions() which
allocates it with pg_malloc0(), making single_txn false.

Which is why it would not be a problem for pg_dump. The cross-option
check you are moving has been added as of 3a819b07, still you are right
that such checks should happen at the beginning so the suggestion of
this thread seems right to me.

Any objections from others?
--
Michael

Fabien COELHO

coelho@cri.ensmp.fr

about 7 years ago

In reply to: Daniel Gustafsson (#5)

Re: Conflicting option checking in pg_restore

Hallå Daniel,

an assertion?

v2 applies, compiles, both local & global make check are ok.

Its CF category could have been "bug fix" or "restructuring".

[...] Perhaps, we don’t really test for all other potential
misconfigurations of the options so I can go either way. It’s also a
cheap enough operation. Do you think it should be a check like today or

Michaël suggests that there is no issue of external tool using the
internal function, so I'm fine with this version.

I have switched the patch to ready for committer.

--
Fabien.

Michael Paquier

michael@paquier.xyz

about 7 years ago

In reply to: Fabien COELHO (#8)

Re: Conflicting option checking in pg_restore

On Mon, Oct 29, 2018 at 07:11:35AM +0100, Fabien COELHO wrote:

Michaël suggests that there is no issue of external tool using the internal
function, so I'm fine with this version.

I have switched the patch to ready for committer.

One catch with this refactoring is that for example this combination
does not result in an error on HEAD, but it does with the patch:
pg_restore -l -C -1

Anyway, the fact that we save the caller from one exit_horribly()
knowing that opening the archive is completely useless makes the move
worth it in my opinion. RestoreArchive should complain about things
which depend on the opened archive, which is the only thing it does
now. I would not risk back-patching it though.

At the same time, I have checked the set of TAP tests for pg_restore and
the cross-option checks are all covered, so no need to go crazy on this
side.

For the archive's sake, the original commit 3a819b07 which did the
option check in RestoreArchive comes from here:
/messages/by-id/496B6B40.1010909@hagander.net

And committed, thanks Daniel and Fabien!
--
Michael