Re: dropdb --force

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> [ drop-database-force-20190708.patch ]

I took a brief look at this, but I don't think it's really close to
being committable.

* The documentation claims FORCE will fail if you don't have privileges
to terminate the other session(s) in the target DB. This is a lie; the
code issues kill() without any regard for such niceties. You could
perhaps make that better by going through pg_signal_backend().

* You've hacked CountOtherDBBackends to do something that's completely
outside the charter one would expect from its name, and not even
bothered to update its header comment. This requires more attention
to not confusing future hackers; I'd say you can't even use that
function name anymore.

* You've also ignored the existing code in CountOtherDBBackends
that is careful *not* to issue a kill() while holding the critical
ProcArrayLock. That problem would get enormously worse if you
tried to sub in pg_signal_backend() there, since that function
may do catalog accesses --- it's pretty likely that you could
get actual deadlocks, never mind just trashing performance.

* I really dislike the addition of more hard-wired delays and
timeouts to dropdb(). It's bad enough that CountOtherDBBackends
has got that. Two layers of timeout are a rickety mess, and
who's to say that a 1-minute timeout is appropriate for anything?

* I'm concerned that the proposed syntax is not future-proof.
FORCE is not a reserved word, and we surely don't want to make
it one; but just appending it to the end of the command without
any decoration seems like a recipe for problems if anybody wants
to add other options later. (Possible examples: RESTRICT/CASCADE,
or a user-defined timeout.) Maybe some parentheses would help?
Or possibly I'm being overly paranoid, but ...

I hadn't been paying any attention to this thread before now,
but I'd assumed from the thread title that the idea was to implement
any attempted kills in the dropdb app, not on the backend side.
(As indeed it looks like the first version did.) Maybe it would be
better to go back to that, instead of putting dubious behaviors
into the core server.

regards, tom lane