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