dropdb --force

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
115 messages Options
1234 ... 6
Reply | Threaded
Open this post in threaded view
|

dropdb --force

Filip Rembiałkowski-4
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.


Thanks,
Filip

postgresql-dropdb--force.20181218_01.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule
Hi

út 18. 12. 2018 v 16:11 odesílatel Filip Rembiałkowski <[hidden email]> 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


Thanks,
Filip

drop-database-force-10.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Marti Raudsepp
Hi

> út 18. 12. 2018 v 16:11 odesílatel Filip Rembiałkowski <[hidden email]> 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 <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

David Fetter
In reply to this post by Filip Rembiałkowski-4
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

Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Tom Lane-2
In reply to this post by Marti Raudsepp
Marti Raudsepp <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Alvaro Herrera-9
In reply to this post by Marti Raudsepp
I wonder if this idea from seven years ago might be useful:
https://postgr.es/m/1305688547-sup-7028@...

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

Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


st 19. 12. 2018 v 16:55 odesílatel Alvaro Herrera <[hidden email]> napsal:
I wonder if this idea from seven years ago might be useful:
https://postgr.es/m/1305688547-sup-7028@...

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




--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Filip Rembiałkowski-4
In reply to this post by Marti Raudsepp
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 Tue, Dec 18, 2018 at 4:34 PM Marti Raudsepp <[hidden email]> wrote:

>
> Hi
>
> > út 18. 12. 2018 v 16:11 odesílatel Filip Rembiałkowski <[hidden email]> 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 <[hidden email]> 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

drop-database-force-20190305_01.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Thomas Munro-5
On Wed, Mar 6, 2019 at 1:39 PM Filip Rembiałkowski
<[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Filip Rembiałkowski-4
Thank you. Updated patch attached.

On Sat, Mar 9, 2019 at 2:53 AM Thomas Munro <[hidden email]> wrote:

>
> On Wed, Mar 6, 2019 at 1:39 PM Filip Rembiałkowski
> <[hidden email]> 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

drop-database-force-20190310_01.patch (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

ryanlambert
Hello,
This is a feature I have wanted for a long time, thank you for your work on this.
The latest patch [1] 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,


Ryan Lambert
RustProof Labs


On Sun, Mar 10, 2019 at 12:54 PM Filip Rembiałkowski <[hidden email]> wrote:
Thank you. Updated patch attached.

On Sat, Mar 9, 2019 at 2:53 AM Thomas Munro <[hidden email]> wrote:
>
> On Wed, Mar 6, 2019 at 1:39 PM Filip Rembiałkowski
> <[hidden email]> 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
Ryan Lambert
RustProof  Labs
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Andres Freund
In reply to this post by Filip Rembiałkowski-4
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


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Filip Rembiałkowski-4
On 31.03.2019, 04:35 Andres Freund <[hidden email]> 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!


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Ibrar Ahmed-4
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.
!>
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Ibrar Ahmed-4
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 <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Ibrar Ahmed-4
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
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Anthony Nowocien
Also works fine according to my testing. Documentation is also clear.
Thanks for this useful patch.
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Anthony Nowocien
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
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule
Hi

po 24. 6. 2019 v 10:28 odesílatel Anthony Nowocien <[hidden email]> 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

The new status of this patch is: Waiting on Author

drop-database-force-20190626_01.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Thomas Munro-5
On Thu, Jun 27, 2019 at 7:15 AM Pavel Stehule <[hidden email]> 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


1234 ... 6