Re: dropdb --force

On Thu, Sep 26, 2019 at 7:18 PM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
wrote:

>
> út 24. 9. 2019 v 14:52 odesílatel Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> napsal:
>
>>
>> One other minor comment:
>> +
>> + This will also fail, if the connections do not terminate in 5
>> seconds.
>> + </para>
>>
>> Is there any implementation in the patch for the above note?
>>
>
> Yes, is there.
>
> The force flag ensure sending SIGTERM to related clients. Nothing more.
> There are still check
>
> -->if (CountOtherDBBackends(db_id, &notherbackends, &npreparedxacts))
> <--><-->ereport(ERROR,
> <--><--><--><-->(errcode(ERRCODE_OBJECT_IN_USE),
> <--><--><--><--> errmsg("database \"%s\" is being accessed by other users",
> <--><--><--><--><--><-->dbname),
> <--><--><--><--> errdetail_busy_db(notherbackends, npreparedxacts)));
>
> that can fails after 5 sec. Sending signal doesn't ensure nothing, so I am
> for no changes in these check.
>

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.

One more point I would like to add here is that I think it is worth
> considering to split this patch by keeping the changes in dropdb
> utility as a separate patch. Even though the code is not very much
> but I think it can be a separate patch atop the main patch which
> contains the core server changes.
>

> I did it - last patch contains server side only. I expect so client side
(very small patch) can be nex

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'.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com