Re: dropdb --force

čt 3. 10. 2019 v 19:48 odesílatel vignesh C <vignesh21(at)gmail(dot)com> napsal:

> On Wed, Oct 2, 2019 at 10:21 PM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >
> > Thank you for careful review. I hope so all issues are out.
> >
> >
> Thanks Pavel for fixing the comments.
> Few comments:
> The else part cannot be hit in DropDatabase function as gram.y expects
> FORCE.
> +
> + if (strcmp(opt->defname, "force") == 0)
> + force = true;
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("unrecognized DROP DATABASE option \"%s\"", opt->defname),
> + parser_errposition(pstate, opt->location)));
> + }
> +
>

I know - but somebody can call DropDatabase function outside parser. So is
better check all possibilities.

> We should change gram.y to accept any keyword and throw error from
> DropDatabase function.
> + */
> +drop_option: FORCE
> + {
> + $$ = makeDefElem("force", NULL, @1);
> + }
> + ;
>

I spent some time with thinking about it, and I think so this variant (with
keyword) is well readable and very illustrative. This will be lost with
generic variant.

When the keyword FORCE already exists, then I prefer current state.

>
> "This will also fail" should be "This will fail"
> + <para>
> + This will fail, if current user has no permissions to terminate
> other
> + connections. Required permissions are the same as with
> + <literal>pg_terminate_backend</literal>, described
> + in <xref linkend="functions-admin-signal"/>.
> +
> + This will also fail if we are not able to terminate connections or
> + when there are active prepared transactions or active logical
> replication
> + slots.
> + </para>
>

fixed

>
> Can we add few tests for this feature.
>

there are not any other test for DROP DATABASE

We can check syntax later inside second patch (for -f option of dropdb
command)

Regards

Pavel

>
> Regards,
> Vignesh
> EnterpriseDB: http://www.enterprisedb.com
>