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

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(dot)ibr(dot)hashim(at)gmail(dot)com>
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 <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
>>
>