Re: dropdb --force

čt 14. 11. 2019 v 12:12 odesílatel vignesh C <vignesh21@gmail.com> napsal:

> On Wed, Nov 13, 2019 at 8:05 PM Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> >
> >
> >
> > st 13. 11. 2019 v 7:13 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
> napsal:
> >>
> >>
> >>
> >> st 13. 11. 2019 v 7:12 odesílatel Amit Kapila <amit.kapila16@gmail.com>
> napsal:
> >>>
> >>> On Tue, Nov 12, 2019 at 11:17 AM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >>> >
> >>> > I am planning to commit this patch tomorrow unless I see more
> comments
> >>> > or interest from someone else to review this.
> >>> >
> >>>
> >>> Pushed.  Pavel, feel free to submit dropdb utility-related patch if
> you want.
> >>
> >>
> >> I hope I send this patch today. It's simply job.
> >
> >
> > here it is. It's based on Filip Rembialkowski's patch if I remember
> correctly
> >
>
> Thanks for working on the patch.
> Few minor comments:
> +        Force termination of connected backends before removing the
> database.
> +       </para>
> +       <para>
> +        This will add the <literal>FORCE</literal> option to the
> <literal>DROP
> +        DATABASE</literal> command sent to the server.
> +       </para>
>
> The above documentation can be changed similar to drop_database.sgml:
>      <para>
>       Attempt to terminate all existing connections to the target database.
>       It doesn't terminate if prepared transactions, active logical
> replication
>       slots or subscriptions are present in the target database.
>      </para>
>      <para>
>       This will fail if the 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.
>      </para>
>
>
done


> We can make the modification in the same location as earlier in the below
> case:
> -    appendPQExpBuffer(&sql, "DROP DATABASE %s%s;",
> -                      (if_exists ? "IF EXISTS " : ""), fmtId(dbname));
> -
>      /* Avoid trying to drop postgres db while we are connected to it. */
>      if (maintenance_db == NULL && strcmp(dbname, "postgres") == 0)
>          maintenance_db = "template1";
> @@ -134,6 +136,12 @@ main(int argc, char *argv[])
>                                        host, port, username,
> prompt_password,
>                                        progname, echo);
>
> +    /* now, only FORCE option can be used, so usage is very simple */
> +    appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
> +                      (if_exists ? "IF EXISTS " : ""),
> +                      fmtId(dbname),
> +                      force ? " WITH (FORCE)" : "");
> +
>
>
done


> We can slightly rephrase the below:
> +    printf(_("  -f, --force               force termination of
> connected backends\n"));
> can be changed to:
> +    printf(_("  -f, --force               terminate the existing
> connections to the target database forcefully\n"));
>

the proposed text is too long - I changed the sentence, and any other can
fix it


> We can slightly rephrase the below:
> +    /* now, only FORCE option can be used, so usage is very simple */
> +    appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
> can be changed to:
> +    /* Generate drop db command using the options specified */
> +    appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
>

I would to say, so generating is simple due only one supported option. If
we support two or more options there, then the code should be more complex.
I changed comment to

/* Currently, only FORCE option is supported */

updated patch attached

Thank you for review

Pavel


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