dropdb --force

Hi

út 18. 12. 2018 v 16:11 odesílatel Filip Rembiałkowski <
filip.rembialkowski@gmail.com> napsal:

Hi,

I propose a simple patch (works-for-me), which adds --force (-f)
option to dropdb utility.

Pros: This seems to be a desired option for many sysadmins, as this
thread proves:
https://dba.stackexchange.com/questions/11893/force-drop-db-while-others-may-be-connected
Cons: another possible foot-gun for the unwary.

Obviously this patch needs some more work (see TODO note inside).

Please share opinions if this makes sense at all, and has any chance
going upstream.

The regression tests is simplistic, please help with an example of
multi-session test so I can follow.

Still one my customer use a patch that implement FORCE on SQL level. It is
necessary under higher load when is not easy to synchronize clients.

Regards

Pavel

Hi

út 18. 12. 2018 v 16:11 odesílatel Filip Rembiałkowski <filip.rembialkowski@gmail.com> napsal:

Please share opinions if this makes sense at all, and has any chance
going upstream.

Clearly since Pavel has another implementation of the same concept,
there is some interest in this feature. :)

On Tue, Dec 18, 2018 at 5:20 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:

Still one my customer use a patch that implement FORCE on SQL level. It is necessary under higher load when is not easy to synchronize clients.

I think Filip's approach of setting pg_database.datallowconn='false'
is pretty clever to avoid the synchronization problem. But it's also a
good idea to expose this functionality via DROP DATABASE in SQL, like
Pavel's patch, not just the 'dropdb' binary.

If this is to be accepted into PostgreSQL core, I think the two
approaches should be combined on the server side.

Regards,
Marti Raudsepp

On Tue, Dec 18, 2018 at 01:25:32PM +0100, Filip Rembiałkowski wrote:

Hi,

I propose a simple patch (works-for-me), which adds --force (-f)
option to dropdb utility.

Nice!

I did something like this in user space back in 2010.

https://people.planetpostgresql.org/dfetter/index.php?/archives/63-Kick-em-out,-and-keep-em-out!.html

so there appears to be interest in such a feature.

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Marti Raudsepp <marti@juffo.org> writes:

I think Filip's approach of setting pg_database.datallowconn='false'
is pretty clever to avoid the synchronization problem.

Some bull-in-a-china-shop has recently added logic that allows ignoring
datallowconn and connecting anyway, so I'm not sure that that'd provide
a production-quality solution. On the other hand, maybe we could revert
BGWORKER_BYPASS_ALLOWCONN.

regards, tom lane

I wonder if this idea from seven years ago might be useful:
/messages/by-id/1305688547-sup-7028@alvh.no-ip.org

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

st 19. 12. 2018 v 16:55 odesílatel Alvaro Herrera <alvherre@2ndquadrant.com>
napsal:

I wonder if this idea from seven years ago might be useful:
/messages/by-id/1305688547-sup-7028@alvh.no-ip.org

Why not - I see this as little bit different case - when I used drop
database force than I didn't need to wait on clients.

The target is clean, but the methods can be different due different
environments, goals and sensitivity

Here is Pavel's patch rebased to master branch, added the dropdb
--force option, a test case & documentation.

I'm willing to work on it if needed. What are possible bad things that
could happen here? Is the documentation clear enough?

Thanks.

On Wed, Mar 6, 2019 at 1:39 PM Filip Rembiałkowski
<filip.rembialkowski@gmail.com> wrote:

Here is Pavel's patch rebased to master branch, added the dropdb
--force option, a test case & documentation.

Hello,

cfbot.cputube.org says this fails on Windows, due to a missing semicolon here:

#ifdef HAVE_SETSID
kill(-(proc->pid), SIGTERM);
#else
kill(proc->pid, SIGTERM)
#endif

The test case failed on Linux, I didn't check why exactly:

Test Summary Report
-------------------
t/050_dropdb.pl (Wstat: 65280 Tests: 13 Failed: 2)
Failed tests: 12-13
Non-zero exit status: 255
Parse errors: Bad plan. You planned 11 tests but ran 13.

+/* Time to sleep after isuing SIGTERM to backends */
+#define TERMINATE_SLEEP_TIME 1

s/isuing/issuing/

But, hmm, this macro doesn't actually seem to be used in the patch.
Wait, is that because the retry loop forgot to actually include the
sleep?

+ /* without "force" flag raise exception immediately, or after
5 minutes */

Normally we call it an "error", not an "exception".

--
Thomas Munro
https://enterprisedb.com

Thank you. Updated patch attached.

Hello,
This is a feature I have wanted for a long time, thank you for your work on
this.
The latest patch [1]/messages/by-id/attachment/99536/drop-database-force-20190310_01.patch applied cleanly for me. In dbcommands.c the comment
references a 5 second delay, I don't see where that happens, am I missing
something?

I tested both the dropdb program and the in database commands. Without
FORCE I get
the expected error message about active connections.

postgres=# DROP DATABASE testdb;
ERROR: source database "testdb" is being accessed by other users
DETAIL: There is 1 other session using the database.

With FORCE the database drops cleanly.

postgres=# DROP DATABASE testdb FORCE;
DROP DATABASE

The active connections get terminated as expected. Thanks,

[1]: /messages/by-id/attachment/99536/drop-database-force-20190310_01.patch
/messages/by-id/attachment/99536/drop-database-force-20190310_01.patch

*Ryan Lambert*
RustProof Labs

On Sun, Mar 10, 2019 at 12:54 PM Filip Rembiałkowski <
filip.rembialkowski@gmail.com> wrote:

Hi,

On 2019-03-10 11:20:42 +0100, Filip Rembiałkowski wrote:

bool
-CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared)
+CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared, bool force_terminate)
{

That doesn't seem like a decent API to me.

Greetings,

Andres Freund

On 31.03.2019, 04:35 Andres Freund <andres@anarazel.de> wrote:

bool
-CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared)
+CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared, bool force_terminate)
{

That doesn't seem like a decent API to me.

Only excuse is that naming was already a bit off, as the function
includes killing autovacuum workers.

Please advise what would be a good approach to improve it. I would
propose something like:

bool CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared);
// make it actually do what the name announces - only the count, no
side effects.

bool KillDBBackends(Oid databaseId, bool killAutovacuum, bool killBackends);
// try to kill off all the backends, return false when there are still any.

Also, there is a question - should the FORCE option rollback prepared
transactions?

Thanks!

Is this the intended behavior? SIGTERM is received.

test=# begin;
BEGIN
test=# create table test(a int);
CREATE TABLE

In another terminal drop the database.

test=# begin;
psql: FATAL: terminating connection due to administrator command
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

Yes, I think it is because of this code Snippet

if (force_terminate)
{
/* try to terminate backend */
#ifdef HAVE_SETSID
kill(-(proc->pid), SIGTERM);
#else
kill(proc->pid, SIGTERM);

On Tue, Apr 9, 2019 at 3:06 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:

Is this the intended behavior? SIGTERM is received.

test=# begin;
BEGIN
test=# create table test(a int);
CREATE TABLE

In another terminal drop the database.

test=# begin;
psql: FATAL: terminating connection due to administrator command
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

--
Ibrar Ahmed

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested

The feature works fine on my machine. The code is well-written.

The new status of this patch is: Ready for Committer

Also works fine according to my testing. Documentation is also clear.
Thanks for this useful patch.

Hi,
patch no longer applies (as of 12beta2).

postgres@ubuntudev:~/pg_testing/source/postgresql-12beta2$ patch -p1 < drop-database-force-20190310_01.patch
patching file doc/src/sgml/ref/drop_database.sgml
patching file doc/src/sgml/ref/dropdb.sgml
patching file src/backend/commands/dbcommands.c
Hunk #1 succeeded at 489 (offset 2 lines).
Hunk #2 succeeded at 779 (offset 2 lines).
Hunk #3 succeeded at 787 (offset 2 lines).
Hunk #4 succeeded at 871 (offset 2 lines).
Hunk #5 succeeded at 1056 (offset 2 lines).
Hunk #6 succeeded at 1186 (offset 2 lines).
patching file src/backend/nodes/copyfuncs.c
Hunk #1 succeeded at 3852 (offset 10 lines).
patching file src/backend/nodes/equalfuncs.c
Hunk #1 succeeded at 1666 (offset 3 lines).
patching file src/backend/parser/gram.y
Hunk #1 succeeded at 10194 (offset 45 lines).
Hunk #2 succeeded at 10202 (offset 45 lines).
patching file src/backend/storage/ipc/procarray.c
Hunk #1 succeeded at 2906 (offset -14 lines).
Hunk #2 succeeded at 2948 (offset -14 lines).
patching file src/backend/tcop/utility.c
patching file src/bin/scripts/dropdb.c
Hunk #1 succeeded at 34 (offset 1 line).
Hunk #2 succeeded at 50 (offset 1 line).
Hunk #3 succeeded at 63 (offset 2 lines).
Hunk #4 succeeded at 88 (offset 2 lines).
Hunk #5 succeeded at 128 (offset 2 lines).
Hunk #6 succeeded at 136 (offset 2 lines).
Hunk #7 succeeded at 164 (offset 1 line).
patching file src/bin/scripts/t/050_dropdb.pl
patching file src/include/commands/dbcommands.h
patching file src/include/nodes/parsenodes.h
Hunk #1 succeeded at 3133 (offset 16 lines).
patching file src/include/storage/procarray.h
Hunk #1 FAILED at 112.
1 out of 1 hunk FAILED -- saving rejects to file src/include/storage/procarray.h.rej

Could you please update it ? Thanks.
Anthony

The new status of this patch is: Waiting on Author

Hi

po 24. 6. 2019 v 10:28 odesílatel Anthony Nowocien <anowocien@gmail.com>
napsal:

Hi,
patch no longer applies (as of 12beta2).

postgres@ubuntudev:~/pg_testing/source/postgresql-12beta2$ patch -p1 <
drop-database-force-20190310_01.patch
patching file doc/src/sgml/ref/drop_database.sgml
patching file doc/src/sgml/ref/dropdb.sgml
patching file src/backend/commands/dbcommands.c
Hunk #1 succeeded at 489 (offset 2 lines).
Hunk #2 succeeded at 779 (offset 2 lines).
Hunk #3 succeeded at 787 (offset 2 lines).
Hunk #4 succeeded at 871 (offset 2 lines).
Hunk #5 succeeded at 1056 (offset 2 lines).
Hunk #6 succeeded at 1186 (offset 2 lines).
patching file src/backend/nodes/copyfuncs.c
Hunk #1 succeeded at 3852 (offset 10 lines).
patching file src/backend/nodes/equalfuncs.c
Hunk #1 succeeded at 1666 (offset 3 lines).
patching file src/backend/parser/gram.y
Hunk #1 succeeded at 10194 (offset 45 lines).
Hunk #2 succeeded at 10202 (offset 45 lines).
patching file src/backend/storage/ipc/procarray.c
Hunk #1 succeeded at 2906 (offset -14 lines).
Hunk #2 succeeded at 2948 (offset -14 lines).
patching file src/backend/tcop/utility.c
patching file src/bin/scripts/dropdb.c
Hunk #1 succeeded at 34 (offset 1 line).
Hunk #2 succeeded at 50 (offset 1 line).
Hunk #3 succeeded at 63 (offset 2 lines).
Hunk #4 succeeded at 88 (offset 2 lines).
Hunk #5 succeeded at 128 (offset 2 lines).
Hunk #6 succeeded at 136 (offset 2 lines).
Hunk #7 succeeded at 164 (offset 1 line).
patching file src/bin/scripts/t/050_dropdb.pl
patching file src/include/commands/dbcommands.h
patching file src/include/nodes/parsenodes.h
Hunk #1 succeeded at 3133 (offset 16 lines).
patching file src/include/storage/procarray.h
Hunk #1 FAILED at 112.
1 out of 1 hunk FAILED -- saving rejects to file
src/include/storage/procarray.h.rej

Could you please update it ? Thanks.

fixed

Regards

Pavel

Anthony

On Thu, Jun 27, 2019 at 7:15 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:

fixed

Hi Pavel,

FYI t/050_dropdb.pl fails consistently with this patch applied:

https://travis-ci.org/postgresql-cfbot/postgresql/builds/555234838

--
Thomas Munro
https://enterprisedb.com