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 <[email protected]> wrote:
> On Sun, Jul 23, 2023 at 6:09 AM Ahmed Ibrahim
> <[email protected]> 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
>
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");
