Re: dropdb --force - Mailing list pgsql-hackers

I think 5 seconds is a hard-coded value that can even change in the future.  So, it is better to write something more generic like "This will also fail if we are not able to terminate connections" or something like that.  This part of the documentation might change after addressing the other comments below.

I still see the code related to dropdb utility in the patch.  See,
diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
index dacd8e5f1d..1bb80fda74 100644
--- a/src/bin/scripts/dropdb.c
+++ b/src/bin/scripts/dropdb.c
@@ -34,6 +34,7 @@ main(int argc, char *argv[])
  {"interactive", no_argument, NULL, 'i'},
  {"if-exists", no_argument, &if_exists, 1},
  {"maintenance-db", required_argument, NULL, 2},
+ {"force", no_argument, NULL, 'f'},
  {NULL, 0, NULL, 0}
  };
 

Few other comments:

--------------------------------

1.
+void
+TerminateOtherDBBackends(Oid databaseId)
+{
+ ProcArrayStruct *arrayP = procArray;
+ List   *pids = NIL;
+ int i;
+
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
+
+ for (i = 0; i < procArray->numProcs; i++)
+ {
+ int pgprocno = arrayP->pgprocnos[i];
+ PGPROC   *proc = &allProcs[pgprocno];
+
+ if (proc->databaseId != databaseId)
+ continue;
+ if (proc == MyProc)
+ continue;
+
+ if (proc->pid != 0)
+ pids = lappend_int(pids, proc->pid);
+ }

So here we are terminating only connection which doesn't have any prepared transaction.  The behavior of this patch with the prepared transactions will be terrible. Basically, after terminating all the connections via TerminateOtherDBBackends, we will give error once CountOtherDBBackends is invoked.  I have tested the same and it gives an error like below:

postgres=# drop database db1 With (Force);
ERROR:  database "db1" is being accessed by other users
DETAIL:  There is 1 prepared transaction using the database.

I think we have two options here:
(a) Document that even with force option, if there are any prepared transactions in the same database, we won't drop the database.  Along with this, fix the code such that we don't terminate other connections if the prepared transactions are active.
(b) Rollback the prepared transactions.

I think (a) is a better option because if the number of prepared transactions is large, then it might take time and I am not sure if it is worth to add the complexity of rolling back all the prepared xacts.  OTOH, if you or others think option (b) is good and doesn't add much complexity, then I think it is worth exploring that option.

I think we should definitely do something to deal with this even if you don't like the proposed options because the current behavior of the patch seems worse than either of these options.

2.
-DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
+DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> [ WITH ( <replaceable class="parameter">option</replaceable> [, ...] ) ]

It is better to keep WITH clause as optional similar to Copy command.  This might address Peter E's concern related to WITH clause.

3.
- * DROP DATABASE [ IF EXISTS ]
+ * DROP DATABASE [ ( options ) ] [ IF EXISTS ]

You seem to forget to change the syntax in the above comments after changing the patch.

4.
+   If anyone else is connected to the target database, this command will fail
+   - unless you use the <literal>FORCE</literal> option described below.

I don't understand the significance of using '-' before unless.  I think we can remove it.

Changed the patch status as 'Waiting on Author'.