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 <[email protected]>
wrote:
> 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..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])
