Re: There should be a way to use the force flag when restoring databases - Mailing list pgsql-hackers

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