On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule <pavel.steh...@gmail.com> wrote: > >> >> Can we add few tests for this feature. > > there are not any other test for DROP DATABASE >
I think there are no dedicated tests for it but in a few tests, we use it like in contrib\sepgsql\sql\alter.sql. I am not sure if we can write a predictable test for force option because it will never be guaranteed to drop the database in the presence of other active sessions. Few more comments: --------------------------------- 1. + if (nprepared > 0) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_IN_USE), + errmsg("database \"%s\" is used by %d prepared transaction(s)", + get_database_name(databaseId), + nprepared))); + You need to use errdetail_plural here to avoid translation problems, see docs[1] for a detailed explanation. You can use function errdetail_busy_db. Also, the indentation is not proper. 2. TerminateOtherDBBackends() { .. + /* We know so we have all necessary rights now */ + foreach (lc, pids) + { + int pid = lfirst_int(lc); + PGPROC *proc = BackendPidGetProc(pid); + + if (proc != NULL) + { + /* If we have setsid(), signal the backend's whole process group */ +#ifdef HAVE_SETSID + (void) kill(-pid, SIGTERM); +#else + (void) kill(pid, SIGTERM); +#endif + } + } + + /* sleep 100ms */ + pg_usleep(100 * 1000L); .. } So here we are sending SIGTERM to all the processes and wait for 100ms to allow them to exit. Have you tested this with many processes? I think we can create 100~500 sessions or maybe more to the database being dropped and see what is behavior? One thing to notice is that in function CountOtherDBBackends() we are sending SIGTERM to 10 autovacuum processes at-a-time. That has been introduced in commit 4abd7b49f1e9, but the reason for the same is not mentioned. I am not sure if it is to avoid sending SIGTERM to many processes in quick succession. I think there should be more comments atop TerminateOtherDBBackends to explain the working of it and some assumptions of that function. 3. +opt_drop_option_list: + opt_with '(' drop_option_list ')' { $$ = $3; } + | /* EMPTY */ I think it is better to keep opt_with as part of the main syntax rather than clubbing it with drop_option_list as we have in other cases in the code. 4. +drop_option: FORCE + { + $$ = makeDefElem("force", NULL, @1); + } + ; We generally keep the option name "FORCE" in the new line. 5. I think it is better if can support tab-completion for this feature. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com