Re: dropdb --force

pá 20. 9. 2019 v 7:58 odesílatel Dilip Kumar <dilipbalaut(at)gmail(dot)com> napsal:

> On Thu, Sep 19, 2019 at 12:14 AM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >
> > Hi
> >
> > I am sending updated version - the changes against last patch are two. I
> use pg_terminate_backed for killing other terminates like Tom proposed. I
> am not sure if it is 100% practical - on second hand the necessary right to
> kill other sessions is almost available - and consistency with
> pg_terminal_backend has sense. More - native implementation is
> significantly better then solution implemented outer. I fixed docs little
> bit - the timeout for logout (as reaction on SIGTERM is only 5 sec).
> >
>
> Few comments on the patch.
>
> @@ -598,10 +598,20 @@ standard_ProcessUtility(PlannedStmt *pstmt,
> case T_DropdbStmt:
> {
> DropdbStmt *stmt = (DropdbStmt *) parsetree;
> + bool force = false;
> + ListCell *lc;
> +
> + foreach(lc, stmt->options)
> + {
> + DefElem *item = (DefElem *) lfirst(lc);
> +
> + if (strcmp(item->defname, "force") == 0)
> + force = true;
> + }
>
> /* no event triggers for global objects */
> PreventInTransactionBlock(isTopLevel, "DROP DATABASE");
> - dropdb(stmt->dbname, stmt->missing_ok);
> + dropdb(stmt->dbname, stmt->missing_ok, force);
>
> If you see the createdb, then options are processed inside the
> createdb function but here we are processing outside
> the function. Wouldn't it be good to keep them simmilar and also it
> will be expandable in case we add more options
> in the future?
>
>
I though about it, but I think so current state is good enough. There are
only few parameters - and I don't think significantly large number of new
options.

When number of parameters of any functions is not too high, then I think so
is better to have a function with classic list of parameters instead
function with one parameter of some struct type.

If somebody try to use function dropdb from outside (extension), then he
don't need to create new structure, new list, and simply call simple C API.
I prefer API of simple types against structures and nodes there. Just I
think so it's more practical of somebody try to use this function from
other places than from parser.

>
> initPQExpBuffer(&sql);
>
> - 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,10 @@ main(int argc, char *argv[])
> host, port, username, prompt_password,
> progname, echo);
> + appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
> + (force ? " (FORCE) " : ""),
> + (if_exists ? "IF EXISTS " : ""), fmtId(dbname));
> +
>
> I did not understand why you have moved this code after
> connectMaintenanceDatabase function? I don't see any problem but in
> earlier code
> initPQExpBuffer and appendPQExpBuffer were together now you have moved
> appendPQExpBuffer after the connectMaintenanceDatabase so if there is
> no reason to do that then better to keep it how it was earlier.
>
>
I am not a author of this change, but it is not necessary and I returned
original order

> -extern bool CountOtherDBBackends(Oid databaseId,
> - int *nbackends, int *nprepared);
> +extern bool CountOtherDBBackends(Oid databaseId, int *nbackends, int
> *nprepared);
>
> Unrelated change
>

fixed

Thank you for check. I am sending updated patch

Regards

Pavel

> --
> Regards,
> Dilip Kumar
> EnterpriseDB: http://www.enterprisedb.com
>