Re: dropdb --force

On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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