There should be a way to use the force flag when restoring databases
Since posgres 13 there's the option to do a FORCE when dropping a database
(so it disconnects current users) Documentation here:
https://www.postgresql.org/docs/current/sql-dropdatabase.html
I am currently using dir format for the output
pg_dump -d "bdname" -F d -j 4 -v -f /tmp/dir
And restoring the database with
pg_restore -d postgres -C -c --exit-on-error -F d -j 3 -v /tmp/dir
Having an option to add the FORCE option to either the generated dump by
pg_dump, or in the pg_restore would be very useful when restoring the
databases to another servers so it would avoid having to do scripting.
In my specific case I am using this to refresh periodically a development
environment with data from production servers for a small database (~200M).
Thanks,
Joan
On Tue, Jul 18, 2023 at 12:53 AM Joan <aseques@gmail.com> wrote:
Since posgres 13 there's the option to do a FORCE when dropping a database (so it disconnects current users) Documentation here: https://www.postgresql.org/docs/current/sql-dropdatabase.html
I am currently using dir format for the output
pg_dump -d "bdname" -F d -j 4 -v -f /tmp/dirAnd restoring the database with
pg_restore -d postgres -C -c --exit-on-error -F d -j 3 -v /tmp/dirHaving an option to add the FORCE option to either the generated dump by pg_dump, or in the pg_restore would be very useful when restoring the databases to another servers so it would avoid having to do scripting.
In my specific case I am using this to refresh periodically a development environment with data from production servers for a small database (~200M).
Making force-drop a part of pg_dump output may be dangerous, and not
provide much flexibility at restore time.
Adding a force option to pg_restore feels like providing the right tradeoff.
The docs for 'pg_restore --create` say "Create the database before
restoring into it. If --clean is also specified, drop and recreate the
target database before connecting to it."
If we provided a force option, it may then additionally say: "If the
--clean and --force options are specified, DROP DATABASE ... WITH
FORCE command will be used to drop the database."
Using WITH FORCE is not a guarantee, as the DROP DATABASE docs clarify
the conditions under which it may fail.
Best regards,
Gurjeet
http://Gurje.et
HI Gurjeet, that woulld be great, all the cases where a FORCE won't apply
make totally sense (either complex scenarios or permission issues)
It doesn't terminate if prepared transactions, active logical replication
slots or subscriptions are present in the target database.
This will fail if the current user has no permissions to terminate other
connections
Regards
Missatge de Gurjeet Singh <gurjeet@singh.im> del dia dc., 19 de jul. 2023 a
les 19:28:
Show quoted text
On Tue, Jul 18, 2023 at 12:53 AM Joan <aseques@gmail.com> wrote:
Since posgres 13 there's the option to do a FORCE when dropping a
database (so it disconnects current users) Documentation here:
https://www.postgresql.org/docs/current/sql-dropdatabase.htmlI am currently using dir format for the output
pg_dump -d "bdname" -F d -j 4 -v -f /tmp/dirAnd restoring the database with
pg_restore -d postgres -C -c --exit-on-error -F d -j 3 -v /tmp/dirHaving an option to add the FORCE option to either the generated dump by
pg_dump, or in the pg_restore would be very useful when restoring the
databases to another servers so it would avoid having to do scripting.In my specific case I am using this to refresh periodically a
development environment with data from production servers for a small
database (~200M).Making force-drop a part of pg_dump output may be dangerous, and not
provide much flexibility at restore time.Adding a force option to pg_restore feels like providing the right
tradeoff.The docs for 'pg_restore --create` say "Create the database before
restoring into it. If --clean is also specified, drop and recreate the
target database before connecting to it."If we provided a force option, it may then additionally say: "If the
--clean and --force options are specified, DROP DATABASE ... WITH
FORCE command will be used to drop the database."Using WITH FORCE is not a guarantee, as the DROP DATABASE docs clarify
the conditions under which it may fail.Best regards,
Gurjeet
http://Gurje.et
On 19 Jul 2023, at 19:28, Gurjeet Singh <gurjeet@singh.im> wrote:
On Tue, Jul 18, 2023 at 12:53 AM Joan <aseques@gmail.com> wrote:
Since posgres 13 there's the option to do a FORCE when dropping a database (so it disconnects current users) Documentation here: https://www.postgresql.org/docs/current/sql-dropdatabase.html
I am currently using dir format for the output
pg_dump -d "bdname" -F d -j 4 -v -f /tmp/dirAnd restoring the database with
pg_restore -d postgres -C -c --exit-on-error -F d -j 3 -v /tmp/dirHaving an option to add the FORCE option to either the generated dump by pg_dump, or in the pg_restore would be very useful when restoring the databases to another servers so it would avoid having to do scripting.
In my specific case I am using this to refresh periodically a development environment with data from production servers for a small database (~200M).
Making force-drop a part of pg_dump output may be dangerous, and not
provide much flexibility at restore time.Adding a force option to pg_restore feels like providing the right tradeoff.
The docs for 'pg_restore --create` say "Create the database before
restoring into it. If --clean is also specified, drop and recreate the
target database before connecting to it."If we provided a force option, it may then additionally say: "If the
--clean and --force options are specified, DROP DATABASE ... WITH
FORCE command will be used to drop the database."
pg_restore --clean refers to dropping any pre-existing database objects and not
just databases, but --force would only apply to databases.
I wonder if it's worth complicating pg_restore with that when running dropdb
--force before pg_restore is an option for those wanting to use WITH FORCE.
--
Daniel Gustafsson
On Thu, Jul 20, 2023 at 2:10 AM Daniel Gustafsson <daniel@yesql.se> wrote:
On 19 Jul 2023, at 19:28, Gurjeet Singh <gurjeet@singh.im> wrote:
The docs for 'pg_restore --create` say "Create the database before
restoring into it. If --clean is also specified, drop and recreate the
target database before connecting to it."If we provided a force option, it may then additionally say: "If the
--clean and --force options are specified, DROP DATABASE ... WITH
FORCE command will be used to drop the database."pg_restore --clean refers to dropping any pre-existing database objects and not
just databases, but --force would only apply to databases.I wonder if it's worth complicating pg_restore with that when running dropdb
--force before pg_restore is an option for those wanting to use WITH FORCE.
Fair point. But the same argument could've been applied to --clean
option, as well; why overload the meaning of --clean and make it drop
database, when a dropdb before pg_restore was an option.
IMHO, if pg_restore offers to drop database, providing an option to
the user to do it forcibly is not that much of a stretch, and within
reason for the user to expect it to be there, like Joan did.
Best regards,
Gurjeet
http://Gurje.et
Hi everyone,
I have been working on this. This is a proposed patch for it so we have a
force option for DROPping the database.
I'd appreciate it if anyone can review.
On Thu, Jul 20, 2023 at 9:36 PM Gurjeet Singh <gurjeet@singh.im> wrote:
Show quoted text
On Thu, Jul 20, 2023 at 2:10 AM Daniel Gustafsson <daniel@yesql.se> wrote:
On 19 Jul 2023, at 19:28, Gurjeet Singh <gurjeet@singh.im> wrote:
The docs for 'pg_restore --create` say "Create the database before
restoring into it. If --clean is also specified, drop and recreate the
target database before connecting to it."If we provided a force option, it may then additionally say: "If the
--clean and --force options are specified, DROP DATABASE ... WITH
FORCE command will be used to drop the database."pg_restore --clean refers to dropping any pre-existing database objects
and not
just databases, but --force would only apply to databases.
I wonder if it's worth complicating pg_restore with that when running
dropdb
--force before pg_restore is an option for those wanting to use WITH
FORCE.
Fair point. But the same argument could've been applied to --clean
option, as well; why overload the meaning of --clean and make it drop
database, when a dropdb before pg_restore was an option.IMHO, if pg_restore offers to drop database, providing an option to
the user to do it forcibly is not that much of a stretch, and within
reason for the user to expect it to be there, like Joan did.Best regards,
Gurjeet
http://Gurje.et
Attachments:
force.patchtext/x-patch; charset=US-ASCII; name=force.patchDownload
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index aba780ef4b..aeec3c8dcb 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -154,6 +154,8 @@ typedef struct _restoreOptions
int enable_row_security;
int sequence_data; /* dump sequence data even in schema-only mode */
int binary_upgrade;
+
+ int force;
} RestoreOptions;
typedef struct _dumpOptions
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 39ebcfec32..cc13be841a 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -532,6 +532,16 @@ RestoreArchive(Archive *AHX)
*/
if (*te->dropStmt != '\0')
{
+ if (ropt->force){
+ char *dropStmt = pg_strdup(te->dropStmt);
+ PQExpBuffer ftStmt = createPQExpBuffer();
+ dropStmt[strlen(dropStmt) - 2] = ' ';
+ dropStmt[strlen(dropStmt) - 1] = '\0';
+ appendPQExpBufferStr(ftStmt, dropStmt);
+ appendPQExpBufferStr(ftStmt, "WITH(FORCE);");
+ te->dropStmt = ftStmt->data;
+ }
+
if (!ropt->if_exists ||
strncmp(te->dropStmt, "--", 2) == 0)
{
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 049a100634..02347e86fb 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -74,6 +74,7 @@ main(int argc, char **argv)
static int no_security_labels = 0;
static int no_subscriptions = 0;
static int strict_names = 0;
+ static int force = 0;
struct option cmdopts[] = {
{"clean", 0, NULL, 'c'},
@@ -123,6 +124,7 @@ main(int argc, char **argv)
{"no-publications", no_argument, &no_publications, 1},
{"no-security-labels", no_argument, &no_security_labels, 1},
{"no-subscriptions", no_argument, &no_subscriptions, 1},
+ {"force", no_argument, &force, 1},
{NULL, 0, NULL, 0}
};
@@ -351,6 +353,7 @@ main(int argc, char **argv)
opts->no_publications = no_publications;
opts->no_security_labels = no_security_labels;
opts->no_subscriptions = no_subscriptions;
+ opts->force = force;
if (if_exists && !opts->dropSchema)
pg_fatal("option --if-exists requires option -c/--clean");
On Sun, Jul 23, 2023 at 6:09 AM Ahmed Ibrahim
<ahmed.ibr.hashim@gmail.com> wrote:
Hi everyone,
I have been working on this. This is a proposed patch for it so we have a force option for DROPping the database.
I'd appreciate it if anyone can review.
Hi Ahmed,
Thanks for working on this patch!
+
+ int force;
That extra blank line is unnecessary.
Using the bool data type, instead of int, for this option would've
more natural.
+ if (ropt->force){
Postgres coding style is to place the curly braces on a new line,
by themselves.
+ char *dropStmt = pg_strdup(te->dropStmt);
See if you can use pnstrdup(). Using that may obviate the need for
doing the null-placement acrobatics below.
+ PQExpBuffer ftStmt = createPQExpBuffer();
What does the 'ft' stand for in this variable's name?
+ dropStmt[strlen(dropStmt) - 2] = ' ';
+ dropStmt[strlen(dropStmt) - 1] = '\0';Try to evaluate the strlen() once and reuse it.
+ appendPQExpBufferStr(ftStmt, dropStmt);
+ appendPQExpBufferStr(ftStmt, "WITH(FORCE);");
+ te->dropStmt = ftStmt->data;
+ }
+Remove the extra trailing whitespace on that last blank line.
I think this whole code block needs to be protected by an 'if
(ropt->createDB)' check, like it's done about 20 lines above this
hunk. Otherwise, we may be appending 'WITH (FORCE)' for the DROP
command of a different (not a database) object type.
Also, you may want to check that the target database version is
one that supports WITH force option. This command will fail for
anything before v13.
The patch needs doc changes (pg_restore.sgml). And it needs to
mention --force option in the help output, as well (usage() function).
Can you please see if you can add appropriate test case for this.
The committers may insist on it, when reviewing.
Here are a couple of helpful links on how to prepare and submit
patches to the community. You may not need to strictly adhere to
these, but try to pick up a few recommendations that would make the
reviewer's job a bit easier.
[1]: https://wiki.postgresql.org/wiki/Creating_Clean_Patches
[2]: https://wiki.postgresql.org/wiki/Submitting_a_Patch
Best regards,
Gurjeet
http://Gurje.et
Hi Gurjeet,
I have addressed all your comments except for the tests.
I have tried adding test cases but I wasn't able to do it as it's in my
mind. I am not able to do things like having connections to the database
and trying to force the restore, then it will complete successfully
otherwise it shows errors.
In the meantime I will continue trying to do the test cases. If anyone can
help on that, I will appreciate it.
Thanks
On Thu, Jul 27, 2023 at 1:36 AM Gurjeet Singh <gurjeet@singh.im> wrote:
Show quoted text
On Sun, Jul 23, 2023 at 6:09 AM Ahmed Ibrahim
<ahmed.ibr.hashim@gmail.com> wrote:Hi everyone,
I have been working on this. This is a proposed patch for it so we have
a force option for DROPping the database.
I'd appreciate it if anyone can review.
Hi Ahmed,
Thanks for working on this patch!
+
+ int force;That extra blank line is unnecessary.
Using the bool data type, instead of int, for this option would've
more natural.+ if (ropt->force){
Postgres coding style is to place the curly braces on a new line,
by themselves.+ char *dropStmt = pg_strdup(te->dropStmt);
See if you can use pnstrdup(). Using that may obviate the need for
doing the null-placement acrobatics below.+ PQExpBuffer ftStmt = createPQExpBuffer();
What does the 'ft' stand for in this variable's name?
+ dropStmt[strlen(dropStmt) - 2] = ' '; + dropStmt[strlen(dropStmt) - 1] = '\0';Try to evaluate the strlen() once and reuse it.
+ appendPQExpBufferStr(ftStmt, dropStmt); + appendPQExpBufferStr(ftStmt, "WITH(FORCE);"); + te->dropStmt = ftStmt->data; + } +Remove the extra trailing whitespace on that last blank line.
I think this whole code block needs to be protected by an 'if
(ropt->createDB)' check, like it's done about 20 lines above this
hunk. Otherwise, we may be appending 'WITH (FORCE)' for the DROP
command of a different (not a database) object type.Also, you may want to check that the target database version is
one that supports WITH force option. This command will fail for
anything before v13.The patch needs doc changes (pg_restore.sgml). And it needs to
mention --force option in the help output, as well (usage() function).Can you please see if you can add appropriate test case for this.
The committers may insist on it, when reviewing.Here are a couple of helpful links on how to prepare and submit
patches to the community. You may not need to strictly adhere to
these, but try to pick up a few recommendations that would make the
reviewer's job a bit easier.[1]: https://wiki.postgresql.org/wiki/Creating_Clean_Patches
[2]: https://wiki.postgresql.org/wiki/Submitting_a_PatchBest regards,
Gurjeet
http://Gurje.et
Attachments:
force-v2.patchapplication/x-patch; name=force-v2.patchDownload
diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index 47bd7dbda0..f24e06fdf7 100644
--- a/doc/src/sgml/ref/pg_restore.sgml
+++ b/doc/src/sgml/ref/pg_restore.sgml
@@ -726,6 +726,15 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--force</option></term>
+ <listitem>
+ <para>
+ Force database to drop while restoring if there are any connections.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index aba780ef4b..2d167b58bf 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -154,6 +154,7 @@ typedef struct _restoreOptions
int enable_row_security;
int sequence_data; /* dump sequence data even in schema-only mode */
int binary_upgrade;
+ bool force;
} RestoreOptions;
typedef struct _dumpOptions
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 39ebcfec32..218d440e35 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -532,6 +532,15 @@ RestoreArchive(Archive *AHX)
*/
if (*te->dropStmt != '\0')
{
+ if (ropt->createDB && ropt->force)
+ {
+ int queryLen = strlen(te->dropStmt);
+ char *dropStmt = pnstrdup(te->dropStmt, queryLen - 2);
+ PQExpBuffer newStmt = createPQExpBuffer();
+ appendPQExpBufferStr(newStmt, dropStmt);
+ appendPQExpBufferStr(newStmt, " WITH(FORCE);");
+ te->dropStmt = newStmt->data;
+ }
if (!ropt->if_exists ||
strncmp(te->dropStmt, "--", 2) == 0)
{
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 5dab1ba9ea..5041092ef9 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1139,6 +1139,7 @@ help(const char *progname)
printf(_(" -w, --no-password never prompt for password\n"));
printf(_(" -W, --password force password prompt (should happen automatically)\n"));
printf(_(" --role=ROLENAME do SET ROLE before dump\n"));
+ printf(_(" --force Force database to drop if there are other connections\n"));
printf(_("\nIf no database name is supplied, then the PGDATABASE environment\n"
"variable value is used.\n\n"));
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 049a100634..fc6cabd9d9 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -74,6 +74,7 @@ main(int argc, char **argv)
static int no_security_labels = 0;
static int no_subscriptions = 0;
static int strict_names = 0;
+ static bool force = 0;
struct option cmdopts[] = {
{"clean", 0, NULL, 'c'},
@@ -123,6 +124,7 @@ main(int argc, char **argv)
{"no-publications", no_argument, &no_publications, 1},
{"no-security-labels", no_argument, &no_security_labels, 1},
{"no-subscriptions", no_argument, &no_subscriptions, 1},
+ {"force", no_argument, &force, 1},
{NULL, 0, NULL, 0}
};
@@ -351,6 +353,7 @@ main(int argc, char **argv)
opts->no_publications = no_publications;
opts->no_security_labels = no_security_labels;
opts->no_subscriptions = no_subscriptions;
+ opts->force = force;
if (if_exists && !opts->dropSchema)
pg_fatal("option --if-exists requires option -c/--clean");
Hi all,
I have addressed the pg version compatibility with the FORCE option in
drop. Here is the last version of the patch
On Tue, Aug 1, 2023 at 6:19 PM Ahmed Ibrahim <ahmed.ibr.hashim@gmail.com>
wrote:
Show quoted text
Hi Gurjeet,
I have addressed all your comments except for the tests.
I have tried adding test cases but I wasn't able to do it as it's in my
mind. I am not able to do things like having connections to the database
and trying to force the restore, then it will complete successfully
otherwise it shows errors.In the meantime I will continue trying to do the test cases. If anyone can
help on that, I will appreciate it.Thanks
On Thu, Jul 27, 2023 at 1:36 AM Gurjeet Singh <gurjeet@singh.im> wrote:
On Sun, Jul 23, 2023 at 6:09 AM Ahmed Ibrahim
<ahmed.ibr.hashim@gmail.com> wrote:Hi everyone,
I have been working on this. This is a proposed patch for it so we have
a force option for DROPping the database.
I'd appreciate it if anyone can review.
Hi Ahmed,
Thanks for working on this patch!
+
+ int force;That extra blank line is unnecessary.
Using the bool data type, instead of int, for this option would've
more natural.+ if (ropt->force){
Postgres coding style is to place the curly braces on a new line,
by themselves.+ char *dropStmt = pg_strdup(te->dropStmt);
See if you can use pnstrdup(). Using that may obviate the need for
doing the null-placement acrobatics below.+ PQExpBuffer ftStmt = createPQExpBuffer();
What does the 'ft' stand for in this variable's name?
+ dropStmt[strlen(dropStmt) - 2] = ' '; + dropStmt[strlen(dropStmt) - 1] = '\0';Try to evaluate the strlen() once and reuse it.
+ appendPQExpBufferStr(ftStmt, dropStmt); + appendPQExpBufferStr(ftStmt, "WITH(FORCE);"); + te->dropStmt = ftStmt->data; + } +Remove the extra trailing whitespace on that last blank line.
I think this whole code block needs to be protected by an 'if
(ropt->createDB)' check, like it's done about 20 lines above this
hunk. Otherwise, we may be appending 'WITH (FORCE)' for the DROP
command of a different (not a database) object type.Also, you may want to check that the target database version is
one that supports WITH force option. This command will fail for
anything before v13.The patch needs doc changes (pg_restore.sgml). And it needs to
mention --force option in the help output, as well (usage() function).Can you please see if you can add appropriate test case for this.
The committers may insist on it, when reviewing.Here are a couple of helpful links on how to prepare and submit
patches to the community. You may not need to strictly adhere to
these, but try to pick up a few recommendations that would make the
reviewer's job a bit easier.[1]: https://wiki.postgresql.org/wiki/Creating_Clean_Patches
[2]: https://wiki.postgresql.org/wiki/Submitting_a_PatchBest regards,
Gurjeet
http://Gurje.et
Attachments:
v3-force.patchtext/x-patch; charset=US-ASCII; name=v3-force.patchDownload
diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index 47bd7dbda0..f24e06fdf7 100644
--- a/doc/src/sgml/ref/pg_restore.sgml
+++ b/doc/src/sgml/ref/pg_restore.sgml
@@ -726,6 +726,15 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--force</option></term>
+ <listitem>
+ <para>
+ Force database to drop while restoring if there are any connections.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index aba780ef4b..2d167b58bf 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -154,6 +154,7 @@ typedef struct _restoreOptions
int enable_row_security;
int sequence_data; /* dump sequence data even in schema-only mode */
int binary_upgrade;
+ bool force;
} RestoreOptions;
typedef struct _dumpOptions
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 39ebcfec32..218d440e35 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -532,6 +532,15 @@ RestoreArchive(Archive *AHX)
*/
if (*te->dropStmt != '\0')
{
+ if (ropt->createDB && ropt->force)
+ {
+ int queryLen = strlen(te->dropStmt);
+ char *dropStmt = pnstrdup(te->dropStmt, queryLen - 2);
+ PQExpBuffer newStmt = createPQExpBuffer();
+ appendPQExpBufferStr(newStmt, dropStmt);
+ appendPQExpBufferStr(newStmt, " WITH(FORCE);");
+ te->dropStmt = newStmt->data;
+ }
if (!ropt->if_exists ||
strncmp(te->dropStmt, "--", 2) == 0)
{
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 5dab1ba9ea..5041092ef9 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1139,6 +1139,7 @@ help(const char *progname)
printf(_(" -w, --no-password never prompt for password\n"));
printf(_(" -W, --password force password prompt (should happen automatically)\n"));
printf(_(" --role=ROLENAME do SET ROLE before dump\n"));
+ printf(_(" --force Force database to drop if there are other connections\n"));
printf(_("\nIf no database name is supplied, then the PGDATABASE environment\n"
"variable value is used.\n\n"));
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 049a100634..d3bc68aa20 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -74,6 +74,7 @@ main(int argc, char **argv)
static int no_security_labels = 0;
static int no_subscriptions = 0;
static int strict_names = 0;
+ static bool force = 0;
struct option cmdopts[] = {
{"clean", 0, NULL, 'c'},
@@ -123,6 +124,7 @@ main(int argc, char **argv)
{"no-publications", no_argument, &no_publications, 1},
{"no-security-labels", no_argument, &no_security_labels, 1},
{"no-subscriptions", no_argument, &no_subscriptions, 1},
+ {"force", no_argument, &force, 1},
{NULL, 0, NULL, 0}
};
@@ -351,12 +353,30 @@ main(int argc, char **argv)
opts->no_publications = no_publications;
opts->no_security_labels = no_security_labels;
opts->no_subscriptions = no_subscriptions;
+ opts->force = force;
if (if_exists && !opts->dropSchema)
pg_fatal("option --if-exists requires option -c/--clean");
opts->if_exists = if_exists;
opts->strict_names = strict_names;
+ if(opts->force)
+ {
+ int currentMajorVersion = 0;
+ int ptr = 0, totalVersionLen = strlen(PG_VERSION);
+ while(ptr < totalVersionLen && PG_VERSION[ptr] >= '0' && PG_VERSION[ptr] <= '9')
+ {
+ currentMajorVersion = currentMajorVersion * 10 + (PG_VERSION[ptr] - '0');
+ ptr++;
+ }
+
+ if(currentMajorVersion < 13)
+ {
+ pg_log_error("option --force cannot be used with postgres versions less than 13");
+ exit_nicely(1);
+ }
+ }
+
if (opts->formatName)
{
switch (opts->formatName[0])
On 06.08.23 21:39, Ahmed Ibrahim wrote:
I have addressed the pg version compatibility with the FORCE option in
drop. Here is the last version of the patch
The patch is pretty small, but I think there is some disagreement
whether we want this option at all? Maybe some more people can make
their opinions more explicit?
On 20 Sep 2023, at 11:24, Peter Eisentraut <peter@eisentraut.org> wrote:
On 06.08.23 21:39, Ahmed Ibrahim wrote:
I have addressed the pg version compatibility with the FORCE option in drop. Here is the last version of the patch
The patch is pretty small, but I think there is some disagreement whether we want this option at all? Maybe some more people can make their opinions more explicit?
My my concern is that a --force parameter conveys to the user that it's a big
hammer to override things and get them done, when in reality this doesn't do
that. Taking the example from the pg_restore documentation which currently has
a dropdb step:
====
:~ $ ./bin/createdb foo
:~ $ ./bin/psql -c "create table t(a int);" foo
CREATE TABLE
:~ $ ./bin/pg_dump --format=custom -f foo.dump foo
:~ $ ./bin/pg_restore -d foo -C -c --force foo.dump
pg_restore: error: could not execute query: ERROR: cannot drop the currently open database
Command was: DROP DATABASE foo WITH(FORCE);
pg_restore: error: could not execute query: ERROR: database "foo" already exists
Command was: CREATE DATABASE foo WITH TEMPLATE = template0 ENCODING = 'UTF8' LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8';
pg_restore: error: could not execute query: ERROR: relation "t" already exists
Command was: CREATE TABLE public.t (
a integer
);
pg_restore: warning: errors ignored on restore: 3
====
Without knowing internals, I would expect an option named --force to make that
just work, especially given the documentation provided in this patch. I think
the risk for user confusion outweighs the benefits, or maybe I'm just not smart
enough to see all the benefits? If so, I would argue that more documentation
is required.
Skimming the patch itself, it updates the --help output with --force for
pg_dump and not for pg_restore. Additionally it produces a compilerwarning:
pg_restore.c:127:26: warning: incompatible pointer types initializing 'int *' with an expression of type 'bool *' [-Wincompatible-pointer-types]
{"force", no_argument, &force, 1},
^~~~~~
1 warning generated.
--
Daniel Gustafsson
On Wed, 20 Sept 2023 at 17:27, Daniel Gustafsson <daniel@yesql.se> wrote:
On 20 Sep 2023, at 11:24, Peter Eisentraut <peter@eisentraut.org> wrote:
On 06.08.23 21:39, Ahmed Ibrahim wrote:
I have addressed the pg version compatibility with the FORCE option in drop. Here is the last version of the patch
The patch is pretty small, but I think there is some disagreement whether we want this option at all? Maybe some more people can make their opinions more explicit?
My my concern is that a --force parameter conveys to the user that it's a big
hammer to override things and get them done, when in reality this doesn't do
that. Taking the example from the pg_restore documentation which currently has
a dropdb step:====
:~ $ ./bin/createdb foo
:~ $ ./bin/psql -c "create table t(a int);" foo
CREATE TABLE
:~ $ ./bin/pg_dump --format=custom -f foo.dump foo
:~ $ ./bin/pg_restore -d foo -C -c --force foo.dump
pg_restore: error: could not execute query: ERROR: cannot drop the currently open database
Command was: DROP DATABASE foo WITH(FORCE);
pg_restore: error: could not execute query: ERROR: database "foo" already exists
Command was: CREATE DATABASE foo WITH TEMPLATE = template0 ENCODING = 'UTF8' LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8';pg_restore: error: could not execute query: ERROR: relation "t" already exists
Command was: CREATE TABLE public.t (
a integer
);pg_restore: warning: errors ignored on restore: 3
====Without knowing internals, I would expect an option named --force to make that
just work, especially given the documentation provided in this patch. I think
the risk for user confusion outweighs the benefits, or maybe I'm just not smart
enough to see all the benefits? If so, I would argue that more documentation
is required.Skimming the patch itself, it updates the --help output with --force for
pg_dump and not for pg_restore. Additionally it produces a compilerwarning:pg_restore.c:127:26: warning: incompatible pointer types initializing 'int *' with an expression of type 'bool *' [-Wincompatible-pointer-types]
{"force", no_argument, &force, 1},
^~~~~~
1 warning generated.
I have changed the status of the patch to "Returned with Feedback" as
the comments have not been addressed for some time. Please feel free
to address these issues and update commitfest accordingly.
Regards,
Vignesh