Re: dropdb --force

ne 6. 10. 2019 v 10:19 odesílatel vignesh C <vignesh21(at)gmail(dot)com> napsal:

> On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >
> >
> >
> > č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)
> >
> Changes in this patch looks fine to me.
> I'm not sure if we have forgotten to miss attaching the second patch
> or can you provide the link to second patch.
>

I plan to work on the second patch after committing of this first. Now we
are early on commit fest, and the complexity of this or second patch is not
too high be necessary to prepare patch series.

Regards

Pavel

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