dropdb --force

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

Re: dropdb --force

Pavel Stehule


st 13. 11. 2019 v 15:34 odesílatel Pavel Stehule <[hidden email]> napsal:


st 13. 11. 2019 v 7:13 odesílatel Pavel Stehule <[hidden email]> napsal:


st 13. 11. 2019 v 7:12 odesílatel Amit Kapila <[hidden email]> napsal:
On Tue, Nov 12, 2019 at 11:17 AM Amit Kapila <[hidden email]> wrote:
>
> I am planning to commit this patch tomorrow unless I see more comments
> or interest from someone else to review this.
>

Pushed.  Pavel, feel free to submit dropdb utility-related patch if you want.

I hope I send this patch today. It's simply job.

here it is. It's based on Filip Rembialkowski's patch if I remember correctly

have I this patch assign to next commitfest or you process it in this commitfest?

This part is trivial

Pavel


Pavel



Pavel


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

Re: dropdb --force

akapila
On Thu, Nov 14, 2019 at 11:43 AM Pavel Stehule <[hidden email]> wrote:
>
> st 13. 11. 2019 v 15:34 odesílatel Pavel Stehule <[hidden email]> napsal:
>>
>>
>> here it is. It's based on Filip Rembialkowski's patch if I remember correctly
>
>
> have I this patch assign to next commitfest or you process it in this commitfest?
>

I will try to review in this CF only, but if I fail to do so, any way
you can register it in new CF after this CF.

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


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

akapila
On Thu, Nov 14, 2019 at 12:14 PM Amit Kapila <[hidden email]> wrote:

>
> On Thu, Nov 14, 2019 at 11:43 AM Pavel Stehule <[hidden email]> wrote:
> >
> > st 13. 11. 2019 v 15:34 odesílatel Pavel Stehule <[hidden email]> napsal:
> >>
> >>
> >> here it is. It's based on Filip Rembialkowski's patch if I remember correctly
> >
> >
> > have I this patch assign to next commitfest or you process it in this commitfest?
> >
>
> I will try to review in this CF only,
>

This is the reason I haven't yet marked the CF entry for this patch as
committed.

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


čt 14. 11. 2019 v 7:44 odesílatel Amit Kapila <[hidden email]> napsal:
On Thu, Nov 14, 2019 at 11:43 AM Pavel Stehule <[hidden email]> wrote:
>
> st 13. 11. 2019 v 15:34 odesílatel Pavel Stehule <[hidden email]> napsal:
>>
>>
>> here it is. It's based on Filip Rembialkowski's patch if I remember correctly
>
>
> have I this patch assign to next commitfest or you process it in this commitfest?
>

I will try to review in this CF only, but if I fail to do so, any way
you can register it in new CF after this CF.

ok.

there is enough long time to do.

Regards

Pavel

--
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 Wed, Nov 13, 2019 at 8:05 PM Pavel Stehule <[hidden email]> wrote:

>
>
>
> st 13. 11. 2019 v 7:13 odesílatel Pavel Stehule <[hidden email]> napsal:
>>
>>
>>
>> st 13. 11. 2019 v 7:12 odesílatel Amit Kapila <[hidden email]> napsal:
>>>
>>> On Tue, Nov 12, 2019 at 11:17 AM Amit Kapila <[hidden email]> wrote:
>>> >
>>> > I am planning to commit this patch tomorrow unless I see more comments
>>> > or interest from someone else to review this.
>>> >
>>>
>>> Pushed.  Pavel, feel free to submit dropdb utility-related patch if you want.
>>
>>
>> I hope I send this patch today. It's simply job.
>
>
> here it is. It's based on Filip Rembialkowski's patch if I remember correctly
>

Thanks for working on the patch.
Few minor comments:
+        Force termination of connected backends before removing the database.
+       </para>
+       <para>
+        This will add the <literal>FORCE</literal> option to the <literal>DROP
+        DATABASE</literal> command sent to the server.
+       </para>

The above documentation can be changed similar to drop_database.sgml:
     <para>
      Attempt to terminate all existing connections to the target database.
      It doesn't terminate if prepared transactions, active logical replication
      slots or subscriptions are present in the target database.
     </para>
     <para>
      This will fail if the 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.
     </para>

We can make the modification in the same location as earlier in the below case:
-    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,12 @@ main(int argc, char *argv[])
                                       host, port, username, prompt_password,
                                       progname, echo);

+    /* now, only FORCE option can be used, so usage is very simple */
+    appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
+                      (if_exists ? "IF EXISTS " : ""),
+                      fmtId(dbname),
+                      force ? " WITH (FORCE)" : "");
+

We can slightly rephrase the below:
+    printf(_("  -f, --force               force termination of
connected backends\n"));
can be changed to:
+    printf(_("  -f, --force               terminate the existing
connections to the target database forcefully\n"));

We can slightly rephrase the below:
+    /* now, only FORCE option can be used, so usage is very simple */
+    appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
can be changed to:
+    /* Generate drop db command using the options specified */
+    appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",

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


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


čt 14. 11. 2019 v 12:12 odesílatel vignesh C <[hidden email]> napsal:
On Wed, Nov 13, 2019 at 8:05 PM Pavel Stehule <[hidden email]> wrote:
>
>
>
> st 13. 11. 2019 v 7:13 odesílatel Pavel Stehule <[hidden email]> napsal:
>>
>>
>>
>> st 13. 11. 2019 v 7:12 odesílatel Amit Kapila <[hidden email]> napsal:
>>>
>>> On Tue, Nov 12, 2019 at 11:17 AM Amit Kapila <[hidden email]> wrote:
>>> >
>>> > I am planning to commit this patch tomorrow unless I see more comments
>>> > or interest from someone else to review this.
>>> >
>>>
>>> Pushed.  Pavel, feel free to submit dropdb utility-related patch if you want.
>>
>>
>> I hope I send this patch today. It's simply job.
>
>
> here it is. It's based on Filip Rembialkowski's patch if I remember correctly
>

Thanks for working on the patch.
Few minor comments:
+        Force termination of connected backends before removing the database.
+       </para>
+       <para>
+        This will add the <literal>FORCE</literal> option to the <literal>DROP
+        DATABASE</literal> command sent to the server.
+       </para>

The above documentation can be changed similar to drop_database.sgml:
     <para>
      Attempt to terminate all existing connections to the target database.
      It doesn't terminate if prepared transactions, active logical replication
      slots or subscriptions are present in the target database.
     </para>
     <para>
      This will fail if the 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.
     </para>


done
 
We can make the modification in the same location as earlier in the below case:
-    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,12 @@ main(int argc, char *argv[])
                                       host, port, username, prompt_password,
                                       progname, echo);

+    /* now, only FORCE option can be used, so usage is very simple */
+    appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
+                      (if_exists ? "IF EXISTS " : ""),
+                      fmtId(dbname),
+                      force ? " WITH (FORCE)" : "");
+


done
 
We can slightly rephrase the below:
+    printf(_("  -f, --force               force termination of
connected backends\n"));
can be changed to:
+    printf(_("  -f, --force               terminate the existing
connections to the target database forcefully\n"));

the proposed text is too long - I changed the sentence, and any other can fix it


We can slightly rephrase the below:
+    /* now, only FORCE option can be used, so usage is very simple */
+    appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
can be changed to:
+    /* Generate drop db command using the options specified */
+    appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",

I would to say, so generating is simple due only one supported option. If we support two or more options there, then the code should be more complex. I changed comment to

/* Currently, only FORCE option is supported */

updated patch attached

Thank you for review

Pavel


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

dropdb-f-20191115.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

vignesh C
On Fri, Nov 15, 2019 at 1:23 PM Pavel Stehule <[hidden email]> wrote:
>
> updated patch attached
>

Thanks Pavel  for providing updated version.
Few comments:
I felt the help text  seems incomplete:
@@ -159,6 +167,7 @@ help(const char *progname)
     printf(_("\nOptions:\n"));
     printf(_("  -e, --echo                show the commands being
sent to the server\n"));
     printf(_("  -i, --interactive         prompt before deleting anything\n"));
+    printf(_("  -f, --force               try to terminate other
connection before\n"));
     printf(_("  -V, --version             output version information,
then exit\n"));
we can change to:
printf(_("  -f, --force               try to terminate other
connection before dropping\n"));

We can add one test including -e option which validates the command
generation including WITH (FORCE):
+$node->safe_psql('postgres', 'CREATE DATABASE foobar2');
+$node->issues_sql_like(
+    [ 'dropdb', '--force', 'foobar2' ],
+    qr/statement: DROP DATABASE foobar2 WITH \(FORCE\);/,
+    'SQL DROP DATABASE (FORCE) run');
+

Also should we include one test where one session is connected to db
and another session tries dropping with -f option?

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


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


so 16. 11. 2019 v 1:10 odesílatel vignesh C <[hidden email]> napsal:
On Fri, Nov 15, 2019 at 1:23 PM Pavel Stehule <[hidden email]> wrote:
>
> updated patch attached
>

Thanks Pavel  for providing updated version.
Few comments:
I felt the help text  seems incomplete:
@@ -159,6 +167,7 @@ help(const char *progname)
     printf(_("\nOptions:\n"));
     printf(_("  -e, --echo                show the commands being
sent to the server\n"));
     printf(_("  -i, --interactive         prompt before deleting anything\n"));
+    printf(_("  -f, --force               try to terminate other
connection before\n"));
     printf(_("  -V, --version             output version information,
then exit\n"));
we can change to:
printf(_("  -f, --force               try to terminate other
connection before dropping\n"));


done. maybe alternative can be "first try to terminate other connections". It is shorter. The current text has 78 chars, what should be acceptable
 
We can add one test including -e option which validates the command
generation including WITH (FORCE):
+$node->safe_psql('postgres', 'CREATE DATABASE foobar2');
+$node->issues_sql_like(
+    [ 'dropdb', '--force', 'foobar2' ],
+    qr/statement: DROP DATABASE foobar2 WITH \(FORCE\);/,
+    'SQL DROP DATABASE (FORCE) run');
+

I don't understand to this point. It is effectively same like existing test


Also should we include one test where one session is connected to db
and another session tries dropping with -f option?

I afraid so test API doesn't allow asynchronous operations. Do you have any idea, how to it?

Regards

Pavel
 

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

dropdb-f-20191116.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

vignesh C
On Sat, Nov 16, 2019 at 1:25 PM Pavel Stehule <[hidden email]> wrote:

>> >
>> > updated patch attached
>> >
>>
>> Thanks Pavel  for providing updated version.
>> Few comments:
>> I felt the help text  seems incomplete:
>> @@ -159,6 +167,7 @@ help(const char *progname)
>>      printf(_("\nOptions:\n"));
>>      printf(_("  -e, --echo                show the commands being
>> sent to the server\n"));
>>      printf(_("  -i, --interactive         prompt before deleting anything\n"));
>> +    printf(_("  -f, --force               try to terminate other
>> connection before\n"));
>>      printf(_("  -V, --version             output version information,
>> then exit\n"));
>> we can change to:
>> printf(_("  -f, --force               try to terminate other
>> connection before dropping\n"));
>>
>
> done. maybe alternative can be "first try to terminate other connections". It is shorter. The current text has 78 chars, what should be acceptable
>
>>
>> We can add one test including -e option which validates the command
>> generation including WITH (FORCE):
>> +$node->safe_psql('postgres', 'CREATE DATABASE foobar2');
>> +$node->issues_sql_like(
>> +    [ 'dropdb', '--force', 'foobar2' ],
>> +    qr/statement: DROP DATABASE foobar2 WITH \(FORCE\);/,
>> +    'SQL DROP DATABASE (FORCE) run');
>> +
>
>
> I don't understand to this point. It is effectively same like existing test
>

When we don't specify -e option, the query used to drop db will not be
printed like below:
./dropdb testdb1
When we specify -e option, the query used to drop db will be printed like below:
./dropdb -e testdb2
SELECT pg_catalog.set_config('search_path', '', false);
DROP DATABASE testdb2;
If we specify -e option, the query that is being used to drop db will
be printed. In the existing test I could not see the inclusion of -e
option. I was thinking to add a test including -e that way the query
that includes force option gets validated.

>>
>> Also should we include one test where one session is connected to db
>> and another session tries dropping with -f option?
>
>
> I afraid so test API doesn't allow asynchronous operations. Do you have any idea, how to it?
>

I had seen that isolation test(src/test/isolation) has a framework to
support this. You can have a look to see if it can be handled using
that.

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


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


po 18. 11. 2019 v 4:43 odesílatel vignesh C <[hidden email]> napsal:
On Sat, Nov 16, 2019 at 1:25 PM Pavel Stehule <[hidden email]> wrote:
>> >
>> > updated patch attached
>> >
>>
>> Thanks Pavel  for providing updated version.
>> Few comments:
>> I felt the help text  seems incomplete:
>> @@ -159,6 +167,7 @@ help(const char *progname)
>>      printf(_("\nOptions:\n"));
>>      printf(_("  -e, --echo                show the commands being
>> sent to the server\n"));
>>      printf(_("  -i, --interactive         prompt before deleting anything\n"));
>> +    printf(_("  -f, --force               try to terminate other
>> connection before\n"));
>>      printf(_("  -V, --version             output version information,
>> then exit\n"));
>> we can change to:
>> printf(_("  -f, --force               try to terminate other
>> connection before dropping\n"));
>>
>
> done. maybe alternative can be "first try to terminate other connections". It is shorter. The current text has 78 chars, what should be acceptable
>
>>
>> We can add one test including -e option which validates the command
>> generation including WITH (FORCE):
>> +$node->safe_psql('postgres', 'CREATE DATABASE foobar2');
>> +$node->issues_sql_like(
>> +    [ 'dropdb', '--force', 'foobar2' ],
>> +    qr/statement: DROP DATABASE foobar2 WITH \(FORCE\);/,
>> +    'SQL DROP DATABASE (FORCE) run');
>> +
>
>
> I don't understand to this point. It is effectively same like existing test
>

When we don't specify -e option, the query used to drop db will not be
printed like below:
./dropdb testdb1
When we specify -e option, the query used to drop db will be printed like below:
./dropdb -e testdb2
SELECT pg_catalog.set_config('search_path', '', false);
DROP DATABASE testdb2;
If we specify -e option, the query that is being used to drop db will
be printed. In the existing test I could not see the inclusion of -e
option. I was thinking to add a test including -e that way the query
that includes force option gets validated.

still I don't understand. The created query is tested already by current test.

Do you want to test just -e option? Then it should be done as separate issue. Do this now is little bit messy.


>>
>> Also should we include one test where one session is connected to db
>> and another session tries dropping with -f option?
>
>
> I afraid so test API doesn't allow asynchronous operations. Do you have any idea, how to it?
>

I had seen that isolation test(src/test/isolation) has a framework to
support this. You can have a look to see if it can be handled using
that.

I'll look there


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

Re: dropdb --force

akapila
On Mon, Nov 18, 2019 at 10:33 AM Pavel Stehule <[hidden email]> wrote:

>
> po 18. 11. 2019 v 4:43 odesílatel vignesh C <[hidden email]> napsal:
>>
>>
>> When we don't specify -e option, the query used to drop db will not be
>> printed like below:
>> ./dropdb testdb1
>> When we specify -e option, the query used to drop db will be printed like below:
>> ./dropdb -e testdb2
>> SELECT pg_catalog.set_config('search_path', '', false);
>> DROP DATABASE testdb2;
>> If we specify -e option, the query that is being used to drop db will
>> be printed. In the existing test I could not see the inclusion of -e
>> option. I was thinking to add a test including -e that way the query
>> that includes force option gets validated.
>
>
> still I don't understand. The created query is tested already by current test.
>
> Do you want to test just -e option?
>
Yeah, it seems Vignesh wants to do that.  It will help in verifying
that the command generated by code is correct.  However, I think there
is no pressing need to have an additional test for this.

> Then it should be done as separate issue.
>

Yeah, I agree.  I think this can be done as a separate test patch to
improve coverage if someone wants.

>>
>> >>
>> >> Also should we include one test where one session is connected to db
>> >> and another session tries dropping with -f option?
>> >
>> >
>> > I afraid so test API doesn't allow asynchronous operations. Do you have any idea, how to it?
>> >
>>
>> I had seen that isolation test(src/test/isolation) has a framework to
>> support this. You can have a look to see if it can be handled using
>> that.
>
>
> I'll look there
>
If we want to have a test for this, then you might want to look at
test src/test/recovery/t/013_crash_restart.  In that test, we keep a
connection open and then validate whether it is terminated.  Having
said that, I think it might be better to add this as a separate test
patch apart from main patch because it is a bit of a timing-dependent
test and might fail on some slow machines.  We can always revert this
if it turns out to be an unstable test.

I have slightly modified the main patch and attached is the result.
Basically, I don't see any need to repeat what is mentioned in the
Drop Database page.  Let me know what you guys think?

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

dropdb-f-20191118.amit.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


po 18. 11. 2019 v 6:24 odesílatel Amit Kapila <[hidden email]> napsal:
On Mon, Nov 18, 2019 at 10:33 AM Pavel Stehule <[hidden email]> wrote:
>
> po 18. 11. 2019 v 4:43 odesílatel vignesh C <[hidden email]> napsal:
>>
>>
>> When we don't specify -e option, the query used to drop db will not be
>> printed like below:
>> ./dropdb testdb1
>> When we specify -e option, the query used to drop db will be printed like below:
>> ./dropdb -e testdb2
>> SELECT pg_catalog.set_config('search_path', '', false);
>> DROP DATABASE testdb2;
>> If we specify -e option, the query that is being used to drop db will
>> be printed. In the existing test I could not see the inclusion of -e
>> option. I was thinking to add a test including -e that way the query
>> that includes force option gets validated.
>
>
> still I don't understand. The created query is tested already by current test.
>
> Do you want to test just -e option?
>

Yeah, it seems Vignesh wants to do that.  It will help in verifying
that the command generated by code is correct.  However, I think there
is no pressing need to have an additional test for this.

> Then it should be done as separate issue.
>

Yeah, I agree.  I think this can be done as a separate test patch to
improve coverage if someone wants.

>>
>> >>
>> >> Also should we include one test where one session is connected to db
>> >> and another session tries dropping with -f option?
>> >
>> >
>> > I afraid so test API doesn't allow asynchronous operations. Do you have any idea, how to it?
>> >
>>
>> I had seen that isolation test(src/test/isolation) has a framework to
>> support this. You can have a look to see if it can be handled using
>> that.
>
>
> I'll look there
>

If we want to have a test for this, then you might want to look at
test src/test/recovery/t/013_crash_restart.  In that test, we keep a
connection open and then validate whether it is terminated.  Having
said that, I think it might be better to add this as a separate test
patch apart from main patch because it is a bit of a timing-dependent
test and might fail on some slow machines.  We can always revert this
if it turns out to be an unstable test.

+1


I have slightly modified the main patch and attached is the result.
Basically, I don't see any need to repeat what is mentioned in the
Drop Database page.  Let me know what you guys think?

+ 1 from me - all has sense

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

Re: dropdb --force

akapila
On Mon, Nov 18, 2019 at 10:59 AM Pavel Stehule <[hidden email]> wrote:

> po 18. 11. 2019 v 6:24 odesílatel Amit Kapila <[hidden email]> napsal:
>>
>> On Mon, Nov 18, 2019 at 10:33 AM Pavel Stehule <[hidden email]> wrote:
>> >
>> > po 18. 11. 2019 v 4:43 odesílatel vignesh C <[hidden email]> napsal:
>> >>
>> >>
>> >> I had seen that isolation test(src/test/isolation) has a framework to
>> >> support this. You can have a look to see if it can be handled using
>> >> that.
>> >
>> >
>> > I'll look there
>> >
>>
>> If we want to have a test for this, then you might want to look at
>> test src/test/recovery/t/013_crash_restart.  In that test, we keep a
>> connection open and then validate whether it is terminated.  Having
>> said that, I think it might be better to add this as a separate test
>> patch apart from main patch because it is a bit of a timing-dependent
>> test and might fail on some slow machines.  We can always revert this
>> if it turns out to be an unstable test.
>
>
> +1
>

So, are you planning to give it a try?  I think even if we want to
commit this separately, it is better to have a test for this before we
commit.


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


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


po 18. 11. 2019 v 7:37 odesílatel Amit Kapila <[hidden email]> napsal:
On Mon, Nov 18, 2019 at 10:59 AM Pavel Stehule <[hidden email]> wrote:
> po 18. 11. 2019 v 6:24 odesílatel Amit Kapila <[hidden email]> napsal:
>>
>> On Mon, Nov 18, 2019 at 10:33 AM Pavel Stehule <[hidden email]> wrote:
>> >
>> > po 18. 11. 2019 v 4:43 odesílatel vignesh C <[hidden email]> napsal:
>> >>
>> >>
>> >> I had seen that isolation test(src/test/isolation) has a framework to
>> >> support this. You can have a look to see if it can be handled using
>> >> that.
>> >
>> >
>> > I'll look there
>> >
>>
>> If we want to have a test for this, then you might want to look at
>> test src/test/recovery/t/013_crash_restart.  In that test, we keep a
>> connection open and then validate whether it is terminated.  Having
>> said that, I think it might be better to add this as a separate test
>> patch apart from main patch because it is a bit of a timing-dependent
>> test and might fail on some slow machines.  We can always revert this
>> if it turns out to be an unstable test.
>
>
> +1
>

So, are you planning to give it a try?  I think even if we want to
commit this separately, it is better to have a test for this before we
commit.

I'll send this test today

Pavel



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

Re: dropdb --force

Pavel Stehule


po 18. 11. 2019 v 7:39 odesílatel Pavel Stehule <[hidden email]> napsal:


po 18. 11. 2019 v 7:37 odesílatel Amit Kapila <[hidden email]> napsal:
On Mon, Nov 18, 2019 at 10:59 AM Pavel Stehule <[hidden email]> wrote:
> po 18. 11. 2019 v 6:24 odesílatel Amit Kapila <[hidden email]> napsal:
>>
>> On Mon, Nov 18, 2019 at 10:33 AM Pavel Stehule <[hidden email]> wrote:
>> >
>> > po 18. 11. 2019 v 4:43 odesílatel vignesh C <[hidden email]> napsal:
>> >>
>> >>
>> >> I had seen that isolation test(src/test/isolation) has a framework to
>> >> support this. You can have a look to see if it can be handled using
>> >> that.
>> >
>> >
>> > I'll look there
>> >
>>
>> If we want to have a test for this, then you might want to look at
>> test src/test/recovery/t/013_crash_restart.  In that test, we keep a
>> connection open and then validate whether it is terminated.  Having
>> said that, I think it might be better to add this as a separate test
>> patch apart from main patch because it is a bit of a timing-dependent
>> test and might fail on some slow machines.  We can always revert this
>> if it turns out to be an unstable test.
>
>
> +1
>

So, are you planning to give it a try?  I think even if we want to
commit this separately, it is better to have a test for this before we
commit.

I'll send this test today

here is it

Regards

Pavel

Pavel



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

dropdb-force2.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

akapila
In reply to this post by Pavel Stehule
On Mon, Nov 18, 2019 at 10:59 AM Pavel Stehule <[hidden email]> wrote:

> po 18. 11. 2019 v 6:24 odesílatel Amit Kapila <[hidden email]> napsal:
>>
>>
>> I have slightly modified the main patch and attached is the result.
>> Basically, I don't see any need to repeat what is mentioned in the
>> Drop Database page.  Let me know what you guys think?
>
>
> + 1 from me - all has sense
>

I have pushed this patch.  The only remaining patch left now is a test
case patch.

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


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


st 20. 11. 2019 v 12:40 odesílatel Amit Kapila <[hidden email]> napsal:
On Mon, Nov 18, 2019 at 10:59 AM Pavel Stehule <[hidden email]> wrote:
> po 18. 11. 2019 v 6:24 odesílatel Amit Kapila <[hidden email]> napsal:
>>
>>
>> I have slightly modified the main patch and attached is the result.
>> Basically, I don't see any need to repeat what is mentioned in the
>> Drop Database page.  Let me know what you guys think?
>
>
> + 1 from me - all has sense
>

I have pushed this patch.  The only remaining patch left now is a test
case patch.

Thank you

Pavel

--
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 Mon, Nov 18, 2019 at 6:30 PM Pavel Stehule <[hidden email]> wrote:
>>
>> I'll send this test today
>
>
> here is it
>

Thanks for adding the test.
Few comments:
This function is same as in test/recovery/t/013_crash_restart.pl, we
can add to a common file and use in both places:
+# Pump until string is matched, or timeout occurs
+sub pump_until
+{
+    my ($proc, $stream, $untl) = @_;
+    $proc->pump_nb();
+    while (1)
+    {
+        last if $$stream =~ /$untl/;
+        if ($psql_timeout->is_expired)
+        {

This can be Tests drop database with force option:
+#
+# Tests killing session connected to dropped database
+#

This can be changed to check database foobar1 does not exist.
+# and there is not a database with this name
+is($node->safe_psql('postgres', qq[SELECT EXISTS(SELECT * FROM
pg_database WHERE datname='foobar1');]),
+    'f', 'database foobar1 was removed');

This can be changed to check the connections on foobar1 database
+
+# and it is connected to foobar1 database
+is($node->safe_psql('postgres', qq[SELECT pid FROM pg_stat_activity
WHERE datname='foobar1' AND pid = $pid;]),
+    $pid, 'database foobar1 is used');

This can be changed to restart psql as the previous connection will be
terminated by previous drop database.
+
+# restart psql processes, now that the crash cycle finished
+($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+$killme->run();

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


Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

Pavel Stehule


čt 21. 11. 2019 v 6:33 odesílatel vignesh C <[hidden email]> napsal:
On Mon, Nov 18, 2019 at 6:30 PM Pavel Stehule <[hidden email]> wrote:
>>
>> I'll send this test today
>
>
> here is it
>

Thanks for adding the test.
Few comments:
This function is same as in test/recovery/t/013_crash_restart.pl, we
can add to a common file and use in both places:
+# Pump until string is matched, or timeout occurs
+sub pump_until
+{
+    my ($proc, $stream, $untl) = @_;
+    $proc->pump_nb();
+    while (1)
+    {
+        last if $$stream =~ /$untl/;
+        if ($psql_timeout->is_expired)
+        {

yes, I know - probably it can be moved to testlib.pm. Unfortunately it is perl, and I am not able to this correctly. More it use global object - timer

This can be Tests drop database with force option:

done

+#
+# Tests killing session connected to dropped database
+#

This can be changed to check database foobar1 does not exist.

done

+# and there is not a database with this name
+is($node->safe_psql('postgres', qq[SELECT EXISTS(SELECT * FROM
pg_database WHERE datname='foobar1');]),
+    'f', 'database foobar1 was removed');

This can be changed to check the connections on foobar1 database
+
+# and it is connected to foobar1 database
+is($node->safe_psql('postgres', qq[SELECT pid FROM pg_stat_activity
WHERE datname='foobar1' AND pid = $pid;]),
+    $pid, 'database foobar1 is used');

done


This can be changed to restart psql as the previous connection will be
terminated by previous drop database.
+

done

new patch attached

Regards

Pavel


+# restart psql processes, now that the crash cycle finished
+($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+$killme->run();

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

dropdb-force.pl-patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: dropdb --force

vignesh C
On Fri, Nov 22, 2019 at 12:10 AM Pavel Stehule <[hidden email]> wrote:

>
>
>
> čt 21. 11. 2019 v 6:33 odesílatel vignesh C <[hidden email]> napsal:
>>
>> On Mon, Nov 18, 2019 at 6:30 PM Pavel Stehule <[hidden email]> wrote:
>> >>
>> >> I'll send this test today
>> >
>> >
>> > here is it
>> >
>>
>> Thanks for adding the test.
>> Few comments:
>> This function is same as in test/recovery/t/013_crash_restart.pl, we
>> can add to a common file and use in both places:
>> +# Pump until string is matched, or timeout occurs
>> +sub pump_until
>> +{
>> +    my ($proc, $stream, $untl) = @_;
>> +    $proc->pump_nb();
>> +    while (1)
>> +    {
>> +        last if $$stream =~ /$untl/;
>> +        if ($psql_timeout->is_expired)
>> +        {
>
>
> yes, I know - probably it can be moved to testlib.pm. Unfortunately it is perl, and I am not able to this correctly. More it use global object - timer
>
>> This can be Tests drop database with force option:
>
>
> done
>
>> +#
>> +# Tests killing session connected to dropped database
>> +#
>>
>> This can be changed to check database foobar1 does not exist.
>
>
> done
>
>> +# and there is not a database with this name
>> +is($node->safe_psql('postgres', qq[SELECT EXISTS(SELECT * FROM
>> pg_database WHERE datname='foobar1');]),
>> +    'f', 'database foobar1 was removed');
>>
>> This can be changed to check the connections on foobar1 database
>> +
>> +# and it is connected to foobar1 database
>> +is($node->safe_psql('postgres', qq[SELECT pid FROM pg_stat_activity
>> WHERE datname='foobar1' AND pid = $pid;]),
>> +    $pid, 'database foobar1 is used');
>
>
> done
>
>>
>> This can be changed to restart psql as the previous connection will be
>> terminated by previous drop database.
>> +
>
>
> done
>
> new patch attached
>
Thanks for fixing the comments. The changes looks fine to me. I have
fixed the first comment, attached patch has the changes for the same.

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

0001-Add-tests-for-the-support-of-f-option-in-dropdb-util.patch (5K) Download Attachment
0002-Made-pump_until-usable-as-a-common-subroutine.patch (8K) Download Attachment
1 ... 34567