Re: There should be a way to use the force flag when restoring databases

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(at)singh(dot)im> wrote:

> On Sun, Jul 23, 2023 at 6:09 AM Ahmed Ibrahim
> <ahmed(dot)ibr(dot)hashim(at)gmail(dot)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
>