Re: dropdb --force

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

> On Wed, Oct 2, 2019 at 10:21 PM Pavel Stehule <pavel.stehule@gmail.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
>