On Tue, Sep 24, 2019 at 6:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> >
> > Thank you for check. I am sending updated patch
> >
>
> Alvaro has up thread suggested some alternative syntax [1] for this
> patch, but I don't see any good argument to not go with what he has
> proposed. In other words, why we should prefer the current syntax as
> in the patch over what Alvaro has proposed?
>
> IIUC, the current syntax implemented by the patch is:
> Drop Database [(options)] [If Exists] name
> Alvaro suggested using options at the end (and use optional keyword
> WITH) based on what other Drop commands does. I see some merits to
> that idea which are (a) if tomorrow we want to introduce new options
> like CASCADE, RESTRICT then it will be better to have all the options
> at the end as we have for other Drop commands, (b) It will resemble
> more with Create Database syntax.
>
> Now, I think the current syntax is also not bad and we already do
> something like that for other commands like Vaccum where options are
> provided before object_name, but I think in this case putting at the
> end is more appealing unless there are some arguments against that.
>
> One other minor comment:
> +
> + This will also fail, if the connections do not terminate in 5 seconds.
> + </para>
>
> Is there any implementation in the patch for the above note?
>
One more point I would like to add here is that I think it is worth
considering to split this patch by keeping the changes in dropdb
utility as a separate patch. Even though the code is not very much
but I think it can be a separate patch atop the main patch which
contains the core server changes.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com