dropdb --force

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
135 messages Options
12345 ... 7
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


po 8. 7. 2019 v 0:07 odesílatel Thomas Munro <[hidden email]> napsal:
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

with attached patch should be ok

Regards

Pavel



--
Thomas Munro
https://enterprisedb.com

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

Re: dropdb --force

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

Hi,

The latest patch [1] applies cleanly and basic functionality retested.  Looks great, thanks!

https://www.postgresql.org/message-id/attachment/102334/drop-database-force-20190708.patch

Ryan Lambert

The new status of this patch is: Ready for Committer
Ryan Lambert
RustProof  Labs
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

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


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


čt 25. 7. 2019 v 5:11 odesílatel Tom Lane <[hidden email]> napsal:
Pavel Stehule <[hidden email]> 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 ...


Can be 

DROP DATABASE '(' options ...) [IF EXISTS] name

ok?
 

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.

I don't think so server side implementation is too helpful - there is lot of situations, where DDL command is much more practical.


                        regards, tom lane
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Thomas Munro-5
On Thu, Jul 25, 2019 at 8:45 PM Pavel Stehule <[hidden email]> wrote:
> čt 25. 7. 2019 v 5:11 odesílatel Tom Lane <[hidden email]> napsal:
>> Pavel Stehule <[hidden email]> 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.

Hi Pavel,

The concept seems popular (I've wanted this myself), but it sounds
like it needs some more work.  I've moved this to the next CF.  If you
don't think you'll be able to work on it for the next CF, of course,
please feel free to change it to "Returned with feedback".

--
Thomas Munro
https://enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

ryanlambert
I set the status to Waiting on Author since Tom's concerns [1] have not been addressed.

[1] https://www.postgresql.org/message-id/15707.1564024305%40sss.pgh.pa.us

Thanks,
Ryan
Ryan Lambert
RustProof  Labs
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Alvaro Herrera-9
In reply to this post by Pavel Stehule
On 2019-Jul-25, Pavel Stehule wrote:

> čt 25. 7. 2019 v 5:11 odesílatel Tom Lane <[hidden email]> napsal:
>
> > Pavel Stehule <[hidden email]> writes:

> > * I'm concerned that the proposed syntax is not future-proof.
>
> Can be
>
> DROP DATABASE '(' options ...) [IF EXISTS] name
>
> ok?

Seems weird to me.  I'd rather have the options at the end with a WITH
keyword.  But that's just me, looking at gram.y for other productions
involving ^DROP.

> I don't think so server side implementation is too helpful - there is lot
> of situations, where DDL command is much more practical.

I tend to agree.  Not really a fan of the double-timeout business,
though.

So when are you submitting an updated patch, addressing the other items
that Tom mentions in his review?

--
Á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


út 3. 9. 2019 v 18:46 odesílatel Alvaro Herrera <[hidden email]> napsal:
On 2019-Jul-25, Pavel Stehule wrote:

> čt 25. 7. 2019 v 5:11 odesílatel Tom Lane <[hidden email]> napsal:
>
> > Pavel Stehule <[hidden email]> writes:

> > * I'm concerned that the proposed syntax is not future-proof.
>
> Can be
>
> DROP DATABASE '(' options ...) [IF EXISTS] name
>
> ok?

Seems weird to me.  I'd rather have the options at the end with a WITH
keyword.  But that's just me, looking at gram.y for other productions
involving ^DROP.

> I don't think so server side implementation is too helpful - there is lot
> of situations, where DDL command is much more practical.

I tend to agree.  Not really a fan of the double-timeout business,
though.

So when are you submitting an updated patch, addressing the other items
that Tom mentions in his review?

I would to prepare patch this week.

Regards

Pavel 

--
Á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
In reply to this post by Tom Lane-2
Hi

I started work on this patch. I changed syntax to

DROP DATABASE [ ( FORCE , ..) ] [IF EXISTS ...]

and now I try to fix all other points from Tom's list

út 17. 9. 2019 v 12:15 odesílatel Tom Lane <[hidden email]> napsal:
Pavel Stehule <[hidden email]> 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().

This is question. There are two possible requirements about necessary rights

a) rights for other process termination
b) database owner can drop database

I understand very well to Tom's objection, but request to have ROLE_SIGNAL_BACKEND or be super user looks too strong and not too much practical.

If I am a owner of database, and I have a right to drop this database, why I cannot to kick some other user that block database dropping.

What do you think about it? If I use pg_signal_backend, then the necessary requirement for using DROP FORCE have to be granted SIGNAL_BACKEND rights.


I am sending updated version

Regards


Pavel

 

* 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





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

Re: dropdb --force

ryanlambert
Hi Pavel,
I took a quick look through the patch, I'll try to build and test it tomorrow. 


--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3145,6 +3145,7 @@ typedef struct DropdbStmt
NodeTag type;
char   *dbname; /* database to drop */
bool missing_ok; /* skip error if db is missing? */
+ List   *options; /* currently only FORCE is supported */
} DropdbStmt;

Why put FORCE as the single item in an options list?  A bool var seems like it would be more clear and consistent.

- * DROP DATABASE [ IF EXISTS ]
+ * DROP DATABASE [ ( FORCE ) ] [ IF EXISTS ]

Why is it `[ ( FORCE ) ]` instead of `[ FORCE ]`?
There are also places in the code that seem like extra () are around FORCE.  Like here:

+ appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
+  (force ? " (FORCE) " : ""),
+  (if_exists ? "IF EXISTS " : ""), fmtId(dbname));

And here:

+$node->safe_psql('postgres', 'CREATE DATABASE foobar2');
+$node->issues_sql_like(
+ [ 'dropdb', '--force', 'foobar2' ],
+ qr/statement: DROP DATABASE (FORCE) foobar2/,
+ 'SQL DROP DATABASE (FORCE) run');
+

I'll try to build and test the patch tomorrow. 

Thanks,

Ryan Lambert

Ryan Lambert
RustProof  Labs
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


st 18. 9. 2019 v 4:53 odesílatel Ryan Lambert <[hidden email]> napsal:
Hi Pavel,
I took a quick look through the patch, I'll try to build and test it tomorrow. 


--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3145,6 +3145,7 @@ typedef struct DropdbStmt
NodeTag type;
char   *dbname; /* database to drop */
bool missing_ok; /* skip error if db is missing? */
+ List   *options; /* currently only FORCE is supported */
} DropdbStmt;

Why put FORCE as the single item in an options list?  A bool var seems like it would be more clear and consistent.

see discussion - it was one from Tom's objections. It is due possible future enhancing or modification. It can looks strange, because now there is only one option, but it allow very easy modifications. More it is consistent with lot of other pg commands.


- * DROP DATABASE [ IF EXISTS ]
+ * DROP DATABASE [ ( FORCE ) ] [ IF EXISTS ]

Why is it `[ ( FORCE ) ]` instead of `[ FORCE ]`?
There are also places in the code that seem like extra () are around FORCE.  Like here:

It was discussed before. FORCE is not reserved keyword, so inside list is due safety against possible collisions.


+ appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
+  (force ? " (FORCE) " : ""),
+  (if_exists ? "IF EXISTS " : ""), fmtId(dbname));

And here:

+$node->safe_psql('postgres', 'CREATE DATABASE foobar2');
+$node->issues_sql_like(
+ [ 'dropdb', '--force', 'foobar2' ],
+ qr/statement: DROP DATABASE (FORCE) foobar2/,
+ 'SQL DROP DATABASE (FORCE) run');
+

I'll try to build and test the patch tomorrow. 

Thanks,

Ryan Lambert

Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


st 18. 9. 2019 v 4:57 odesílatel Pavel Stehule <[hidden email]> napsal:


st 18. 9. 2019 v 4:53 odesílatel Ryan Lambert <[hidden email]> napsal:
Hi Pavel,
I took a quick look through the patch, I'll try to build and test it tomorrow. 


--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3145,6 +3145,7 @@ typedef struct DropdbStmt
NodeTag type;
char   *dbname; /* database to drop */
bool missing_ok; /* skip error if db is missing? */
+ List   *options; /* currently only FORCE is supported */
} DropdbStmt;

Why put FORCE as the single item in an options list?  A bool var seems like it would be more clear and consistent.

see discussion - it was one from Tom's objections. It is due possible future enhancing or modification. It can looks strange, because now there is only one option, but it allow very easy modifications. More it is consistent with lot of other pg commands.

I can imagine so somebody will write support for concurrently dropping - so this list will be longer


- * DROP DATABASE [ IF EXISTS ]
+ * DROP DATABASE [ ( FORCE ) ] [ IF EXISTS ]

Why is it `[ ( FORCE ) ]` instead of `[ FORCE ]`?
There are also places in the code that seem like extra () are around FORCE.  Like here:

It was discussed before. FORCE is not reserved keyword, so inside list is due safety against possible collisions.


+ appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
+  (force ? " (FORCE) " : ""),
+  (if_exists ? "IF EXISTS " : ""), fmtId(dbname));

And here:

+$node->safe_psql('postgres', 'CREATE DATABASE foobar2');
+$node->issues_sql_like(
+ [ 'dropdb', '--force', 'foobar2' ],
+ qr/statement: DROP DATABASE (FORCE) foobar2/,
+ 'SQL DROP DATABASE (FORCE) run');
+

I'll try to build and test the patch tomorrow. 

Thanks,

Ryan Lambert

Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

vignesh C
On Wed, Sep 18, 2019 at 8:30 AM Pavel Stehule <[hidden email]> wrote:
>
>
Hi Pavel,

One Comment:
In the documentation we say drop database will fail after 60 seconds
  <varlistentry>
    <term><literal>FORCE</literal></term>
    <listitem>
     <para>
      Attempt to terminate all existing connections to the target database.
     </para>
     <para>
      This will fail, if current user has no permissions to terminate other
      connections. Required permissions are the same as with
      <literal>pg_terminate_backend</literal>, described
      in <xref linkend="functions-admin-signal"/>.

      This will also fail, if the connections do not terminate in 60 seconds.
     </para>
    </listitem>
   </varlistentry>


But in TerminateOtherDBBackends:
foreach (lc, pids)
+ {
+ int pid = lfirst_int(lc);
+
+ (void) kill(pid, SIGTERM); /* ignore any error */
+ }
+
+ /* sleep 100ms */
+ pg_usleep(100 * 1000L);
+ }

We check for any connected backends after sending kill signal in
CountOtherDBBackends and throw error immediately.

I had also tested this scenario to get the following error immediately:
test=# drop database (force) test1;
ERROR:  database "test1" is being accessed by other users
DETAIL:  There is 1 other session using the database.

I feel some change is required to keep documentation and code in sync.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


st 18. 9. 2019 v 5:59 odesílatel vignesh C <[hidden email]> napsal:
On Wed, Sep 18, 2019 at 8:30 AM Pavel Stehule <[hidden email]> wrote:
>
>
Hi Pavel,

One Comment:
In the documentation we say drop database will fail after 60 seconds
  <varlistentry>
    <term><literal>FORCE</literal></term>
    <listitem>
     <para>
      Attempt to terminate all existing connections to the target database.
     </para>
     <para>
      This will fail, if current user has no permissions to terminate other
      connections. Required permissions are the same as with
      <literal>pg_terminate_backend</literal>, described
      in <xref linkend="functions-admin-signal"/>.

      This will also fail, if the connections do not terminate in 60 seconds.
     </para>
    </listitem>
   </varlistentry>

This is not valid. With FORCE flag the clients are closed immediately



But in TerminateOtherDBBackends:
foreach (lc, pids)
+ {
+ int pid = lfirst_int(lc);
+
+ (void) kill(pid, SIGTERM); /* ignore any error */
+ }
+
+ /* sleep 100ms */
+ pg_usleep(100 * 1000L);
+ }

We check for any connected backends after sending kill signal in
CountOtherDBBackends and throw error immediately.

I had also tested this scenario to get the following error immediately:
test=# drop database (force) test1;
ERROR:  database "test1" is being accessed by other users
DETAIL:  There is 1 other session using the database.


sure - you cannot to kill self
 
I feel some change is required to keep documentation and code in sync.

I am waiting to Tom's reply about necessary rights. But the doc part is not synced, and should be fixed.

Pavel

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

vignesh C
On Wed, Sep 18, 2019 at 9:41 AM Pavel Stehule <[hidden email]> wrote:

>
>
>
> st 18. 9. 2019 v 5:59 odesílatel vignesh C <[hidden email]> napsal:
>>
>> On Wed, Sep 18, 2019 at 8:30 AM Pavel Stehule <[hidden email]> wrote:
>> >
>> >
>> Hi Pavel,
>>
>> One Comment:
>> In the documentation we say drop database will fail after 60 seconds
>>   <varlistentry>
>>     <term><literal>FORCE</literal></term>
>>     <listitem>
>>      <para>
>>       Attempt to terminate all existing connections to the target database.
>>      </para>
>>      <para>
>>       This will fail, if current user has no permissions to terminate other
>>       connections. Required permissions are the same as with
>>       <literal>pg_terminate_backend</literal>, described
>>       in <xref linkend="functions-admin-signal"/>.
>>
>>       This will also fail, if the connections do not terminate in 60 seconds.
>>      </para>
>>     </listitem>
>>    </varlistentry>
>
>
> This is not valid. With FORCE flag the clients are closed immediately
>
>>
>>
>> But in TerminateOtherDBBackends:
>> foreach (lc, pids)
>> + {
>> + int pid = lfirst_int(lc);
>> +
>> + (void) kill(pid, SIGTERM); /* ignore any error */
>> + }
>> +
>> + /* sleep 100ms */
>> + pg_usleep(100 * 1000L);
>> + }
>>
>> We check for any connected backends after sending kill signal in
>> CountOtherDBBackends and throw error immediately.
>>
>> I had also tested this scenario to get the following error immediately:
>> test=# drop database (force) test1;
>> ERROR:  database "test1" is being accessed by other users
>> DETAIL:  There is 1 other session using the database.
>>
>
> sure - you cannot to kill self
>
This was not a case where we try to do drop database from the same
session, I got this error when one of the process took longer time to
terminate the other connected process.
But this scenario was simulated using gdb, I'm not sure if similar
scenario is possible without gdb in production environment. If
terminating process does not happen immediately then the above
scenario can happen.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


st 18. 9. 2019 v 19:11 odesílatel vignesh C <[hidden email]> napsal:
On Wed, Sep 18, 2019 at 9:41 AM Pavel Stehule <[hidden email]> wrote:
>
>
>
> st 18. 9. 2019 v 5:59 odesílatel vignesh C <[hidden email]> napsal:
>>
>> On Wed, Sep 18, 2019 at 8:30 AM Pavel Stehule <[hidden email]> wrote:
>> >
>> >
>> Hi Pavel,
>>
>> One Comment:
>> In the documentation we say drop database will fail after 60 seconds
>>   <varlistentry>
>>     <term><literal>FORCE</literal></term>
>>     <listitem>
>>      <para>
>>       Attempt to terminate all existing connections to the target database.
>>      </para>
>>      <para>
>>       This will fail, if current user has no permissions to terminate other
>>       connections. Required permissions are the same as with
>>       <literal>pg_terminate_backend</literal>, described
>>       in <xref linkend="functions-admin-signal"/>.
>>
>>       This will also fail, if the connections do not terminate in 60 seconds.
>>      </para>
>>     </listitem>
>>    </varlistentry>
>
>
> This is not valid. With FORCE flag the clients are closed immediately
>
>>
>>
>> But in TerminateOtherDBBackends:
>> foreach (lc, pids)
>> + {
>> + int pid = lfirst_int(lc);
>> +
>> + (void) kill(pid, SIGTERM); /* ignore any error */
>> + }
>> +
>> + /* sleep 100ms */
>> + pg_usleep(100 * 1000L);
>> + }
>>
>> We check for any connected backends after sending kill signal in
>> CountOtherDBBackends and throw error immediately.
>>
>> I had also tested this scenario to get the following error immediately:
>> test=# drop database (force) test1;
>> ERROR:  database "test1" is being accessed by other users
>> DETAIL:  There is 1 other session using the database.
>>
>
> sure - you cannot to kill self
>
This was not a case where we try to do drop database from the same
session, I got this error when one of the process took longer time to
terminate the other connected process.
But this scenario was simulated using gdb, I'm not sure if similar
scenario is possible without gdb in production environment. If
terminating process does not happen immediately then the above
scenario can happen.

maybe - gdb can handle SIGTERM signal.

I think so current design should be ok, because it immediately send SIGTERM signals and then try to check 5 sec if these signals are really processed. If not, then it raise error, and do nothing.

Pavel



Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule
Hi

I am sending updated version - the changes against last patch are two. I use pg_terminate_backed for killing other terminates like Tom proposed. I am not sure if it is 100% practical - on second hand the necessary right to kill other sessions is  almost available - and consistency with pg_terminal_backend has sense. More - native implementation is significantly better then solution implemented outer. I fixed docs little bit - the timeout for logout (as reaction on SIGTERM is only 5 sec).

Regards

Pavel




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

Re: dropdb --force

vignesh C
On Thu, Sep 19, 2019 at 12:14 AM Pavel Stehule <[hidden email]> wrote:

>
> Hi
>
> I am sending updated version - the changes against last patch are two. I use pg_terminate_backed for killing other terminates like Tom proposed. I am not sure if it is 100% practical - on second hand the necessary right to kill other sessions is  almost available - and consistency with pg_terminal_backend has sense. More - native implementation is significantly better then solution implemented outer. I fixed docs little bit - the timeout for logout (as reaction on SIGTERM is only 5 sec).
>
> Regards
>
> Pavel
>
>
I observed one thing with the latest patch:
There is a possibility that in case of drop database failure some
sessions may be terminated.

It can happen in the following scenario:
1) create database test; /* super user creates test database */
2) create user test password 'test@123'; /* super user creates test user */
3) alter user test nosuperuser; /* super user changes test use to non
super user */
4) alter database test owner to test; /* super user set test as test
database owner */

5) psql -d test /* connect to test database as super user - Session 1 */
6) psql -d postgres -U test /* connect to test database as test user -
Session 2 */
7) psql -d postgres -U test /* connect to test database as test user -
Session 3 */

8) drop database (force) test; /* Executed from Session 3 */

Drop database fails in Session 3 with:
ERROR:  must be a superuser to terminate superuser process

Session 2 is terminated, Session 1 is left as it is.

Is the above behaviour ok to terminate session 2 in case of drop
database failure?
Thoughts?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


čt 19. 9. 2019 v 13:52 odesílatel vignesh C <[hidden email]> napsal:
On Thu, Sep 19, 2019 at 12:14 AM Pavel Stehule <[hidden email]> wrote:
>
> Hi
>
> I am sending updated version - the changes against last patch are two. I use pg_terminate_backed for killing other terminates like Tom proposed. I am not sure if it is 100% practical - on second hand the necessary right to kill other sessions is  almost available - and consistency with pg_terminal_backend has sense. More - native implementation is significantly better then solution implemented outer. I fixed docs little bit - the timeout for logout (as reaction on SIGTERM is only 5 sec).
>
> Regards
>
> Pavel
>
>
I observed one thing with the latest patch:
There is a possibility that in case of drop database failure some
sessions may be terminated.

It can happen in the following scenario:
1) create database test; /* super user creates test database */
2) create user test password 'test@123'; /* super user creates test user */
3) alter user test nosuperuser; /* super user changes test use to non
super user */
4) alter database test owner to test; /* super user set test as test
database owner */

5) psql -d test /* connect to test database as super user - Session 1 */
6) psql -d postgres -U test /* connect to test database as test user -
Session 2 */
7) psql -d postgres -U test /* connect to test database as test user -
Session 3 */

8) drop database (force) test; /* Executed from Session 3 */

Drop database fails in Session 3 with:
ERROR:  must be a superuser to terminate superuser process

Session 2 is terminated, Session 1 is left as it is.

Is the above behaviour ok to terminate session 2 in case of drop
database failure?
Thoughts?

I agree so it's not nice behave. Again there are two possible solutions

1. strategy - owner can all - and don't check rigts
2. implement own check of rights instead using checks from pg_terminate_backend.

It's easy fixable when we find a agreement what is preferred behave.

Pavel


Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Thomas Munro-5
In reply to this post by Pavel Stehule
On Thu, Sep 19, 2019 at 6:44 AM Pavel Stehule <[hidden email]> wrote:
> I am sending updated version

+    appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
+                      (force ? " (FORCE) " : ""),

An extra space before (FORCE) caused the test to fail:

# 2019-09-19 19:07:22.203 UTC [1516] 050_dropdb.pl LOG: statement:
DROP DATABASE (FORCE) foobar2;

#     doesn't match '(?^:statement: DROP DATABASE (FORCE) foobar2)'

--
Thomas Munro
https://enterprisedb.com


12345 ... 7