dropdb --force

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
136 messages Options
1234567
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

vignesh C
On Thu, Sep 19, 2019 at 11:41 PM Pavel Stehule <[hidden email]> wrote:

>
>
>
> č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
>
For the above I felt, if we could identify if we don't have
permissions to terminate any of the connected session, throwing the
error at that point would be appropriate.

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


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Dilip Kumar-2
In reply to this post by Pavel Stehule
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).
>

Few comments on the patch.

@@ -598,10 +598,20 @@ standard_ProcessUtility(PlannedStmt *pstmt,
  case T_DropdbStmt:
  {
  DropdbStmt *stmt = (DropdbStmt *) parsetree;
+ bool force = false;
+ ListCell   *lc;
+
+ foreach(lc, stmt->options)
+ {
+ DefElem    *item = (DefElem *) lfirst(lc);
+
+ if (strcmp(item->defname, "force") == 0)
+ force = true;
+ }

  /* no event triggers for global objects */
  PreventInTransactionBlock(isTopLevel, "DROP DATABASE");
- dropdb(stmt->dbname, stmt->missing_ok);
+ dropdb(stmt->dbname, stmt->missing_ok, force);

If you see the createdb, then options are processed inside the
createdb function but here we are processing outside
the function.  Wouldn't it be good to keep them simmilar and also it
will be expandable in case we add more options
in the future?


initPQExpBuffer(&sql);

- appendPQExpBuffer(&sql, "DROP DATABASE %s%s;",
-   (if_exists ? "IF EXISTS " : ""), fmtId(dbname));
-
  /* Avoid trying to drop postgres db while we are connected to it. */
  if (maintenance_db == NULL && strcmp(dbname, "postgres") == 0)
  maintenance_db = "template1";
@@ -134,6 +136,10 @@ main(int argc, char *argv[])
    host, port, username, prompt_password,
    progname, echo);
+ appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
+   (force ? " (FORCE) " : ""),
+   (if_exists ? "IF EXISTS " : ""), fmtId(dbname));
+

I did not understand why you have moved this code after
connectMaintenanceDatabase function?  I don't see any problem but in
earlier code
initPQExpBuffer and appendPQExpBuffer were together now you have moved
appendPQExpBuffer after the connectMaintenanceDatabase so if there is
no reason to do that then better to keep it how it was earlier.


-extern bool CountOtherDBBackends(Oid databaseId,
- int *nbackends, int *nprepared);
+extern bool CountOtherDBBackends(Oid databaseId, int *nbackends, int
*nprepared);

Unrelated change

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


pá 20. 9. 2019 v 7:58 odesílatel Dilip Kumar <[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).
>

Few comments on the patch.

@@ -598,10 +598,20 @@ standard_ProcessUtility(PlannedStmt *pstmt,
  case T_DropdbStmt:
  {
  DropdbStmt *stmt = (DropdbStmt *) parsetree;
+ bool force = false;
+ ListCell   *lc;
+
+ foreach(lc, stmt->options)
+ {
+ DefElem    *item = (DefElem *) lfirst(lc);
+
+ if (strcmp(item->defname, "force") == 0)
+ force = true;
+ }

  /* no event triggers for global objects */
  PreventInTransactionBlock(isTopLevel, "DROP DATABASE");
- dropdb(stmt->dbname, stmt->missing_ok);
+ dropdb(stmt->dbname, stmt->missing_ok, force);

If you see the createdb, then options are processed inside the
createdb function but here we are processing outside
the function.  Wouldn't it be good to keep them simmilar and also it
will be expandable in case we add more options
in the future?


I though about it, but I think so current state is good enough. There are only few parameters - and I don't think significantly large number of new options.

When number of parameters of any functions is not too high, then I think so is better to have a function with classic list of parameters instead function with one parameter of some struct type.

If somebody try to use function dropdb from outside (extension), then he don't need to create new structure, new list, and simply call simple C API. I prefer API of simple types against structures and nodes there. Just I think so it's more practical of somebody try to use this function from other places than from parser.
 

initPQExpBuffer(&sql);

- appendPQExpBuffer(&sql, "DROP DATABASE %s%s;",
-   (if_exists ? "IF EXISTS " : ""), fmtId(dbname));
-
  /* Avoid trying to drop postgres db while we are connected to it. */
  if (maintenance_db == NULL && strcmp(dbname, "postgres") == 0)
  maintenance_db = "template1";
@@ -134,6 +136,10 @@ main(int argc, char *argv[])
    host, port, username, prompt_password,
    progname, echo);
+ appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
+   (force ? " (FORCE) " : ""),
+   (if_exists ? "IF EXISTS " : ""), fmtId(dbname));
+

I did not understand why you have moved this code after
connectMaintenanceDatabase function?  I don't see any problem but in
earlier code
initPQExpBuffer and appendPQExpBuffer were together now you have moved
appendPQExpBuffer after the connectMaintenanceDatabase so if there is
no reason to do that then better to keep it how it was earlier.


I am not a author of this change, but it is not necessary and I returned original order


-extern bool CountOtherDBBackends(Oid databaseId,
- int *nbackends, int *nprepared);
+extern bool CountOtherDBBackends(Oid databaseId, int *nbackends, int
*nprepared);

Unrelated change

fixed

Thank you for check. I am sending updated patch

Regards

Pavel



--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

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

Re: dropdb --force

Pavel Stehule
In reply to this post by Thomas Munro-5


pá 20. 9. 2019 v 5:10 odesílatel Thomas Munro <[hidden email]> napsal:
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)'

should be fixed now.

thank you for echo

Pavel


--
Thomas Munro
https://enterprisedb.com
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

akapila
In reply to this post by Pavel Stehule
On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule <[hidden email]> wrote:
>
> Thank you for check. I am sending updated patch
>

Alvaro has up thread suggested some alternative syntax [1] for this
patch, but I don't see any good argument to not go with what he has
proposed.  In other words, why we should prefer the current syntax as
in the patch over what Alvaro has proposed?

IIUC,  the current syntax implemented by the patch is:
Drop Database [(options)] [If Exists] name
Alvaro suggested using options at the end (and use optional keyword
WITH) based on what other Drop commands does.  I see some merits to
that idea which are (a) if tomorrow we want to introduce new options
like CASCADE, RESTRICT then it will be better to have all the options
at the end as we have for other Drop commands, (b) It will resemble
more with Create Database syntax.

Now, I think the current syntax is also not bad and we already do
something like that for other commands like Vaccum where options are
provided before object_name, but I think in this case putting at the
end is more appealing unless there are some arguments against that.

One other minor comment:
+
+      This will also fail, if the connections do not terminate in 5 seconds.
+     </para>

Is there any implementation in the patch for the above note?

[1] - https://www.postgresql.org/message-id/20190903164633.GA16408%40alvherre.pgsql

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

vignesh C
On Tue, Sep 24, 2019 at 6:22 PM Amit Kapila <[hidden email]> wrote:
>
> On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule <[hidden email]> wrote:
> >
> > Thank you for check. I am sending updated patch
> >
>
Session termination in case of drop database is solved.
Some typos:
+ /*
+ * Similar code to pg_terminate_backend, but we check rigts first
+ * here, and only when we have all necessary rights we start to
+ * terminate other clients. In this case we should not to raise
+ * some warnings - like "PID %d is not a PostgreSQL server process",
+ * because for this purpose - already finished session is not
+ * problem.
+ */
"rigts" should be "rights/privilege"
"should not to raise" could be "should not raise"

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


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

akapila
In reply to this post by akapila
On Tue, Sep 24, 2019 at 6:22 PM Amit Kapila <[hidden email]> wrote:

> On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule <[hidden email]> wrote:
> >
> > Thank you for check. I am sending updated patch
> >
>
> Alvaro has up thread suggested some alternative syntax [1] for this
> patch, but I don't see any good argument to not go with what he has
> proposed.  In other words, why we should prefer the current syntax as
> in the patch over what Alvaro has proposed?
>
> IIUC,  the current syntax implemented by the patch is:
> Drop Database [(options)] [If Exists] name
> Alvaro suggested using options at the end (and use optional keyword
> WITH) based on what other Drop commands does.  I see some merits to
> that idea which are (a) if tomorrow we want to introduce new options
> like CASCADE, RESTRICT then it will be better to have all the options
> at the end as we have for other Drop commands, (b) It will resemble
> more with Create Database syntax.
>
> Now, I think the current syntax is also not bad and we already do
> something like that for other commands like Vaccum where options are
> provided before object_name, but I think in this case putting at the
> end is more appealing unless there are some arguments against that.
>
> One other minor comment:
> +
> +      This will also fail, if the connections do not terminate in 5 seconds.
> +     </para>
>
> Is there any implementation in the patch for the above note?
>

One more point I would like to add here is that I think it is worth
considering to split this patch by keeping the changes in dropdb
utility as a separate patch.  Even though the code is not very much
but I think it can be a separate patch atop the main patch which
contains the core server changes.


--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

vignesh C
In reply to this post by Pavel Stehule
On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule <[hidden email]> wrote:
> fixed
>
> Thank you for check. I am sending updated patch
>
Thanks for fixing all the comments.
Couple of suggestions:
-DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
+DROP DATABASE [ ( <replaceable class="parameter">option</replaceable>
[, ...] ) ] [ IF EXISTS ] <replaceable
class="parameter">name</replaceable>
+
+<phrase>where <replaceable class="parameter">option</replaceable> can
be one of:</phrase>
+
+    FORCE

+drop_option_list:
+ drop_option
+ {
+ $$ = list_make1((Node *) $1);
+ }
+ | drop_option_list ',' drop_option
+ {
+ $$ = lappend($1, (Node *) $3);
+ }
+ ;
+
+drop_option: FORCE
+ {
+ $$ = makeDefElem("force", NULL, @1);
+ }
+ ;

Currently only force option is supported in: drop database (force) dbname
Should we have a list or a single element for this?
I'm not sure if we have any plans to add more option in this area.

If possible we can add an error message like 'ERROR:  unrecognized
drop database option "force1"' if an invalid input is given.
The above error message will be inline with error message of vacuum's
error message whose syntax is similar to the current feature.
We could throw the error from here:
  case T_DropdbStmt:
  {
  DropdbStmt *stmt = (DropdbStmt *) parsetree;
+ bool force = false;
+ ListCell   *lc;
+
+ foreach(lc, stmt->options)
+ {
+ DefElem    *item = (DefElem *) lfirst(lc);
+
+ if (strcmp(item->defname, "force") == 0)
+ force = true;
+ }
Thoughts?

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


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule
In reply to this post by akapila


út 24. 9. 2019 v 14:52 odesílatel Amit Kapila <[hidden email]> napsal:
On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule <[hidden email]> wrote:
>
> Thank you for check. I am sending updated patch
>

Alvaro has up thread suggested some alternative syntax [1] for this
patch, but I don't see any good argument to not go with what he has
proposed.  In other words, why we should prefer the current syntax as
in the patch over what Alvaro has proposed?

IIUC,  the current syntax implemented by the patch is:
Drop Database [(options)] [If Exists] name
Alvaro suggested using options at the end (and use optional keyword
WITH) based on what other Drop commands does.  I see some merits to
that idea which are (a) if tomorrow we want to introduce new options
like CASCADE, RESTRICT then it will be better to have all the options
at the end as we have for other Drop commands, (b) It will resemble
more with Create Database syntax.

Now, I think the current syntax is also not bad and we already do
something like that for other commands like Vaccum where options are
provided before object_name, but I think in this case putting at the
end is more appealing unless there are some arguments against that.

Originally it was placed after name.  Tom's objection was possibility to collision with some future standards and requirement to be implemented safe.

cite: "* 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 ..."

When I use parenthesis, then current placement looks correct - and it is well known syntax already.

Alternative is DROP DATABASE [IF EXISTS] name [ CASCADE | RESTRICT ] [ WITH FORCE ]

but in this case WIDTH keyword should not be optional (If I need to solve Tom's note). Currently WITH keyword is optional every where, so I think so using syntax with required WIDTH keyword is not happy.

When I looks to other statement, then the most similar case is DROP INDEX CONCURRENTLY ... so most consistent syntax is DROP DATABASE FORCE ... or DROP DATABASE (FORCE, ..)

Optional syntax can be (similar to CREATE USER MAPPING - but it looks like too verbose

DROP DATABASE xxx  OPTIONS (FORCE, ...)

It's easy to change syntax, and I'll do it - I have not strong preferences, but If wouldn't to increase Tom's paranoia, I think so current syntax is most common in pg, and well known.

What do you think about it?




One other minor comment:
+
+      This will also fail, if the connections do not terminate in 5 seconds.
+     </para>

Is there any implementation in the patch for the above note?

Yes, is there.

The force flag ensure sending SIGTERM to related clients. Nothing more. There are still check

-->if (CountOtherDBBackends(db_id, &notherbackends, &npreparedxacts))
<--><-->ereport(ERROR,
<--><--><--><-->(errcode(ERRCODE_OBJECT_IN_USE),
<--><--><--><--> errmsg("database \"%s\" is being accessed by other users",
<--><--><--><--><--><-->dbname),
<--><--><--><--> errdetail_busy_db(notherbackends, npreparedxacts)));

that can fails after 5 sec. Sending signal doesn't ensure nothing, so I am for no changes in these check.

Regards

Pavel
 

[1] - https://www.postgresql.org/message-id/20190903164633.GA16408%40alvherre.pgsql

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule
In reply to this post by vignesh C


út 24. 9. 2019 v 17:51 odesílatel vignesh C <[hidden email]> napsal:
On Tue, Sep 24, 2019 at 6:22 PM Amit Kapila <[hidden email]> wrote:
>
> On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule <[hidden email]> wrote:
> >
> > Thank you for check. I am sending updated patch
> >
>
Session termination in case of drop database is solved.
Some typos:
+ /*
+ * Similar code to pg_terminate_backend, but we check rigts first
+ * here, and only when we have all necessary rights we start to
+ * terminate other clients. In this case we should not to raise
+ * some warnings - like "PID %d is not a PostgreSQL server process",
+ * because for this purpose - already finished session is not
+ * problem.
+ */
"rigts" should be "rights/privilege"
"should not to raise" could be "should not raise"

fixed


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

Re: dropdb --force

Pavel Stehule
In reply to this post by vignesh C


st 25. 9. 2019 v 6:38 odesílatel vignesh C <[hidden email]> napsal:
On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule <[hidden email]> wrote:
> fixed
>
> Thank you for check. I am sending updated patch
>
Thanks for fixing all the comments.
Couple of suggestions:
-DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
+DROP DATABASE [ ( <replaceable class="parameter">option</replaceable>
[, ...] ) ] [ IF EXISTS ] <replaceable
class="parameter">name</replaceable>
+
+<phrase>where <replaceable class="parameter">option</replaceable> can
be one of:</phrase>
+
+    FORCE

+drop_option_list:
+ drop_option
+ {
+ $$ = list_make1((Node *) $1);
+ }
+ | drop_option_list ',' drop_option
+ {
+ $$ = lappend($1, (Node *) $3);
+ }
+ ;
+
+drop_option: FORCE
+ {
+ $$ = makeDefElem("force", NULL, @1);
+ }
+ ;

Currently only force option is supported in: drop database (force) dbname
Should we have a list or a single element for this?
I'm not sure if we have any plans to add more option in this area.

If we open door for next possible syntax enhancing and it is motivation for this syntax, then we should to use pattern for this situation.
It looks little bit strange, but I think so current code is very well readable. I wrote comment, so only one flag is supported now, but
syntax allows add other flags. I don't think so using defelem directly reduces significantly enough lines - just if we implement some
what looks like possible list, then we should to use lists inside.


If possible we can add an error message like 'ERROR:  unrecognized
drop database option "force1"' if an invalid input is given.
The above error message will be inline with error message of vacuum's
error message whose syntax is similar to the current feature.
We could throw the error from here:
  case T_DropdbStmt:
  {
  DropdbStmt *stmt = (DropdbStmt *) parsetree;
+ bool force = false;
+ ListCell   *lc;
+
+ foreach(lc, stmt->options)
+ {
+ DefElem    *item = (DefElem *) lfirst(lc);
+
+ if (strcmp(item->defname, "force") == 0)
+ force = true;
+ }
Thoughts?

I moved this check to separate function DropDatabase with new check and exception like you proposed.

Regards

Pavel
 

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

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

Re: dropdb --force

Pavel Stehule
In reply to this post by akapila


st 25. 9. 2019 v 4:14 odesílatel Amit Kapila <[hidden email]> napsal:
On Tue, Sep 24, 2019 at 6:22 PM Amit Kapila <[hidden email]> wrote:
> On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule <[hidden email]> wrote:
> >
> > Thank you for check. I am sending updated patch
> >
>
> Alvaro has up thread suggested some alternative syntax [1] for this
> patch, but I don't see any good argument to not go with what he has
> proposed.  In other words, why we should prefer the current syntax as
> in the patch over what Alvaro has proposed?
>
> IIUC,  the current syntax implemented by the patch is:
> Drop Database [(options)] [If Exists] name
> Alvaro suggested using options at the end (and use optional keyword
> WITH) based on what other Drop commands does.  I see some merits to
> that idea which are (a) if tomorrow we want to introduce new options
> like CASCADE, RESTRICT then it will be better to have all the options
> at the end as we have for other Drop commands, (b) It will resemble
> more with Create Database syntax.
>
> Now, I think the current syntax is also not bad and we already do
> something like that for other commands like Vaccum where options are
> provided before object_name, but I think in this case putting at the
> end is more appealing unless there are some arguments against that.
>
> One other minor comment:
> +
> +      This will also fail, if the connections do not terminate in 5 seconds.
> +     </para>
>
> Is there any implementation in the patch for the above note?
>

One more point I would like to add here is that I think it is worth
considering to split this patch by keeping the changes in dropdb
utility as a separate patch.  Even though the code is not very much
but I think it can be a separate patch atop the main patch which
contains the core server changes.

I did it - last patch contains server side only. I expect so client side (very small patch) can be next.

Regards

Pavel



--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

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

> Alternative is DROP DATABASE [IF EXISTS] name [ CASCADE | RESTRICT ] [ WITH
> FORCE ]
>
> but in this case WIDTH keyword should not be optional (If I need to solve
> Tom's note). Currently WITH keyword is optional every where, so I think so
> using syntax with required WIDTH keyword is not happy.

Well, you would have one of those:

DROP DATABASE [IF EXISTS] name WITH (FORCE)
DROP DATABASE [IF EXISTS] name

Naturally, the WITH is optional in the sense that the clause itself is
optional.  (Note we don't have CASCADE/RESTRICT in DROP DATABASE.)

You propose

DROP DATABASE (FORCE) [IF EXISTS] name

which seems weird to me -- I think only legacy syntax uses that form.

--
Á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 26. 9. 2019 v 17:35 odesílatel Alvaro Herrera <[hidden email]> napsal:
On 2019-Sep-26, Pavel Stehule wrote:

> Alternative is DROP DATABASE [IF EXISTS] name [ CASCADE | RESTRICT ] [ WITH
> FORCE ]
>
> but in this case WIDTH keyword should not be optional (If I need to solve
> Tom's note). Currently WITH keyword is optional every where, so I think so
> using syntax with required WIDTH keyword is not happy.

Well, you would have one of those:

DROP DATABASE [IF EXISTS] name WITH (FORCE)
DROP DATABASE [IF EXISTS] name

Naturally, the WITH is optional in the sense that the clause itself is
optional.  (Note we don't have CASCADE/RESTRICT in DROP DATABASE.)

You propose

DROP DATABASE (FORCE) [IF EXISTS] name

which seems weird to me -- I think only legacy syntax uses that form.

I have not strong opinion about it, little bit prefer option list after DROP DATABASE, because it is some what I know from EXPLAIN ANALYZE daily work, but it is not too important. Your proposed syntax is ok.

Second patch implements Alvaro's proposed syntax.

Pavel


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

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

Re: dropdb --force

Peter Eisentraut-6
In reply to this post by Alvaro Herrera-9
On 2019-09-26 17:35, Alvaro Herrera wrote:
> Well, you would have one of those:
>
> DROP DATABASE [IF EXISTS] name WITH (FORCE)
> DROP DATABASE [IF EXISTS] name
>
> Naturally, the WITH is optional in the sense that the clause itself is
> optional.  (Note we don't have CASCADE/RESTRICT in DROP DATABASE.)

The WITH here seems weird to me.  Why not leave it out?

--
Peter Eisentraut              http://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 26. 9. 2019 v 18:34 odesílatel Peter Eisentraut <[hidden email]> napsal:
On 2019-09-26 17:35, Alvaro Herrera wrote:
> Well, you would have one of those:
>
> DROP DATABASE [IF EXISTS] name WITH (FORCE)
> DROP DATABASE [IF EXISTS] name
>
> Naturally, the WITH is optional in the sense that the clause itself is
> optional.  (Note we don't have CASCADE/RESTRICT in DROP DATABASE.)

The WITH here seems weird to me.  Why not leave it out?

it is just my subjective opinion so it looks better with it than without it.

so there are three variants

DROP DATABASE ( FORCE) name;
DROP DATABASE name (FORCE)
DROP DATABASE name WITH (FORCE)

It is true so in this case it is just syntactic sugar

Maybe

DROP DATABASE name [[ WITH ] OPTIONS( FORCE ) ] ?

It looks well for me

DROP DATABASE test WITH OPTIONS (FORCE)
DROP DATABASE test OPTIONS (FORCE)

?


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

Re: dropdb --force

akapila
In reply to this post by Peter Eisentraut-6
On Thu, Sep 26, 2019 at 10:04 PM Peter Eisentraut
<[hidden email]> wrote:

>
> On 2019-09-26 17:35, Alvaro Herrera wrote:
> > Well, you would have one of those:
> >
> > DROP DATABASE [IF EXISTS] name WITH (FORCE)
> > DROP DATABASE [IF EXISTS] name
> >
> > Naturally, the WITH is optional in the sense that the clause itself is
> > optional.  (Note we don't have CASCADE/RESTRICT in DROP DATABASE.)
>
> The WITH here seems weird to me.  Why not leave it out?
>

Yeah, we can leave it as well.  However, other commands like COPY
seems to be using WITH clause for a somewhat similar purpose.  I think
we use WITH clause in other cases while specifying multiple options.
So to me, using WITH here doesn't sound to be a bad idea.


--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

akapila
In reply to this post by Pavel Stehule
On Thu, Sep 26, 2019 at 7:18 PM Pavel Stehule <[hidden email]> wrote:

út 24. 9. 2019 v 14:52 odesílatel Amit Kapila <[hidden email]> napsal:

One other minor comment:
+
+      This will also fail, if the connections do not terminate in 5 seconds.
+     </para>

Is there any implementation in the patch for the above note?

Yes, is there.

The force flag ensure sending SIGTERM to related clients. Nothing more. There are still check

-->if (CountOtherDBBackends(db_id, &notherbackends, &npreparedxacts))
<--><-->ereport(ERROR,
<--><--><--><-->(errcode(ERRCODE_OBJECT_IN_USE),
<--><--><--><--> errmsg("database \"%s\" is being accessed by other users",
<--><--><--><--><--><-->dbname),
<--><--><--><--> errdetail_busy_db(notherbackends, npreparedxacts)));

that can fails after 5 sec. Sending signal doesn't ensure nothing, so I am for no changes in these check.

I think 5 seconds is a hard-coded value that can even change in the future.  So, it is better to write something more generic like "This will also fail if we are not able to terminate connections" or something like that.  This part of the documentation might change after addressing the other comments below.

One more point I would like to add here is that I think it is worth
considering to split this patch by keeping the changes in dropdb
utility as a separate patch.  Even though the code is not very much
but I think it can be a separate patch atop the main patch which
contains the core server changes.

> I did it - last patch contains server side only. I expect so client side (very small patch) can be nex

I still see the code related to dropdb utility in the patch.  See,
diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
index dacd8e5f1d..1bb80fda74 100644
--- a/src/bin/scripts/dropdb.c
+++ b/src/bin/scripts/dropdb.c
@@ -34,6 +34,7 @@ main(int argc, char *argv[])
  {"interactive", no_argument, NULL, 'i'},
  {"if-exists", no_argument, &if_exists, 1},
  {"maintenance-db", required_argument, NULL, 2},
+ {"force", no_argument, NULL, 'f'},
  {NULL, 0, NULL, 0}
  };
 
Few other comments:
--------------------------------
1.
+void
+TerminateOtherDBBackends(Oid databaseId)
+{
+ ProcArrayStruct *arrayP = procArray;
+ List   *pids = NIL;
+ int i;
+
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
+
+ for (i = 0; i < procArray->numProcs; i++)
+ {
+ int pgprocno = arrayP->pgprocnos[i];
+ PGPROC   *proc = &allProcs[pgprocno];
+
+ if (proc->databaseId != databaseId)
+ continue;
+ if (proc == MyProc)
+ continue;
+
+ if (proc->pid != 0)
+ pids = lappend_int(pids, proc->pid);
+ }

So here we are terminating only connection which doesn't have any prepared transaction.  The behavior of this patch with the prepared transactions will be terrible. Basically, after terminating all the connections via TerminateOtherDBBackends, we will give error once CountOtherDBBackends is invoked.  I have tested the same and it gives an error like below:

postgres=# drop database db1 With (Force);
ERROR:  database "db1" is being accessed by other users
DETAIL:  There is 1 prepared transaction using the database.

I think we have two options here:
(a) Document that even with force option, if there are any prepared transactions in the same database, we won't drop the database.  Along with this, fix the code such that we don't terminate other connections if the prepared transactions are active.
(b) Rollback the prepared transactions.

I think (a) is a better option because if the number of prepared transactions is large, then it might take time and I am not sure if it is worth to add the complexity of rolling back all the prepared xacts.  OTOH, if you or others think option (b) is good and doesn't add much complexity, then I think it is worth exploring that option.

I think we should definitely do something to deal with this even if you don't like the proposed options because the current behavior of the patch seems worse than either of these options.

2.
-DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
+DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> [ WITH ( <replaceable class="parameter">option</replaceable> [, ...] ) ]

It is better to keep WITH clause as optional similar to Copy command.  This might address Peter E's concern related to WITH clause.

3.
- * DROP DATABASE [ IF EXISTS ]
+ * DROP DATABASE [ ( options ) ] [ IF EXISTS ]

You seem to forget to change the syntax in the above comments after changing the patch.

4.
+   If anyone else is connected to the target database, this command will fail
+   - unless you use the <literal>FORCE</literal> option described below.

I don't understand the significance of using '-' before unless.  I think we can remove it.

Changed the patch status as 'Waiting on Author'.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


st 2. 10. 2019 v 5:20 odesílatel Amit Kapila <[hidden email]> napsal:
On Thu, Sep 26, 2019 at 7:18 PM Pavel Stehule <[hidden email]> wrote:

út 24. 9. 2019 v 14:52 odesílatel Amit Kapila <[hidden email]> napsal:

One other minor comment:
+
+      This will also fail, if the connections do not terminate in 5 seconds.
+     </para>

Is there any implementation in the patch for the above note?

Yes, is there.

The force flag ensure sending SIGTERM to related clients. Nothing more. There are still check

-->if (CountOtherDBBackends(db_id, &notherbackends, &npreparedxacts))
<--><-->ereport(ERROR,
<--><--><--><-->(errcode(ERRCODE_OBJECT_IN_USE),
<--><--><--><--> errmsg("database \"%s\" is being accessed by other users",
<--><--><--><--><--><-->dbname),
<--><--><--><--> errdetail_busy_db(notherbackends, npreparedxacts)));

that can fails after 5 sec. Sending signal doesn't ensure nothing, so I am for no changes in these check.

I think 5 seconds is a hard-coded value that can even change in the future.  So, it is better to write something more generic like "This will also fail if we are not able to terminate connections" or something like that.  This part of the documentation might change after addressing the other comments below.

done


One more point I would like to add here is that I think it is worth
considering to split this patch by keeping the changes in dropdb
utility as a separate patch.  Even though the code is not very much
but I think it can be a separate patch atop the main patch which
contains the core server changes.

> I did it - last patch contains server side only. I expect so client side (very small patch) can be nex

I still see the code related to dropdb utility in the patch.  See,
diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
index dacd8e5f1d..1bb80fda74 100644
--- a/src/bin/scripts/dropdb.c
+++ b/src/bin/scripts/dropdb.c
@@ -34,6 +34,7 @@ main(int argc, char *argv[])
  {"interactive", no_argument, NULL, 'i'},
  {"if-exists", no_argument, &if_exists, 1},
  {"maintenance-db", required_argument, NULL, 2},
+ {"force", no_argument, NULL, 'f'},
  {NULL, 0, NULL, 0}
  };

removed

 
Few other comments:
--------------------------------
1.
+void
+TerminateOtherDBBackends(Oid databaseId)
+{
+ ProcArrayStruct *arrayP = procArray;
+ List   *pids = NIL;
+ int i;
+
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
+
+ for (i = 0; i < procArray->numProcs; i++)
+ {
+ int pgprocno = arrayP->pgprocnos[i];
+ PGPROC   *proc = &allProcs[pgprocno];
+
+ if (proc->databaseId != databaseId)
+ continue;
+ if (proc == MyProc)
+ continue;
+
+ if (proc->pid != 0)
+ pids = lappend_int(pids, proc->pid);
+ }

So here we are terminating only connection which doesn't have any prepared transaction.  The behavior of this patch with the prepared transactions will be terrible. Basically, after terminating all the connections via TerminateOtherDBBackends, we will give error once CountOtherDBBackends is invoked.  I have tested the same and it gives an error like below:

postgres=# drop database db1 With (Force);
ERROR:  database "db1" is being accessed by other users
DETAIL:  There is 1 prepared transaction using the database.

I think we have two options here:
(a) Document that even with force option, if there are any prepared transactions in the same database, we won't drop the database.  Along with this, fix the code such that we don't terminate other connections if the prepared transactions are active.
(b) Rollback the prepared transactions.

I not use prepared transactions often, and then I have not own strong opinion about it. Original parch didn't touch this area, so I think we can continue in this direction (minimally for start).

I did precheck of opened prepared transactions, and when I find any opened, then I raise a exception (before when I try to terminate other processes).

I updated doc about possible stops.


I think (a) is a better option because if the number of prepared transactions is large, then it might take time and I am not sure if it is worth to add the complexity of rolling back all the prepared xacts.  OTOH, if you or others think option (b) is good and doesn't add much complexity, then I think it is worth exploring that option.

I think we should definitely do something to deal with this even if you don't like the proposed options because the current behavior of the patch seems worse than either of these options.

2.
-DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
+DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> [ WITH ( <replaceable class="parameter">option</replaceable> [, ...] ) ]

It is better to keep WITH clause as optional similar to Copy command.  This might address Peter E's concern related to WITH clause.

WITH keyword is optional now
 

3.
- * DROP DATABASE [ IF EXISTS ]
+ * DROP DATABASE [ ( options ) ] [ IF EXISTS ]

fixed


You seem to forget to change the syntax in the above comments after changing the patch.

4.
+   If anyone else is connected to the target database, this command will fail
+   - unless you use the <literal>FORCE</literal> option described below.

I don't understand the significance of using '-' before unless.  I think we can remove it.

fixed


Changed the patch status as 'Waiting on Author'.

Thank you for careful review. I hope so all issues are out.

Regards

Pavel

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

Re: dropdb --force

vignesh C
On Wed, Oct 2, 2019 at 10:21 PM Pavel Stehule <[hidden email]> wrote:
>
> Thank you for careful review. I hope so all issues are out.
>
>
Thanks Pavel for fixing the comments.
Few comments:
The else part cannot be hit in DropDatabase function as gram.y expects FORCE.
+
+ if (strcmp(opt->defname, "force") == 0)
+ force = true;
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("unrecognized DROP DATABASE option \"%s\"", opt->defname),
+ parser_errposition(pstate, opt->location)));
+ }
+

We should change gram.y to accept any keyword and throw error from
DropDatabase function.
+ */
+drop_option: FORCE
+ {
+ $$ = makeDefElem("force", NULL, @1);
+ }
+ ;

"This will also fail" should be "This will fail"
+     <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 we are not able to terminate connections or
+      when there are active prepared transactions or active logical replication
+      slots.
+     </para>

Can we add few tests for this feature.

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


1234567