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

On Tue, Jul 18, 2023 at 12:53 AM Joan <aseques@gmail.com> wrote:
>
> Since posgres 13 there's the option to do a FORCE when dropping a database (so it disconnects current users)
Documentationhere: https://www.postgresql.org/docs/current/sql-dropdatabase.html 
>
> I am currently using dir format for the output
>    pg_dump -d "bdname" -F d -j 4 -v -f /tmp/dir
>
> And restoring the database with
>   pg_restore -d postgres -C -c --exit-on-error -F d -j 3 -v /tmp/dir
>
> Having an option to add the FORCE option to either the generated dump by pg_dump, or in the pg_restore would be very
usefulwhen restoring the databases to another servers so it would avoid having to do scripting. 
>
> In my specific case I am using this to refresh periodically a development environment with data from production
serversfor a small database (~200M). 

Making force-drop a part of pg_dump output may be dangerous, and not
provide much flexibility at restore time.

Adding a force option to pg_restore feels like providing the right tradeoff.

The docs for 'pg_restore --create` say "Create the database before
restoring into it. If --clean is also specified, drop and recreate the
target database before connecting to it."

If we provided a force option, it may then additionally say: "If the
--clean and --force options are specified, DROP DATABASE ... WITH
FORCE command will be used to drop the database."

Using WITH FORCE is not a guarantee, as the DROP DATABASE docs clarify
the conditions under which it may fail.

Best regards,
Gurjeet
http://Gurje.et

> On 19 Jul 2023, at 19:28, Gurjeet Singh <gurjeet@singh.im> wrote:
>
> On Tue, Jul 18, 2023 at 12:53 AM Joan <aseques@gmail.com> wrote:
>>
>> Since posgres 13 there's the option to do a FORCE when dropping a database (so it disconnects current users)
Documentationhere: https://www.postgresql.org/docs/current/sql-dropdatabase.html 
>>
>> I am currently using dir format for the output
>>   pg_dump -d "bdname" -F d -j 4 -v -f /tmp/dir
>>
>> And restoring the database with
>>  pg_restore -d postgres -C -c --exit-on-error -F d -j 3 -v /tmp/dir
>>
>> Having an option to add the FORCE option to either the generated dump by pg_dump, or in the pg_restore would be very
usefulwhen restoring the databases to another servers so it would avoid having to do scripting. 
>>
>> In my specific case I am using this to refresh periodically a development environment with data from production
serversfor a small database (~200M). 
>
> Making force-drop a part of pg_dump output may be dangerous, and not
> provide much flexibility at restore time.
>
> Adding a force option to pg_restore feels like providing the right tradeoff.
>
> The docs for 'pg_restore --create` say "Create the database before
> restoring into it. If --clean is also specified, drop and recreate the
> target database before connecting to it."
>
> If we provided a force option, it may then additionally say: "If the
> --clean and --force options are specified, DROP DATABASE ... WITH
> FORCE command will be used to drop the database."

pg_restore --clean refers to dropping any pre-existing database objects and not
just databases, but --force would only apply to databases.

I wonder if it's worth complicating pg_restore with that when running dropdb
--force before pg_restore is an option for those wanting to use WITH FORCE.

--
Daniel Gustafsson

On Thu, Jul 20, 2023 at 2:10 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 19 Jul 2023, at 19:28, Gurjeet Singh <gurjeet@singh.im> wrote:
> >
> > The docs for 'pg_restore --create` say "Create the database before
> > restoring into it. If --clean is also specified, drop and recreate the
> > target database before connecting to it."
> >
> > If we provided a force option, it may then additionally say: "If the
> > --clean and --force options are specified, DROP DATABASE ... WITH
> > FORCE command will be used to drop the database."
>
> pg_restore --clean refers to dropping any pre-existing database objects and not
> just databases, but --force would only apply to databases.
>
> I wonder if it's worth complicating pg_restore with that when running dropdb
> --force before pg_restore is an option for those wanting to use WITH FORCE.

Fair point. But the same argument could've been applied to --clean
option, as well; why overload the meaning of --clean and make it drop
database, when a dropdb before pg_restore was an option.

IMHO, if pg_restore offers to drop database, providing an option to
the user to do it forcibly is not that much of a stretch, and within
reason for the user to expect it to be there, like Joan did.

Best regards,
Gurjeet
http://Gurje.et

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

On 06.08.23 21:39, Ahmed Ibrahim wrote:
> I have addressed the pg version compatibility with the FORCE option in 
> drop. Here is the last version of the patch

The patch is pretty small, but I think there is some disagreement 
whether we want this option at all?  Maybe some more people can make 
their opinions more explicit?

> On 20 Sep 2023, at 11:24, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 06.08.23 21:39, Ahmed Ibrahim wrote:
>> I have addressed the pg version compatibility with the FORCE option in drop. Here is the last version of the patch
>
> The patch is pretty small, but I think there is some disagreement whether we want this option at all?  Maybe some
morepeople can make their opinions more explicit? 

My my concern is that a --force parameter conveys to the user that it's a big
hammer to override things and get them done, when in reality this doesn't do
that.  Taking the example from the pg_restore documentation which currently has
a dropdb step:

====
:~ $ ./bin/createdb foo
:~ $ ./bin/psql -c "create table t(a int);" foo
CREATE TABLE
:~ $ ./bin/pg_dump --format=custom -f foo.dump foo
:~ $ ./bin/pg_restore -d foo -C -c --force foo.dump
pg_restore: error: could not execute query: ERROR:  cannot drop the currently open database
Command was: DROP DATABASE foo WITH(FORCE);
pg_restore: error: could not execute query: ERROR:  database "foo" already exists
Command was: CREATE DATABASE foo WITH TEMPLATE = template0 ENCODING = 'UTF8' LOCALE_PROVIDER = libc LOCALE =
'en_US.UTF-8';


pg_restore: error: could not execute query: ERROR:  relation "t" already exists
Command was: CREATE TABLE public.t (
    a integer
);


pg_restore: warning: errors ignored on restore: 3
====

Without knowing internals, I would expect an option named --force to make that
just work, especially given the documentation provided in this patch.  I think
the risk for user confusion outweighs the benefits, or maybe I'm just not smart
enough to see all the benefits?  If so, I would argue that more documentation
is required.

Skimming the patch itself, it updates the --help output with --force for
pg_dump and not for pg_restore.  Additionally it produces a compilerwarning:

pg_restore.c:127:26: warning: incompatible pointer types initializing 'int *' with an expression of type 'bool *'
[-Wincompatible-pointer-types]
                {"force", no_argument, &force, 1},
                                       ^~~~~~
1 warning generated.

--
Daniel Gustafsson

On Wed, 20 Sept 2023 at 17:27, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 20 Sep 2023, at 11:24, Peter Eisentraut <peter@eisentraut.org> wrote:
> >
> > On 06.08.23 21:39, Ahmed Ibrahim wrote:
> >> I have addressed the pg version compatibility with the FORCE option in drop. Here is the last version of the
patch
> >
> > The patch is pretty small, but I think there is some disagreement whether we want this option at all?  Maybe some
morepeople can make their opinions more explicit?
 
>
> My my concern is that a --force parameter conveys to the user that it's a big
> hammer to override things and get them done, when in reality this doesn't do
> that.  Taking the example from the pg_restore documentation which currently has
> a dropdb step:
>
> ====
> :~ $ ./bin/createdb foo
> :~ $ ./bin/psql -c "create table t(a int);" foo
> CREATE TABLE
> :~ $ ./bin/pg_dump --format=custom -f foo.dump foo
> :~ $ ./bin/pg_restore -d foo -C -c --force foo.dump
> pg_restore: error: could not execute query: ERROR:  cannot drop the currently open database
> Command was: DROP DATABASE foo WITH(FORCE);
> pg_restore: error: could not execute query: ERROR:  database "foo" already exists
> Command was: CREATE DATABASE foo WITH TEMPLATE = template0 ENCODING = 'UTF8' LOCALE_PROVIDER = libc LOCALE =
'en_US.UTF-8';
>
>
> pg_restore: error: could not execute query: ERROR:  relation "t" already exists
> Command was: CREATE TABLE public.t (
>     a integer
> );
>
>
> pg_restore: warning: errors ignored on restore: 3
> ====
>
> Without knowing internals, I would expect an option named --force to make that
> just work, especially given the documentation provided in this patch.  I think
> the risk for user confusion outweighs the benefits, or maybe I'm just not smart
> enough to see all the benefits?  If so, I would argue that more documentation
> is required.
>
> Skimming the patch itself, it updates the --help output with --force for
> pg_dump and not for pg_restore.  Additionally it produces a compilerwarning:
>
> pg_restore.c:127:26: warning: incompatible pointer types initializing 'int *' with an expression of type 'bool *'
[-Wincompatible-pointer-types]
>                 {"force", no_argument, &force, 1},
>                                        ^~~~~~
> 1 warning generated.

I have changed the status of the patch to "Returned with Feedback" as
the comments have not been addressed for some time. Please feel free
to address these issues and update commitfest accordingly.

Regards,
Vignesh