Pavel Stehule <pavel.steh...@gmail.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