Re: dropdb --force

[Amit Kapila]

On Sat, Nov 30, 2019 at 7:46 AM Michael Paquier  wrote:
>
> On Fri, Nov 29, 2019 at 03:31:21PM +0530, Amit Kapila wrote:
> > It might be better to move it to next CF as the discussion is still active.
>
> OK, just did that.
>

I have marked this as committed in CF.  This was committed some time
back[1][2].  I was just waiting for the conclusion on what we want to
do about Windows behavior related to socket closure which we
discovered while testing this feature.  It is not very clear whether
we want to do anything about it, see discussion on thread [3], so I
closed this.

[1] - 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=1379fd537f9fc7941c8acff8c879ce3636dbdb77
[2] - 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=80e05a088e4edd421c9c0374d54d787c8a4c0d86
[3] - 
https://www.postgresql.org/message-id/CALDaNm2tEvr_Kum7SyvFn0%3D6H3P0P-Zkhnd%3DdkkX%2BQ%3DwKutZ%3DA%40mail.gmail.com

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

Re: dropdb --force

[Michael Paquier]

On Fri, Nov 29, 2019 at 03:31:21PM +0530, Amit Kapila wrote:
> It might be better to move it to next CF as the discussion is still active.

OK, just did that.
--
Michael


signature.asc
Description: PGP signature

Re: dropdb --force

[vignesh C]

On Fri, Nov 29, 2019 at 1:36 PM Juan José Santamaría Flecha
 wrote:
>
>
>
> On Fri, Nov 29, 2019 at 7:30 AM Michael Paquier  wrote:
>>
>> On Thu, Nov 28, 2019 at 08:53:56AM +0530, Amit Kapila wrote:
>> > I have pushed the refactoring patch.  In the second patch, I have a
>> > few more comments.  I am not completely sure if it is a good idea to
>> > write a new test file 060_dropdb_force.pl when we already have an
>> > existing file 050_dropdb.pl for dropdb tests, but I think if we want
>> > to do that, then lets move existing test for dropdb '-f' from
>> > 050_dropdb.pl to new file and it might be better to name new file as
>> > 051_dropdb_force.pl.  I see that in some other cases like vacuumdb and
>> > clusterdb, we have separate test files to cover a different kinds of
>> > scenarios, so it should be okay to have a new file for dropdb tests.
>>
>> Amit, as most of the patch set has been committed, would it make sense
>> to mark this entry as committed in the CF app?
>>
>
> Test 051_dropdb_force.pl is failing on Windows critters in the build farm, 
> e.g:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo=2019-11-29%2003%3A54%3A06
>

Attached patch includes the fix for the following failure in buildfarm:
Nov 28 09:00:01 #   Failed test 'database foobar1 is used'
Nov 28 09:00:01 #   at t/051_dropdb_force.pl line 71.
Nov 28 09:00:01 #  got: '7380'
Nov 28 09:00:01 # expected: '7380
'
Nov 28 09:00:01 # aborting wait: program died

This test passes in most buildfarm environment, but it fails in few
windows environment randomly. The  attached patch removes the query
which is not really needed for this test. Alternatively we could also
modify something like below as in PostgresNode.pm:
$pid =~ s/\r//g if $TestLib::windows_os;
I do not have an environment in which I could reproduce and I felt
this is not really needed as part of this testcase.

Any thoughts?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
diff --git a/src/bin/scripts/t/051_dropdb_force.pl b/src/bin/scripts/t/051_dropdb_force.pl
index a252b43..5326395 100644
--- a/src/bin/scripts/t/051_dropdb_force.pl
+++ b/src/bin/scripts/t/051_dropdb_force.pl
@@ -6,7 +6,7 @@ use warnings;
 
 use PostgresNode;
 use TestLib;
-use Test::More tests => 9;
+use Test::More tests => 8;
 
 # To avoid hanging while expecting some specific input from a psql
 # instance being driven by us, add a timeout high enough that it
@@ -67,14 +67,6 @@ chomp($pid);
 $killme_stdout = '';
 $killme_stderr = '';
 
-# Check the connections on 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');
-
 # Now drop database with dropdb --force command.
 $node->issues_sql_like(
 	[ 'dropdb', '--force', 'foobar1' ],

Re: dropdb --force

[Amit Kapila]

On Fri, Nov 29, 2019 at 1:36 PM Juan José Santamaría Flecha
 wrote:
>
>
> On Fri, Nov 29, 2019 at 7:30 AM Michael Paquier  wrote:
>>
>> On Thu, Nov 28, 2019 at 08:53:56AM +0530, Amit Kapila wrote:
>> > I have pushed the refactoring patch.  In the second patch, I have a
>> > few more comments.  I am not completely sure if it is a good idea to
>> > write a new test file 060_dropdb_force.pl when we already have an
>> > existing file 050_dropdb.pl for dropdb tests, but I think if we want
>> > to do that, then lets move existing test for dropdb '-f' from
>> > 050_dropdb.pl to new file and it might be better to name new file as
>> > 051_dropdb_force.pl.  I see that in some other cases like vacuumdb and
>> > clusterdb, we have separate test files to cover a different kinds of
>> > scenarios, so it should be okay to have a new file for dropdb tests.
>>
>> Amit, as most of the patch set has been committed, would it make sense
>> to mark this entry as committed in the CF app?
>>

It might be better to move it to next CF as the discussion is still active.

>
> Test 051_dropdb_force.pl is failing on Windows critters in the build farm, 
> e.g:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo=2019-11-29%2003%3A54%3A06
>

Yeah,  I have proposed something for it on pgsql-committers [1].

[1] - 
https://www.postgresql.org/message-id/CAA4eK1%2BGNyjaPK77y%2Beuh5eAgM75pncG1JYZhxYZF%2BSgS6NpjA%40mail.gmail.com

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

Re: dropdb --force

[Juan José Santamaría Flecha]

On Fri, Nov 29, 2019 at 7:30 AM Michael Paquier  wrote:

> On Thu, Nov 28, 2019 at 08:53:56AM +0530, Amit Kapila wrote:
> > I have pushed the refactoring patch.  In the second patch, I have a
> > few more comments.  I am not completely sure if it is a good idea to
> > write a new test file 060_dropdb_force.pl when we already have an
> > existing file 050_dropdb.pl for dropdb tests, but I think if we want
> > to do that, then lets move existing test for dropdb '-f' from
> > 050_dropdb.pl to new file and it might be better to name new file as
> > 051_dropdb_force.pl.  I see that in some other cases like vacuumdb and
> > clusterdb, we have separate test files to cover a different kinds of
> > scenarios, so it should be okay to have a new file for dropdb tests.
>
> Amit, as most of the patch set has been committed, would it make sense
> to mark this entry as committed in the CF app?
>
>
Test 051_dropdb_force.pl is failing on Windows critters in the build farm,
e.g:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo=2019-11-29%2003%3A54%3A06


Regards,

Juan José Santamaría Flecha

Re: dropdb --force

[Michael Paquier]

On Thu, Nov 28, 2019 at 08:53:56AM +0530, Amit Kapila wrote:
> I have pushed the refactoring patch.  In the second patch, I have a
> few more comments.  I am not completely sure if it is a good idea to
> write a new test file 060_dropdb_force.pl when we already have an
> existing file 050_dropdb.pl for dropdb tests, but I think if we want
> to do that, then lets move existing test for dropdb '-f' from
> 050_dropdb.pl to new file and it might be better to name new file as
> 051_dropdb_force.pl.  I see that in some other cases like vacuumdb and
> clusterdb, we have separate test files to cover a different kinds of
> scenarios, so it should be okay to have a new file for dropdb tests.

Amit, as most of the patch set has been committed, would it make sense
to mark this entry as committed in the CF app?
--
Michael


signature.asc
Description: PGP signature

Re: dropdb --force

[vignesh C]

On Thu, Nov 28, 2019 at 8:54 AM Amit Kapila  wrote:
>
> On Wed, Nov 27, 2019 at 10:15 AM vignesh C  wrote:
> >
> >
> > Attached patch has the fixes for the above comments.
> >
>
> I have pushed the refactoring patch.  In the second patch, I have a
> few more comments.  I am not completely sure if it is a good idea to
> write a new test file 060_dropdb_force.pl when we already have an
> existing file 050_dropdb.pl for dropdb tests, but I think if we want
> to do that, then lets move existing test for dropdb '-f' from
> 050_dropdb.pl to new file and it might be better to name new file as
> 051_dropdb_force.pl.  I see that in some other cases like vacuumdb and
> clusterdb, we have separate test files to cover a different kinds of
> scenarios, so it should be okay to have a new file for dropdb tests.
>

Thanks for pushing the patch. Please find the attached patch having
the fixes for the comments suggested.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
From 21423c5e14a4279722d93866857910cbf86b5bff Mon Sep 17 00:00:00 2001
From: Pavel Stehule , Vignesh
Date: Thu, 28 Nov 2019 09:58:11 +0530
Subject: [PATCH] Add tests for the support of '-f' option in dropdb utility.

Add tests for the support of '-f' option in dropdb utility.
---
 src/bin/scripts/t/050_dropdb.pl   |   8 +--
 src/bin/scripts/t/051_dropdb_force.pl | 100 ++
 2 files changed, 101 insertions(+), 7 deletions(-)
 create mode 100644 src/bin/scripts/t/051_dropdb_force.pl

diff --git a/src/bin/scripts/t/050_dropdb.pl b/src/bin/scripts/t/050_dropdb.pl
index c51babe..25aa54a 100644
--- a/src/bin/scripts/t/050_dropdb.pl
+++ b/src/bin/scripts/t/050_dropdb.pl
@@ -3,7 +3,7 @@ use warnings;
 
 use PostgresNode;
 use TestLib;
-use Test::More tests => 13;
+use Test::More tests => 11;
 
 program_help_ok('dropdb');
 program_version_ok('dropdb');
@@ -19,11 +19,5 @@ $node->issues_sql_like(
 	qr/statement: DROP DATABASE foobar1/,
 	'SQL DROP DATABASE run');
 
-$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');
-
 $node->command_fails([ 'dropdb', 'nonexistent' ],
 	'fails with nonexistent database');
diff --git a/src/bin/scripts/t/051_dropdb_force.pl b/src/bin/scripts/t/051_dropdb_force.pl
new file mode 100644
index 000..0f32dc5
--- /dev/null
+++ b/src/bin/scripts/t/051_dropdb_force.pl
@@ -0,0 +1,100 @@
+#
+# Tests drop database with force option.
+#
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More tests => 9;
+
+# To avoid hanging while expecting some specific input from a psql
+# instance being driven by us, add a timeout high enough that it
+# should never trigger even on very slow machines, unless something
+# is really wrong.
+my $psql_timeout = IPC::Run::timer(60);
+
+my $node = get_new_node('master');
+$node->init;
+$node->start;
+
+# Create database that will be dropped.
+$node->safe_psql('postgres', 'CREATE DATABASE foobar');
+$node->issues_sql_like(
+[ 'dropdb', '--force', 'foobar' ],
+qr/statement: DROP DATABASE foobar WITH \(FORCE\);/,
+'SQL DROP DATABASE (FORCE) run');
+
+# Check database foobar does not exist.
+is( $node->safe_psql(
+		'postgres',
+		qq[SELECT EXISTS(SELECT * FROM pg_database WHERE datname='foobar');]
+	),
+	'f',
+	'database foobar was removed');
+
+# Create database that will be dropped.
+$node->safe_psql('postgres', 'CREATE DATABASE foobar1');
+
+# Run psql, keeping session alive, so we have an alive backend to kill.
+my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+my $killme = IPC::Run::start(
+	[
+		'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d',
+		$node->connstr('foobar1')
+	],
+	'<',
+	\$killme_stdin,
+	'>',
+	\$killme_stdout,
+	'2>',
+	\$killme_stderr,
+	$psql_timeout);
+
+# Ensure killme process is active.
+$killme_stdin .= q[
+SELECT pg_backend_pid();
+];
+ok( TestLib::pump_until(
+		$killme, $psql_timeout, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m),
+	'acquired pid for SIGTERM');
+my $pid = $killme_stdout;
+chomp($pid);
+$killme_stdout = '';
+$killme_stderr = '';
+
+# Check the connections on 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');
+
+# Now drop database with dropdb --force command.
+$node->issues_sql_like(
+	[ 'dropdb', '--force', 'foobar1' ],
+	qr/statement: DROP DATABASE foobar1 WITH \(FORCE\);/,
+	'SQL DROP DATABASE (FORCE) run');
+
+# Check that psql sees the killed backend as having been terminated.
+$killme_stdin .= q[
+SELECT 1;
+];
+ok( TestLib::pump_until(
+		$killme, $psql_timeout, \$killme_stderr,
+		qr/FATAL:  terminating connection due to administrator command/m),
+	"psql query died successfully after SIGTERM");
+$killme_stderr = '';
+$killme_stdout = '';
+$killme->finish;
+
+# 

Re: dropdb --force

[Amit Kapila]

On Wed, Nov 27, 2019 at 10:15 AM vignesh C  wrote:
>
>
> Attached patch has the fixes for the above comments.
>

I have pushed the refactoring patch.  In the second patch, I have a
few more comments.  I am not completely sure if it is a good idea to
write a new test file 060_dropdb_force.pl when we already have an
existing file 050_dropdb.pl for dropdb tests, but I think if we want
to do that, then lets move existing test for dropdb '-f' from
050_dropdb.pl to new file and it might be better to name new file as
051_dropdb_force.pl.  I see that in some other cases like vacuumdb and
clusterdb, we have separate test files to cover a different kinds of
scenarios, so it should be okay to have a new file for dropdb tests.

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

Re: dropdb --force

[vignesh C]

On Tue, Nov 26, 2019 at 11:37 AM Amit Kapila  wrote:
>
> On Mon, Nov 25, 2019 at 11:22 PM vignesh C  wrote:
> >
> > On Sun, Nov 24, 2019 at 5:06 PM Pavel Stehule  
> > wrote:
> > >
> > >
> > >
> > > ne 24. 11. 2019 v 11:25 odesílatel vignesh C  napsal:
> > >>
> > >> On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila  
> > >> wrote:
> > >> >
> > >> > On Fri, Nov 22, 2019 at 3:16 PM vignesh C  wrote:
> > >> > >
> > >> > > 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.
> > >> > >
> > >> >
> > >> > Few comments:
> > >> > --
> > >> > 1. There is a lot of duplicate code in this test.  Basically, almost
> > >> > the same code is used once to test Drop Database command and dropdb.
> > >> > I think we can create a few sub-functions to avoid duplicate code, but
> > >> > do we really need to test the same thing once via command and then by
> > >> > dropdb utility?   I don't think it is required, but even if we want to
> > >> > do so, then I am not sure if this is the right test script.  I suggest
> > >> > removing the command related test.
> > >> >
> > >>
> > >> Pavel: What is your opinion on this?
> > >
> > >
> > > I have not any problem with this reduction.
> > >
> > > We will see in future years what is benefit of this test.
> > >
> >
> > Fixed, removed dropdb utility test.
> >
>
> Hmm, you have done the opposite of what I have asked.   This test file
> is in rc/bin/scripts/t/  where we generally keep the tests for
> utilities. So, retaining the dropdb utility test in that file seems
> more sensible.
>

Fixed. Retained the test of dropdb utility and removed drop database
sql command test.

> +ok( TestLib::pump_until(
> + $killme, $psql_timeout, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m),
> + 'acquired pid');
>
> How about changing 'acquired pid' to 'acquired pid for SIGTERM'?
>

Fixed. Changed as suggested.

> > >> >
> > >>
> > >> I have verified by running perltidy.
> > >>
>
> I think we don't need to run perltidy on the existing code.  So, I am
> not sure if it is a good idea to run it for file 013_crash_restart.pl
> as it changes some existing code formatting.
>

I have retained the format same as old format, one additional change I
added was to break the line if the line is lengthy in the modified
code.

Attached patch has the fixes for the above comments.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
From ef60824cd15b3c4cab1fc7fb26f79e5243d8cb7b Mon Sep 17 00:00:00 2001
From: Pavel Stehule 
Date: Wed, 27 Nov 2019 09:18:26 +0530
Subject: [PATCH 2/2] Add tests for the support of '-f' option in dropdb
 utility.

Add tests for the support of '-f' option in dropdb utility.
---
 src/bin/scripts/t/060_dropdb_force.pl | 84 +++
 1 file changed, 84 insertions(+)
 create mode 100644 src/bin/scripts/t/060_dropdb_force.pl

diff --git a/src/bin/scripts/t/060_dropdb_force.pl b/src/bin/scripts/t/060_dropdb_force.pl
new file mode 100644
index 000..0a1f612
--- /dev/null
+++ b/src/bin/scripts/t/060_dropdb_force.pl
@@ -0,0 +1,84 @@
+#
+# Tests drop database with force option.
+#
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 6;
+
+# To avoid hanging while expecting some specific input from a psql
+# instance being driven by us, add a timeout high enough that it
+# should never trigger even on very slow machines, unless something
+# is really wrong.
+my $psql_timeout = IPC::Run::timer(60);
+
+my $node = get_new_node('master');
+$node->init;
+$node->start;
+
+# Create database that will be dropped.
+$node->safe_psql('postgres', 'CREATE DATABASE foobar1');
+
+# Run psql, keeping session alive, so we have an alive backend to kill.
+my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+my $killme = IPC::Run::start(
+	[
+		'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d',
+		$node->connstr('foobar1')
+	],
+	'<',
+	\$killme_stdin,
+	'>',
+	\$killme_stdout,
+	'2>',
+	\$killme_stderr,
+	$psql_timeout);
+
+# Ensure killme process is active.
+$killme_stdin .= q[
+SELECT pg_backend_pid();
+];
+ok( TestLib::pump_until(
+		$killme, $psql_timeout, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m),
+	'acquired pid for SIGTERM');
+my $pid = $killme_stdout;
+chomp($pid);
+$killme_stdout = '';
+$killme_stderr = '';
+
+# Check the connections on 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');
+
+# Now drop database with dropdb --force command.
+$node->issues_sql_like(
+	[ 'dropdb', '--force', 'foobar1' ],
+	qr/statement: DROP DATABASE foobar1 WITH \(FORCE\);/,
+	'SQL DROP DATABASE (FORCE) run');
+
+# Check that psql sees the killed backend as having been terminated.
+$killme_stdin .= q[
+SELECT 1;
+];
+ok( TestLib::pump_until(
+		$killme, $psql_timeout, \$killme_stderr,
+		qr/FATAL:  

Re: dropdb --force

[Amit Kapila]

On Mon, Nov 25, 2019 at 11:22 PM vignesh C  wrote:
>
> On Sun, Nov 24, 2019 at 5:06 PM Pavel Stehule  wrote:
> >
> >
> >
> > ne 24. 11. 2019 v 11:25 odesílatel vignesh C  napsal:
> >>
> >> On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila  
> >> wrote:
> >> >
> >> > On Fri, Nov 22, 2019 at 3:16 PM vignesh C  wrote:
> >> > >
> >> > > 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.
> >> > >
> >> >
> >> > Few comments:
> >> > --
> >> > 1. There is a lot of duplicate code in this test.  Basically, almost
> >> > the same code is used once to test Drop Database command and dropdb.
> >> > I think we can create a few sub-functions to avoid duplicate code, but
> >> > do we really need to test the same thing once via command and then by
> >> > dropdb utility?   I don't think it is required, but even if we want to
> >> > do so, then I am not sure if this is the right test script.  I suggest
> >> > removing the command related test.
> >> >
> >>
> >> Pavel: What is your opinion on this?
> >
> >
> > I have not any problem with this reduction.
> >
> > We will see in future years what is benefit of this test.
> >
>
> Fixed, removed dropdb utility test.
>

Hmm, you have done the opposite of what I have asked.   This test file
is in rc/bin/scripts/t/  where we generally keep the tests for
utilities. So, retaining the dropdb utility test in that file seems
more sensible.

+ok( TestLib::pump_until(
+ $killme, $psql_timeout, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m),
+ 'acquired pid');

How about changing 'acquired pid' to 'acquired pid for SIGTERM'?

> >> >
> >>
> >> I have verified by running perltidy.
> >>

I think we don't need to run perltidy on the existing code.  So, I am
not sure if it is a good idea to run it for file 013_crash_restart.pl
as it changes some existing code formatting.

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

Re: dropdb --force

[vignesh C]

On Sun, Nov 24, 2019 at 5:06 PM Pavel Stehule  wrote:
>
>
>
> ne 24. 11. 2019 v 11:25 odesílatel vignesh C  napsal:
>>
>> On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila  wrote:
>> >
>> > On Fri, Nov 22, 2019 at 3:16 PM vignesh C  wrote:
>> > >
>> > > 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.
>> > >
>> >
>> > Few comments:
>> > --
>> > 1. There is a lot of duplicate code in this test.  Basically, almost
>> > the same code is used once to test Drop Database command and dropdb.
>> > I think we can create a few sub-functions to avoid duplicate code, but
>> > do we really need to test the same thing once via command and then by
>> > dropdb utility?   I don't think it is required, but even if we want to
>> > do so, then I am not sure if this is the right test script.  I suggest
>> > removing the command related test.
>> >
>>
>> Pavel: What is your opinion on this?
>
>
> I have not any problem with this reduction.
>
> We will see in future years what is benefit of this test.
>

Fixed, removed dropdb utility test.

>>
>> > 2.
>> > ok( TestLib::pump_until(
>> > + $killme,
>> > + $psql_timeout,
>> > + \$killme_stderr,
>> > + qr/FATAL:  terminating connection due to administrator command/m
>> > + ),
>> > + "psql query died successfully after SIGTERM");
>> >
>> > Extra space before TestLib.
>>
>> Ran perltidy, perltidy adds an extra space. I'm not sure which version
>> is right whether to include space or without space. I had noticed
>> similarly in 001_stream_rep.pl, in few places space is present and in
>> few places it is not present. If required I can update based on
>> suggestion.
>>
>> >
>> > 3.
>> > +=item pump_until(proc, psql_timeout, stream, untl)
>> >
>> > I think moving pump_until to TestLib is okay, but if you are making it
>> > a generic function to be used from multiple places, it is better to
>> > name the variable as 'timeout' instead of 'psql_timeout'
>> >
>>
>> Fixed.
>>
>> > 4. Have you ran perltidy, if not, can you please run it?  See
>> > src/test/perl/README for the recommendation.
>> >
>>
>> I have verified by running perltidy.
>>
>> Please find the updated patch attached. 1st patch is same as previous,
>> 2nd patch includes the fix for comments 2,3 & 4.
>>

Attached patch handles the fix for the above comments.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
From 0890c781b67595a51b04a1dfe972890fb6be18a4 Mon Sep 17 00:00:00 2001
From: vignesh 
Date: Mon, 25 Nov 2019 23:12:19 +0530
Subject: [PATCH 2/2] Made pump_until usable as a common subroutine.

Patch includes movement of pump_until subroutine from 013_crash_restart to
TestLib so that it can be used as a common sub from 013_crash_restart and
060_dropdb_force.
---
 src/test/perl/TestLib.pm | 37 +++
 src/test/recovery/t/013_crash_restart.pl | 62 
 2 files changed, 59 insertions(+), 40 deletions(-)

diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index a377cdb..b58679a 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -860,6 +860,43 @@ sub command_checks_all
 
 =pod
 
+=item pump_until(proc, timeout, stream, untl)
+
+# Pump until string is matched, or timeout occurs
+
+=cut
+
+sub pump_until
+{
+	my ($proc, $timeout, $stream, $untl) = @_;
+	$proc->pump_nb();
+	while (1)
+	{
+		last if $$stream =~ /$untl/;
+		if ($timeout->is_expired)
+		{
+			diag("aborting wait: program timed out");
+			diag("stream contents: >>", $$stream, "<<");
+			diag("pattern searched for: ", $untl);
+
+			return 0;
+		}
+		if (not $proc->pumpable())
+		{
+			diag("aborting wait: program died");
+			diag("stream contents: >>", $$stream, "<<");
+			diag("pattern searched for: ", $untl);
+
+			return 0;
+		}
+		$proc->pump();
+	}
+	return 1;
+
+}
+
+=pod
+
 =back
 
 =cut
diff --git a/src/test/recovery/t/013_crash_restart.pl b/src/test/recovery/t/013_crash_restart.pl
index 2c47797..dd08924 100644
--- a/src/test/recovery/t/013_crash_restart.pl
+++ b/src/test/recovery/t/013_crash_restart.pl
@@ -72,7 +72,8 @@ CREATE TABLE alive(status text);
 INSERT INTO alive VALUES($$committed-before-sigquit$$);
 SELECT pg_backend_pid();
 ];
-ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m),
+ok( TestLib::pump_until(
+		$killme, $psql_timeout, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m),
 	'acquired pid for SIGQUIT');
 my $pid = $killme_stdout;
 chomp($pid);
@@ -84,7 +85,9 @@ $killme_stdin .= q[
 BEGIN;
 INSERT INTO alive VALUES($$in-progress-before-sigquit$$) RETURNING status;
 ];
-ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigquit/m),
+ok( TestLib::pump_until(
+		$killme, $psql_timeout,
+		\$killme_stdout, qr/in-progress-before-sigquit/m),
 	'inserted in-progress-before-sigquit');
 $killme_stdout = '';
 $killme_stderr = '';
@@ -97,7 +100,8 @@ $monitor_stdin .= q[
 SELECT $$psql-connected$$;
 SELECT 

Re: dropdb --force

[Amit Kapila]

On Sun, Nov 24, 2019 at 3:55 PM vignesh C  wrote:
>
> On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila  wrote:
> >
>
> > 2.
> > ok( TestLib::pump_until(
> > + $killme,
> > + $psql_timeout,
> > + \$killme_stderr,
> > + qr/FATAL:  terminating connection due to administrator command/m
> > + ),
> > + "psql query died successfully after SIGTERM");
> >
> > Extra space before TestLib.
>
> Ran perltidy, perltidy adds an extra space. I'm not sure which version
> is right whether to include space or without space. I had noticed
> similarly in 001_stream_rep.pl, in few places space is present and in
> few places it is not present. If required I can update based on
> suggestion.
>

You can try by running perltidy on other existing .pl files where you
find the usage "without space" and see if it adds the extra space in
all places.  I think keeping the version after running perltidy would
be a better choice.

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

Re: dropdb --force

[Pavel Stehule]

ne 24. 11. 2019 v 11:25 odesílatel vignesh C  napsal:

> On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila 
> wrote:
> >
> > On Fri, Nov 22, 2019 at 3:16 PM vignesh C  wrote:
> > >
> > > 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.
> > >
> >
> > Few comments:
> > --
> > 1. There is a lot of duplicate code in this test.  Basically, almost
> > the same code is used once to test Drop Database command and dropdb.
> > I think we can create a few sub-functions to avoid duplicate code, but
> > do we really need to test the same thing once via command and then by
> > dropdb utility?   I don't think it is required, but even if we want to
> > do so, then I am not sure if this is the right test script.  I suggest
> > removing the command related test.
> >
>
> Pavel: What is your opinion on this?
>

I have not any problem with this reduction.

We will see in future years what is benefit of this test.


> > 2.
> > ok( TestLib::pump_until(
> > + $killme,
> > + $psql_timeout,
> > + \$killme_stderr,
> > + qr/FATAL:  terminating connection due to administrator command/m
> > + ),
> > + "psql query died successfully after SIGTERM");
> >
> > Extra space before TestLib.
>
> Ran perltidy, perltidy adds an extra space. I'm not sure which version
> is right whether to include space or without space. I had noticed
> similarly in 001_stream_rep.pl, in few places space is present and in
> few places it is not present. If required I can update based on
> suggestion.
>
> >
> > 3.
> > +=item pump_until(proc, psql_timeout, stream, untl)
> >
> > I think moving pump_until to TestLib is okay, but if you are making it
> > a generic function to be used from multiple places, it is better to
> > name the variable as 'timeout' instead of 'psql_timeout'
> >
>
> Fixed.
>
> > 4. Have you ran perltidy, if not, can you please run it?  See
> > src/test/perl/README for the recommendation.
> >
>
> I have verified by running perltidy.
>
> Please find the updated patch attached. 1st patch is same as previous,
> 2nd patch includes the fix for comments 2,3 & 4.
>
> Regards,
> Vignesh
> EnterpriseDB: http://www.enterprisedb.com
>

Re: dropdb --force

[vignesh C]

On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila  wrote:
>
> On Fri, Nov 22, 2019 at 3:16 PM vignesh C  wrote:
> >
> > 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.
> >
>
> Few comments:
> --
> 1. There is a lot of duplicate code in this test.  Basically, almost
> the same code is used once to test Drop Database command and dropdb.
> I think we can create a few sub-functions to avoid duplicate code, but
> do we really need to test the same thing once via command and then by
> dropdb utility?   I don't think it is required, but even if we want to
> do so, then I am not sure if this is the right test script.  I suggest
> removing the command related test.
>

Pavel: What is your opinion on this?

> 2.
> ok( TestLib::pump_until(
> + $killme,
> + $psql_timeout,
> + \$killme_stderr,
> + qr/FATAL:  terminating connection due to administrator command/m
> + ),
> + "psql query died successfully after SIGTERM");
>
> Extra space before TestLib.

Ran perltidy, perltidy adds an extra space. I'm not sure which version
is right whether to include space or without space. I had noticed
similarly in 001_stream_rep.pl, in few places space is present and in
few places it is not present. If required I can update based on
suggestion.

>
> 3.
> +=item pump_until(proc, psql_timeout, stream, untl)
>
> I think moving pump_until to TestLib is okay, but if you are making it
> a generic function to be used from multiple places, it is better to
> name the variable as 'timeout' instead of 'psql_timeout'
>

Fixed.

> 4. Have you ran perltidy, if not, can you please run it?  See
> src/test/perl/README for the recommendation.
>

I have verified by running perltidy.

Please find the updated patch attached. 1st patch is same as previous,
2nd patch includes the fix for comments 2,3 & 4.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
From dc8fc7432fab576845ad7f89e221c544926496f1 Mon Sep 17 00:00:00 2001
From: Pavel Stehule 
Date: Fri, 22 Nov 2019 14:38:23 +0530
Subject: [PATCH 1/2] Add tests for the support of '-f' option in dropdb
 utility.

Add tests for the support of '-f' option in dropdb utility and drop database
SQL.
---
 src/bin/scripts/t/060_dropdb_force.pl | 122 ++
 1 file changed, 122 insertions(+)
 create mode 100644 src/bin/scripts/t/060_dropdb_force.pl

diff --git a/src/bin/scripts/t/060_dropdb_force.pl b/src/bin/scripts/t/060_dropdb_force.pl
new file mode 100644
index 000..446dc09
--- /dev/null
+++ b/src/bin/scripts/t/060_dropdb_force.pl
@@ -0,0 +1,122 @@
+#
+# Tests drop database with force option.
+#
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 10;
+
+# To avoid hanging while expecting some specific input from a psql
+# instance being driven by us, add a timeout high enough that it
+# should never trigger even on very slow machines, unless something
+# is really wrong.
+my $psql_timeout = IPC::Run::timer(60);
+
+my $node = get_new_node('master');
+$node->init;
+$node->start;
+
+# Create database that will be dropped.
+$node->safe_psql('postgres', 'CREATE DATABASE foobar1');
+
+# Run psql, keeping session alive, so we have an alive backend to kill.
+my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+my $killme = IPC::Run::start(
+	[
+		'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d',
+		$node->connstr('foobar1')
+	],
+	'<',
+	\$killme_stdin,
+	'>',
+	\$killme_stdout,
+	'2>',
+	\$killme_stderr,
+	$psql_timeout);
+
+# Ensure killme process is active.
+$killme_stdin .= q[
+SELECT pg_backend_pid();
+];
+ok(TestLib::pump_until($killme, $psql_timeout, \$killme_stdout, 
+		qr/[[:digit:]]+[\r\n]$/m), 'acquired pid');
+my $pid = $killme_stdout;
+chomp($pid);
+$killme_stdout = '';
+$killme_stderr = '';
+
+# Check the connections on 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');
+
+$node->safe_psql('postgres', 'DROP DATABASE foobar1 WITH (FORCE)');
+
+# Check that psql sees the killed backend as having been terminated.
+$killme_stdin .= q[
+SELECT 1;
+];
+ok( TestLib::pump_until(
+		$killme,
+		$psql_timeout,
+		\$killme_stderr,
+		qr/FATAL:  terminating connection due to administrator command/m
+	),
+	"psql query died successfully after SIGTERM");
+$killme_stderr = '';
+$killme_stdout = '';
+$killme->finish;
+
+# Check database foobar1 does not exist.
+is($node->safe_psql('postgres', qq[SELECT EXISTS(SELECT * FROM pg_database WHERE datname='foobar1');]),
+	'f', 'database foobar1 was removed');
+
+# Create database that will be dropped.
+$node->safe_psql('postgres', 'CREATE DATABASE foobar1');
+
+# Restart psql as the previous connection will be
+# terminated by previous drop database.
+($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+$killme->run();
+
+# Acquire pid of 

Re: dropdb --force

[Amit Kapila]

On Fri, Nov 22, 2019 at 3:16 PM vignesh C  wrote:
>
> 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.
>

Few comments:
--
1. There is a lot of duplicate code in this test.  Basically, almost
the same code is used once to test Drop Database command and dropdb.
I think we can create a few sub-functions to avoid duplicate code, but
do we really need to test the same thing once via command and then by
dropdb utility?   I don't think it is required, but even if we want to
do so, then I am not sure if this is the right test script.  I suggest
removing the command related test.

2.
ok( TestLib::pump_until(
+ $killme,
+ $psql_timeout,
+ \$killme_stderr,
+ qr/FATAL:  terminating connection due to administrator command/m
+ ),
+ "psql query died successfully after SIGTERM");

Extra space before TestLib.

3.
+=item pump_until(proc, psql_timeout, stream, untl)

I think moving pump_until to TestLib is okay, but if you are making it
a generic function to be used from multiple places, it is better to
name the variable as 'timeout' instead of 'psql_timeout'

4. Have you ran perltidy, if not, can you please run it?  See
src/test/perl/README for the recommendation.

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

Re: dropdb --force

[Pavel Stehule]

pá 22. 11. 2019 v 10:46 odesílatel vignesh C  napsal:

> On Fri, Nov 22, 2019 at 12:10 AM Pavel Stehule 
> wrote:
> >
> >
> >
> > čt 21. 11. 2019 v 6:33 odesílatel vignesh C 
> napsal:
> >>
> >> On Mon, Nov 18, 2019 at 6:30 PM Pavel Stehule 
> 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.
>

thank you

looks well

Pavel


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

Re: dropdb --force

[vignesh C]

On Fri, Nov 22, 2019 at 12:10 AM Pavel Stehule  wrote:
>
>
>
> čt 21. 11. 2019 v 6:33 odesílatel vignesh C  napsal:
>>
>> On Mon, Nov 18, 2019 at 6:30 PM Pavel Stehule  
>> 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
From dc8fc7432fab576845ad7f89e221c544926496f1 Mon Sep 17 00:00:00 2001
From: Pavel Stehule 
Date: Fri, 22 Nov 2019 14:38:23 +0530
Subject: [PATCH 1/2] Add tests for the support of '-f' option in dropdb
 utility.

Add tests for the support of '-f' option in dropdb utility and drop database
SQL.
---
 src/bin/scripts/t/060_dropdb_force.pl | 122 ++
 1 file changed, 122 insertions(+)
 create mode 100644 src/bin/scripts/t/060_dropdb_force.pl

diff --git a/src/bin/scripts/t/060_dropdb_force.pl b/src/bin/scripts/t/060_dropdb_force.pl
new file mode 100644
index 000..446dc09
--- /dev/null
+++ b/src/bin/scripts/t/060_dropdb_force.pl
@@ -0,0 +1,122 @@
+#
+# Tests drop database with force option.
+#
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 10;
+
+# To avoid hanging while expecting some specific input from a psql
+# instance being driven by us, add a timeout high enough that it
+# should never trigger even on very slow machines, unless something
+# is really wrong.
+my $psql_timeout = IPC::Run::timer(60);
+
+my $node = get_new_node('master');
+$node->init;
+$node->start;
+
+# Create database that will be dropped.
+$node->safe_psql('postgres', 'CREATE DATABASE foobar1');
+
+# Run psql, keeping session alive, so we have an alive backend to kill.
+my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+my $killme = IPC::Run::start(
+	[
+		'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d',
+		$node->connstr('foobar1')
+	],
+	'<',
+	\$killme_stdin,
+	'>',
+	\$killme_stdout,
+	'2>',
+	\$killme_stderr,
+	$psql_timeout);
+
+# Ensure killme process is active.
+$killme_stdin .= q[
+SELECT pg_backend_pid();
+];
+ok(TestLib::pump_until($killme, $psql_timeout, \$killme_stdout, 
+		qr/[[:digit:]]+[\r\n]$/m), 'acquired pid');
+my $pid = $killme_stdout;
+chomp($pid);
+$killme_stdout = '';
+$killme_stderr = '';
+
+# Check the connections on 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');
+
+$node->safe_psql('postgres', 'DROP DATABASE foobar1 WITH (FORCE)');
+
+# Check that psql sees the killed backend as having been terminated.
+$killme_stdin .= q[
+SELECT 1;
+];
+ok( TestLib::pump_until(
+		$killme,
+		$psql_timeout,
+		\$killme_stderr,
+		qr/FATAL:  terminating connection due to administrator command/m
+	),
+	"psql query died successfully after SIGTERM");
+$killme_stderr = '';
+$killme_stdout = '';
+$killme->finish;
+
+# Check database foobar1 does not exist.
+is($node->safe_psql('postgres', qq[SELECT EXISTS(SELECT * FROM pg_database WHERE datname='foobar1');]),
+	'f', 'database foobar1 was removed');
+
+# Create database that will be dropped.
+$node->safe_psql('postgres', 'CREATE DATABASE foobar1');
+
+# Restart psql as the previous connection will be
+# terminated by previous drop database.
+($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+$killme->run();
+
+# Acquire pid of new backend.

Re: dropdb --force

[Pavel Stehule]

čt 21. 11. 2019 v 6:33 odesílatel vignesh C  napsal:

> On Mon, Nov 18, 2019 at 6:30 PM Pavel Stehule 
> 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
Description: Binary data

Re: dropdb --force

[vignesh C]

On Mon, Nov 18, 2019 at 6:30 PM Pavel Stehule  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

Re: dropdb --force

[Pavel Stehule]

st 20. 11. 2019 v 12:40 odesílatel Amit Kapila 
napsal:

> On Mon, Nov 18, 2019 at 10:59 AM Pavel Stehule 
> wrote:
> > po 18. 11. 2019 v 6:24 odesílatel Amit Kapila 
> 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
>

Re: dropdb --force

[Amit Kapila]

On Mon, Nov 18, 2019 at 10:59 AM Pavel Stehule  wrote:
> po 18. 11. 2019 v 6:24 odesílatel Amit Kapila  
> 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

Re: dropdb --force

[Pavel Stehule]

po 18. 11. 2019 v 7:39 odesílatel Pavel Stehule 
napsal:

>
>
> po 18. 11. 2019 v 7:37 odesílatel Amit Kapila 
> napsal:
>
>> On Mon, Nov 18, 2019 at 10:59 AM Pavel Stehule 
>> wrote:
>> > po 18. 11. 2019 v 6:24 odesílatel Amit Kapila 
>> napsal:
>> >>
>> >> On Mon, Nov 18, 2019 at 10:33 AM Pavel Stehule <
>> pavel.steh...@gmail.com> wrote:
>> >> >
>> >> > po 18. 11. 2019 v 4:43 odesílatel vignesh C 
>> 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
>>
>
diff --git a/src/bin/scripts/t/060_dropdb_force.pl b/src/bin/scripts/t/060_dropdb_force.pl
new file mode 100644
index 00..b895a7fbba
--- /dev/null
+++ b/src/bin/scripts/t/060_dropdb_force.pl
@@ -0,0 +1,149 @@
+#
+# Tests killing session connected to dropped database
+#
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 10;
+
+# To avoid hanging while expecting some specific input from a psql
+# instance being driven by us, add a timeout high enough that it
+# should never trigger even on very slow machines, unless something
+# is really wrong.
+my $psql_timeout = IPC::Run::timer(60);
+
+my $node = get_new_node('master');
+$node->init;
+$node->start;
+
+# Create database that will be dropped
+$node->safe_psql('postgres', 'CREATE DATABASE foobar1');
+
+# Run psql, keeping session alive, so we have an alive backend to kill.
+my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+my $killme = IPC::Run::start(
+	[
+		'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d',
+		$node->connstr('foobar1')
+	],
+	'<',
+	\$killme_stdin,
+	'>',
+	\$killme_stdout,
+	'2>',
+	\$killme_stderr,
+	$psql_timeout);
+
+#Ensure killme process is active
+$killme_stdin .= q[
+SELECT pg_backend_pid();
+];
+ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m),
+	'acquired pid');
+my $pid = $killme_stdout;
+chomp($pid);
+$killme_stdout = '';
+$killme_stderr = '';
+
+# 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');
+
+$node->safe_psql('postgres', 'DROP DATABASE foobar1 WITH (FORCE)');
+
+# Check that psql sees the killed backend as having been terminated
+$killme_stdin .= q[
+SELECT 1;
+];
+ok( pump_until(
+		$killme,
+		\$killme_stderr,
+		qr/FATAL:  terminating connection due to administrator command/m
+	),
+	"psql query died successfully after SIGTERM");
+$killme_stderr = '';
+$killme_stdout = '';
+$killme->finish;
+
+# 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');
+
+# Create database that will be dropped
+$node->safe_psql('postgres', 'CREATE DATABASE foobar1');
+
+# restart psql processes, now that the crash cycle finished
+($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+$killme->run();
+
+# Acquire pid of new backend
+$killme_stdin .= q[
+SELECT pg_backend_pid();
+];
+ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m),
+	"acquired pid for SIGKILL");
+$pid = $killme_stdout;
+chomp($pid);
+$killme_stdout = '';
+$killme_stderr = '';
+
+# 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');
+
+# Now drop database with dropdb --force command
+$node->issues_sql_like(
+	[ 'dropdb', '--force', 'foobar1' ],
+	qr/statement: DROP DATABASE foobar1 WITH \(FORCE\);/,
+	'SQL DROP DATABASE (FORCE) run');
+
+# Check that psql sees the killed backend as having been terminated
+$killme_stdin .= q[
+SELECT 1;
+];
+ok( pump_until(
+		$killme,
+		\$killme_stderr,
+		qr/FATAL:  terminating connection due to administrator command/m
+	),
+	"psql query died successfully after SIGTERM");
+$killme_stderr = '';
+$killme_stdout = '';
+$killme->finish;
+
+# and 

Re: dropdb --force

[Pavel Stehule]

po 18. 11. 2019 v 7:37 odesílatel Amit Kapila 
napsal:

> On Mon, Nov 18, 2019 at 10:59 AM Pavel Stehule 
> wrote:
> > po 18. 11. 2019 v 6:24 odesílatel Amit Kapila 
> napsal:
> >>
> >> On Mon, Nov 18, 2019 at 10:33 AM Pavel Stehule 
> wrote:
> >> >
> >> > po 18. 11. 2019 v 4:43 odesílatel vignesh C 
> 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
>

Re: dropdb --force

[Amit Kapila]

On Mon, Nov 18, 2019 at 10:59 AM Pavel Stehule  wrote:
> po 18. 11. 2019 v 6:24 odesílatel Amit Kapila  
> napsal:
>>
>> On Mon, Nov 18, 2019 at 10:33 AM Pavel Stehule  
>> wrote:
>> >
>> > po 18. 11. 2019 v 4:43 odesílatel vignesh C  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

Re: dropdb --force

[Pavel Stehule]

po 18. 11. 2019 v 6:24 odesílatel Amit Kapila 
napsal:

> On Mon, Nov 18, 2019 at 10:33 AM Pavel Stehule 
> wrote:
> >
> > po 18. 11. 2019 v 4:43 odesílatel vignesh C 
> 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
>

Re: dropdb --force

[Amit Kapila]

On Mon, Nov 18, 2019 at 10:33 AM Pavel Stehule  wrote:
>
> po 18. 11. 2019 v 4:43 odesílatel vignesh C  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
Description: Binary data

Re: dropdb --force

[Pavel Stehule]

po 18. 11. 2019 v 4:43 odesílatel vignesh C  napsal:

> On Sat, Nov 16, 2019 at 1:25 PM Pavel Stehule 
> 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, --echoshow 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
>

Re: dropdb --force

[vignesh C]

On Sat, Nov 16, 2019 at 1:25 PM Pavel Stehule  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, --echoshow 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

Re: dropdb --force

[Pavel Stehule]

so 16. 11. 2019 v 1:10 odesílatel vignesh C  napsal:

> On Fri, Nov 15, 2019 at 1:23 PM Pavel Stehule 
> 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, --echoshow 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
>
diff --git a/doc/src/sgml/ref/dropdb.sgml b/doc/src/sgml/ref/dropdb.sgml
index 3fbdb33716..63b90005b4 100644
--- a/doc/src/sgml/ref/dropdb.sgml
+++ b/doc/src/sgml/ref/dropdb.sgml
@@ -86,6 +86,27 @@ PostgreSQL documentation
   
  
 
+ 
+  -f
+  --force
+  
+   
+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.
+   
+   
+This will fail if the current user has no permissions
+to terminate other connections. Required permissions
+are the same as with pg_terminate_backend,
+described in .
+This will also fail if we are not able to terminate
+connections.
+   
+  
+ 
+
  
-V
--version
diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
index dacd8e5f1d..8aab5f1eaf 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, _exists, 1},
 		{"maintenance-db", required_argument, NULL, 2},
+		{"force", no_argument, NULL, 'f'},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -49,6 +50,7 @@ main(int argc, char *argv[])
 	enum trivalue prompt_password = TRI_DEFAULT;
 	bool		echo = false;
 	bool		interactive = false;
+	bool		force = false;
 
 	PQExpBufferData sql;
 
@@ -61,7 +63,7 @@ main(int argc, char *argv[])
 
 	handle_help_version_opts(argc, argv, "dropdb", help);
 
-	while ((c = getopt_long(argc, argv, "h:p:U:wWei", long_options, )) != -1)
+	while ((c = getopt_long(argc, argv, "h:p:U:wWeif", long_options, )) != -1)
 	{
 		switch (c)
 		{
@@ -86,6 +88,9 @@ main(int argc, char *argv[])
 			case 'i':
 interactive = true;
 break;
+			case 'f':
+force = true;
+break;
 			case 0:
 /* this covers the long options */
 break;
@@ -123,8 +128,11 @@ main(int argc, char *argv[])
 
 	initPQExpBuffer();
 
-	appendPQExpBuffer(, "DROP DATABASE %s%s;",
-	  (if_exists ? "IF EXISTS " : ""), fmtId(dbname));
+	/* Currently, only FORCE option is supported */
+	appendPQExpBuffer(, "DROP DATABASE %s%s%s;",
+	  (if_exists ? "IF EXISTS " : ""),
+	  fmtId(dbname),
+	  force ? " WITH (FORCE)" : "");
 
 	/* Avoid trying to drop postgres db while we are connected to it. */
 	if (maintenance_db == NULL && strcmp(dbname, "postgres") == 0)
@@ -159,6 +167,7 @@ help(const char *progname)
 	printf(_("\nOptions:\n"));
 	printf(_("  -e, --echoshow the commands being sent to the server\n"));
 	printf(_("  -i, --interactive prompt before deleting anything\n"));
+	printf(_("  -f, --force   try to terminate other connections before dropping\n"));
 	printf(_("  -V, --version output version information, then exit\n"));
 	printf(_("  --if-exists   don't report error if database doesn't exist\n"));
 	printf(_("  -?, --helpshow this help, then exit\n"));
diff --git a/src/bin/scripts/t/050_dropdb.pl b/src/bin/scripts/t/050_dropdb.pl
index 25aa54a4ae..c51babe093 100644
--- a/src/bin/scripts/t/050_dropdb.pl
+++ b/src/bin/scripts/t/050_dropdb.pl
@@ -3,7 

Re: dropdb --force

[vignesh C]

On Fri, Nov 15, 2019 at 1:23 PM Pavel Stehule  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, --echoshow 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

Re: dropdb --force

[Pavel Stehule]

čt 14. 11. 2019 v 12:12 odesílatel vignesh C  napsal:

> On Wed, Nov 13, 2019 at 8:05 PM Pavel Stehule 
> wrote:
> >
> >
> >
> > st 13. 11. 2019 v 7:13 odesílatel Pavel Stehule 
> napsal:
> >>
> >>
> >>
> >> st 13. 11. 2019 v 7:12 odesílatel Amit Kapila 
> napsal:
> >>>
> >>> On Tue, Nov 12, 2019 at 11:17 AM Amit Kapila 
> 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.
> +   
> +   
> +This will add the FORCE option to the
> DROP
> +DATABASE command sent to the server.
> +   
>
> The above documentation can be changed similar to drop_database.sgml:
>  
>   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.
>  
>  
>   This will fail if the current user has no permissions to terminate
> other
>   connections. Required permissions are the same as with
>   pg_terminate_backend, described in
>   .  This will also fail if we
>   are not able to terminate connections.
>  
>
>
done


> We can make the modification in the same location as earlier in the below
> case:
> -appendPQExpBuffer(, "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(, "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(, "DROP DATABASE %s%s%s;",
> can be changed to:
> +/* Generate drop db command using the options specified */
> +appendPQExpBuffer(, "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
>
diff --git a/doc/src/sgml/ref/dropdb.sgml b/doc/src/sgml/ref/dropdb.sgml
index 3fbdb33716..63b90005b4 100644
--- a/doc/src/sgml/ref/dropdb.sgml
+++ b/doc/src/sgml/ref/dropdb.sgml
@@ -86,6 +86,27 @@ PostgreSQL documentation
   
  
 
+ 
+  -f
+  --force
+  
+   
+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.
+   
+   
+This will fail if the current user has no permissions
+to terminate other connections. Required permissions
+are the same as with pg_terminate_backend,
+described in .
+This will also fail if we are not able to terminate
+connections.
+   
+  
+ 
+
  
-V
--version
diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
index dacd8e5f1d..ea7e5d8f75 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, _exists, 1},
 		{"maintenance-db", required_argument, NULL, 2},
+		{"force", no_argument, NULL, 'f'},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -49,6 +50,7 @@ main(int argc, char *argv[])
 	enum trivalue prompt_password = TRI_DEFAULT;
 	bool		echo = false;
 	bool		interactive = false;
+	bool		force = 

Re: dropdb --force

[vignesh C]

On Wed, Nov 13, 2019 at 8:05 PM Pavel Stehule  wrote:
>
>
>
> st 13. 11. 2019 v 7:13 odesílatel Pavel Stehule  
> napsal:
>>
>>
>>
>> st 13. 11. 2019 v 7:12 odesílatel Amit Kapila  
>> napsal:
>>>
>>> On Tue, Nov 12, 2019 at 11:17 AM Amit Kapila  
>>> 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.
+   
+   
+This will add the FORCE option to the DROP
+DATABASE command sent to the server.
+   

The above documentation can be changed similar to drop_database.sgml:
 
  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.
 
 
  This will fail if the current user has no permissions to terminate other
  connections. Required permissions are the same as with
  pg_terminate_backend, described in
  .  This will also fail if we
  are not able to terminate connections.
 

We can make the modification in the same location as earlier in the below case:
-appendPQExpBuffer(, "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(, "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(, "DROP DATABASE %s%s%s;",
can be changed to:
+/* Generate drop db command using the options specified */
+appendPQExpBuffer(, "DROP DATABASE %s%s%s;",

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

Re: dropdb --force

[Pavel Stehule]

čt 14. 11. 2019 v 7:44 odesílatel Amit Kapila 
napsal:

> On Thu, Nov 14, 2019 at 11:43 AM Pavel Stehule 
> wrote:
> >
> > st 13. 11. 2019 v 15:34 odesílatel Pavel Stehule <
> pavel.steh...@gmail.com> 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
>

Re: dropdb --force

[Amit Kapila]

On Thu, Nov 14, 2019 at 12:14 PM Amit Kapila  wrote:
>
> On Thu, Nov 14, 2019 at 11:43 AM Pavel Stehule  
> wrote:
> >
> > st 13. 11. 2019 v 15:34 odesílatel Pavel Stehule  
> > 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

Re: dropdb --force

[Amit Kapila]

On Thu, Nov 14, 2019 at 11:43 AM Pavel Stehule  wrote:
>
> st 13. 11. 2019 v 15:34 odesílatel Pavel Stehule  
> 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

Re: dropdb --force

[Pavel Stehule]

st 13. 11. 2019 v 15:34 odesílatel Pavel Stehule 
napsal:

>
>
> st 13. 11. 2019 v 7:13 odesílatel Pavel Stehule 
> napsal:
>
>>
>>
>> st 13. 11. 2019 v 7:12 odesílatel Amit Kapila 
>> napsal:
>>
>>> On Tue, Nov 12, 2019 at 11:17 AM Amit Kapila 
>>> 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
>>>
>>

Re: dropdb --force

[Pavel Stehule]

st 13. 11. 2019 v 7:13 odesílatel Pavel Stehule 
napsal:

>
>
> st 13. 11. 2019 v 7:12 odesílatel Amit Kapila 
> napsal:
>
>> On Tue, Nov 12, 2019 at 11:17 AM Amit Kapila 
>> 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

Pavel



> Pavel
>
>
>> --
>> With Regards,
>> Amit Kapila.
>> EnterpriseDB: http://www.enterprisedb.com
>>
>
diff --git a/doc/src/sgml/ref/dropdb.sgml b/doc/src/sgml/ref/dropdb.sgml
index 3fbdb33716..5d10ad1c92 100644
--- a/doc/src/sgml/ref/dropdb.sgml
+++ b/doc/src/sgml/ref/dropdb.sgml
@@ -86,6 +86,20 @@ PostgreSQL documentation
   
  
 
+ 
+  -f
+  --force
+  
+   
+Force termination of connected backends before removing the database.
+   
+   
+This will add the FORCE option to the DROP
+DATABASE command sent to the server.
+   
+  
+ 
+
  
-V
--version
diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
index dacd8e5f1d..3a6aac8ac3 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, _exists, 1},
 		{"maintenance-db", required_argument, NULL, 2},
+		{"force", no_argument, NULL, 'f'},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -49,6 +50,7 @@ main(int argc, char *argv[])
 	enum trivalue prompt_password = TRI_DEFAULT;
 	bool		echo = false;
 	bool		interactive = false;
+	bool		force = false;
 
 	PQExpBufferData sql;
 
@@ -61,7 +63,7 @@ main(int argc, char *argv[])
 
 	handle_help_version_opts(argc, argv, "dropdb", help);
 
-	while ((c = getopt_long(argc, argv, "h:p:U:wWei", long_options, )) != -1)
+	while ((c = getopt_long(argc, argv, "h:p:U:wWeif", long_options, )) != -1)
 	{
 		switch (c)
 		{
@@ -86,6 +88,9 @@ main(int argc, char *argv[])
 			case 'i':
 interactive = true;
 break;
+			case 'f':
+force = true;
+break;
 			case 0:
 /* this covers the long options */
 break;
@@ -123,9 +128,6 @@ main(int argc, char *argv[])
 
 	initPQExpBuffer();
 
-	appendPQExpBuffer(, "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(, "DROP DATABASE %s%s%s;",
+	  (if_exists ? "IF EXISTS " : ""),
+	  fmtId(dbname),
+	  force ? " WITH (FORCE)" : "");
+
 	if (echo)
 		printf("%s\n", sql.data);
 	result = PQexec(conn, sql.data);
@@ -159,6 +167,7 @@ help(const char *progname)
 	printf(_("\nOptions:\n"));
 	printf(_("  -e, --echoshow the commands being sent to the server\n"));
 	printf(_("  -i, --interactive prompt before deleting anything\n"));
+	printf(_("  -f, --force   force termination of connected backends\n"));
 	printf(_("  -V, --version output version information, then exit\n"));
 	printf(_("  --if-exists   don't report error if database doesn't exist\n"));
 	printf(_("  -?, --helpshow this help, then exit\n"));
diff --git a/src/bin/scripts/t/050_dropdb.pl b/src/bin/scripts/t/050_dropdb.pl
index 25aa54a4ae..c51babe093 100644
--- a/src/bin/scripts/t/050_dropdb.pl
+++ b/src/bin/scripts/t/050_dropdb.pl
@@ -3,7 +3,7 @@ use warnings;
 
 use PostgresNode;
 use TestLib;
-use Test::More tests => 11;
+use Test::More tests => 13;
 
 program_help_ok('dropdb');
 program_version_ok('dropdb');
@@ -19,5 +19,11 @@ $node->issues_sql_like(
 	qr/statement: DROP DATABASE foobar1/,
 	'SQL DROP DATABASE run');
 
+$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');
+
 $node->command_fails([ 'dropdb', 'nonexistent' ],
 	'fails with nonexistent database');

Re: dropdb --force

[Pavel Stehule]

st 13. 11. 2019 v 7:12 odesílatel Amit Kapila 
napsal:

> On Tue, Nov 12, 2019 at 11:17 AM Amit Kapila 
> 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.

Pavel


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

Re: dropdb --force

[Amit Kapila]

On Tue, Nov 12, 2019 at 11:17 AM Amit Kapila  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.

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

Re: dropdb --force

[Amit Kapila]

On Mon, Nov 11, 2019 at 1:57 PM Amit Kapila  wrote:
>
> This patch adds a few test cases to test the syntax in
> /src/test/regress/sql/drop_if_exists.sql which I think is not the best
> place to add the tests for this feature as it is primarily testing ..
> IF EXISTS .. syntax.  OTOH, I am not sure if there is any other better
> place to add those.  The other option could be to add a new test file,
> but again I am not sure if it is worth to do so for a few tests.  We
> can even decide not to have tests for this feature as the tests are
> just testing the syntax which I think can help if we want to extend
> the syntax in the future.
>

Today, I again looked at some of the other drop command syntaxes and
it seems some of them like 'Drop Extension ..' are also tested via
this file.  So, I decided to keep the test in this file but changed
the name of the database used in the test to match with other tests in
that file and modified comments.

I am planning to commit this patch tomorrow unless I see more comments
or interest from someone else to review this.

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


drop-database-force-20191112.amit.patch
Description: Binary data

Re: dropdb --force

[Amit Kapila]

On Mon, Nov 11, 2019 at 8:45 AM Amit Kapila  wrote:
>
> On Fri, Nov 8, 2019 at 4:57 PM Pavel Stehule  wrote:
> >
> > I check it now - it has sense. the described switch can protect users 
> > against useless client killing.
> >
>
> I have committed that patch.  Please find the rebased patch attached.
> Additionally, I ran the pgindent.  I will wait for two days and see if
> you or anyone else has more inputs on the patch, if not, then I will
> take one more pass and commit it on Wednesday morning.
>

This patch adds a few test cases to test the syntax in
/src/test/regress/sql/drop_if_exists.sql which I think is not the best
place to add the tests for this feature as it is primarily testing ..
IF EXISTS .. syntax.  OTOH, I am not sure if there is any other better
place to add those.  The other option could be to add a new test file,
but again I am not sure if it is worth to do so for a few tests.  We
can even decide not to have tests for this feature as the tests are
just testing the syntax which I think can help if we want to extend
the syntax in the future.

Any opinions?


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

Re: dropdb --force

[Amit Kapila]

On Fri, Nov 8, 2019 at 4:57 PM Pavel Stehule  wrote:
>
> pá 8. 11. 2019 v 11:50 odesílatel Amit Kapila  
> napsal:
>>
>> Thanks, but are you planning to look at the other thread mentioned in
>> my previous email?  We need to finish that before this.
>
>
> I check it now - it has sense. the described switch can protect users against 
> useless client killing.
>

I have committed that patch.  Please find the rebased patch attached.
Additionally, I ran the pgindent.  I will wait for two days and see if
you or anyone else has more inputs on the patch, if not, then I will
take one more pass and commit it on Wednesday morning.

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


drop-database-force-2019.amit.patch
Description: Binary data

Re: dropdb --force

[Pavel Stehule]

pá 8. 11. 2019 v 11:50 odesílatel Amit Kapila 
napsal:

> On Fri, Nov 8, 2019 at 4:13 PM Pavel Stehule 
> wrote:
> >
> >>> Did you get a chance to look at the other related patch posted by me
> >>> [1]?  I have asked it before as well because I think that need to go
> >>> before this.  We need to avoid errors to happen after terminating the
> >>> connections as otherwise, the termination of other databases will go
> >>> be of no use.
> >>>
> >>> I have added the comment and changed one condition in tab-completion
> >>> as otherwise, it was allowing tab completion for the following syntax:
> >>> "drop database db1 , FORCE" which doesn't make sense to me.  Please,
> >>> find the result attached.  Let me know what you think?
> >>>
> >>> [1] -
> https://www.postgresql.org/message-id/CAA4eK1%2BqhLkCYG2oy9xug9ur_j%3DG2wQNRYAyd%2B-kZfZ1z42pLw%40mail.gmail.com
> >>
> >>
> >> I'll check it today
> >
> >
> > I checked it for 800 active clients, and it is working without problems.
> I run "check-world" without problem too.
> >
> > The patch looks well, I have not any comments.
> >
>
> Thanks, but are you planning to look at the other thread mentioned in
> my previous email?  We need to finish that before this.
>

I check it now - it has sense. the described switch can protect users
against useless client killing.

The practical benefit will not be high, but it has sense and the code will
more logic.

Regards

Pavel

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

Re: dropdb --force

[Amit Kapila]

On Fri, Nov 8, 2019 at 4:13 PM Pavel Stehule  wrote:
>
>>> Did you get a chance to look at the other related patch posted by me
>>> [1]?  I have asked it before as well because I think that need to go
>>> before this.  We need to avoid errors to happen after terminating the
>>> connections as otherwise, the termination of other databases will go
>>> be of no use.
>>>
>>> I have added the comment and changed one condition in tab-completion
>>> as otherwise, it was allowing tab completion for the following syntax:
>>> "drop database db1 , FORCE" which doesn't make sense to me.  Please,
>>> find the result attached.  Let me know what you think?
>>>
>>> [1] - 
>>> https://www.postgresql.org/message-id/CAA4eK1%2BqhLkCYG2oy9xug9ur_j%3DG2wQNRYAyd%2B-kZfZ1z42pLw%40mail.gmail.com
>>
>>
>> I'll check it today
>
>
> I checked it for 800 active clients, and it is working without problems. I 
> run "check-world" without problem too.
>
> The patch looks well, I have not any comments.
>

Thanks, but are you planning to look at the other thread mentioned in
my previous email?  We need to finish that before this.



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

Re: dropdb --force

[Pavel Stehule]

pá 8. 11. 2019 v 6:40 odesílatel Pavel Stehule 
napsal:

>
>
> pá 8. 11. 2019 v 6:39 odesílatel Amit Kapila 
> napsal:
>
>> On Thu, Nov 7, 2019 at 11:29 AM Pavel Stehule 
>> wrote:
>> > čt 7. 11. 2019 v 6:56 odesílatel Amit Kapila 
>> napsal:
>> >>
>> >> Okay, no problem.  I will pick the previous version and do this.  I
>> >> will post the patch in a day or so for your review.
>> >
>> >
>> > Thank you very much
>> >
>>
>> Did you get a chance to look at the other related patch posted by me
>> [1]?  I have asked it before as well because I think that need to go
>> before this.  We need to avoid errors to happen after terminating the
>> connections as otherwise, the termination of other databases will go
>> be of no use.
>>
>> I have added the comment and changed one condition in tab-completion
>> as otherwise, it was allowing tab completion for the following syntax:
>> "drop database db1 , FORCE" which doesn't make sense to me.  Please,
>> find the result attached.  Let me know what you think?
>>
>> [1] -
>> https://www.postgresql.org/message-id/CAA4eK1%2BqhLkCYG2oy9xug9ur_j%3DG2wQNRYAyd%2B-kZfZ1z42pLw%40mail.gmail.com
>
>
> I'll check it today
>

I checked it for 800 active clients, and it is working without problems. I
run "check-world" without problem too.

The patch looks well, I have not any comments.

Regards

Pavel


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

Re: dropdb --force

[Pavel Stehule]

pá 8. 11. 2019 v 6:39 odesílatel Amit Kapila 
napsal:

> On Thu, Nov 7, 2019 at 11:29 AM Pavel Stehule 
> wrote:
> > čt 7. 11. 2019 v 6:56 odesílatel Amit Kapila 
> napsal:
> >>
> >> Okay, no problem.  I will pick the previous version and do this.  I
> >> will post the patch in a day or so for your review.
> >
> >
> > Thank you very much
> >
>
> Did you get a chance to look at the other related patch posted by me
> [1]?  I have asked it before as well because I think that need to go
> before this.  We need to avoid errors to happen after terminating the
> connections as otherwise, the termination of other databases will go
> be of no use.
>
> I have added the comment and changed one condition in tab-completion
> as otherwise, it was allowing tab completion for the following syntax:
> "drop database db1 , FORCE" which doesn't make sense to me.  Please,
> find the result attached.  Let me know what you think?
>
> [1] -
> https://www.postgresql.org/message-id/CAA4eK1%2BqhLkCYG2oy9xug9ur_j%3DG2wQNRYAyd%2B-kZfZ1z42pLw%40mail.gmail.com


I'll check it today

Pavel

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

Re: dropdb --force

[Amit Kapila]

On Thu, Nov 7, 2019 at 11:29 AM Pavel Stehule  wrote:
> čt 7. 11. 2019 v 6:56 odesílatel Amit Kapila  napsal:
>>
>> Okay, no problem.  I will pick the previous version and do this.  I
>> will post the patch in a day or so for your review.
>
>
> Thank you very much
>

Did you get a chance to look at the other related patch posted by me
[1]?  I have asked it before as well because I think that need to go
before this.  We need to avoid errors to happen after terminating the
connections as otherwise, the termination of other databases will go
be of no use.

I have added the comment and changed one condition in tab-completion
as otherwise, it was allowing tab completion for the following syntax:
"drop database db1 , FORCE" which doesn't make sense to me.  Please,
find the result attached.  Let me know what you think?

[1] - 
https://www.postgresql.org/message-id/CAA4eK1%2BqhLkCYG2oy9xug9ur_j%3DG2wQNRYAyd%2B-kZfZ1z42pLw%40mail.gmail.com

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


drop-database-force-20191108.amit.patch
Description: Binary data

Re: dropdb --force

[Pavel Stehule]

čt 7. 11. 2019 v 6:56 odesílatel Amit Kapila 
napsal:

> On Thu, Nov 7, 2019 at 10:46 AM Pavel Stehule 
> wrote:
> >
> > čt 7. 11. 2019 v 3:42 odesílatel Amit Kapila 
> napsal:
> >>
> >> On Wed, Nov 6, 2019 at 11:46 PM Pavel Stehule 
> wrote:
> >> >
> >> > st 6. 11. 2019 v 14:59 odesílatel Tom Lane 
> napsal:
> >> >>
> >> >> Amit Kapila  writes:
> >> >> > I think there is still a window where the same problem can happen,
> say
> >> >> > the signal has been sent by SendProcSignal to the required process
> and
> >> >> > it releases the ProcArrayLock.  Now, the target process exits and a
> >> >> > new process gets the same pid before the signal is received.
> >> >>
> >> >> In principle, no use of Unix signals is ever safe against this sort
> >> >> of race condition --- process A can never know that process B didn't
> >> >> exit immediately before A does kill(B, n).  In practice, it's okay
> >> >> because the kernel is expected not to reassign a dead PID for some
> >> >> reasonable grace period [1].  I'd be inclined to lean more heavily
> >> >> on that expectation than anything internal to Postgres.  That is,
> >> >> remembering the PID we want to kill for some small number of
> >> >> microseconds is probably a safer API than anything that depends on
> >> >> the contents of the ProcArray, because there indeed *isn't* any
> >> >> guarantee that a ProcArray entry won't be recycled immediately.
> >> >>
> >>
> >> Right, this makes sense.  I think I was overly paranoid about this
> >> behavior even though that was used at a few other places as this patch
> >> might need to rely on many pids not being reused after the lock is
> >> released.
> >>
> >> >
> >> >
> >> > so we can return back to just simple killing.
> >> >
> >>
> >> I think so.  I think we might want to add a comment about this race
> >> condition and add or reference to comments in pg_signal_backend which
> >> mentions the same race condition.
> >
> >
> > Please, can you do it. It's bad task for my with my bad English.
> >
>
> Okay, no problem.  I will pick the previous version and do this.  I
> will post the patch in a day or so for your review.
>

Thank you very much

Pavel

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

Re: dropdb --force

[Amit Kapila]

On Thu, Nov 7, 2019 at 10:46 AM Pavel Stehule  wrote:
>
> čt 7. 11. 2019 v 3:42 odesílatel Amit Kapila  napsal:
>>
>> On Wed, Nov 6, 2019 at 11:46 PM Pavel Stehule  
>> wrote:
>> >
>> > st 6. 11. 2019 v 14:59 odesílatel Tom Lane  napsal:
>> >>
>> >> Amit Kapila  writes:
>> >> > I think there is still a window where the same problem can happen, say
>> >> > the signal has been sent by SendProcSignal to the required process and
>> >> > it releases the ProcArrayLock.  Now, the target process exits and a
>> >> > new process gets the same pid before the signal is received.
>> >>
>> >> In principle, no use of Unix signals is ever safe against this sort
>> >> of race condition --- process A can never know that process B didn't
>> >> exit immediately before A does kill(B, n).  In practice, it's okay
>> >> because the kernel is expected not to reassign a dead PID for some
>> >> reasonable grace period [1].  I'd be inclined to lean more heavily
>> >> on that expectation than anything internal to Postgres.  That is,
>> >> remembering the PID we want to kill for some small number of
>> >> microseconds is probably a safer API than anything that depends on
>> >> the contents of the ProcArray, because there indeed *isn't* any
>> >> guarantee that a ProcArray entry won't be recycled immediately.
>> >>
>>
>> Right, this makes sense.  I think I was overly paranoid about this
>> behavior even though that was used at a few other places as this patch
>> might need to rely on many pids not being reused after the lock is
>> released.
>>
>> >
>> >
>> > so we can return back to just simple killing.
>> >
>>
>> I think so.  I think we might want to add a comment about this race
>> condition and add or reference to comments in pg_signal_backend which
>> mentions the same race condition.
>
>
> Please, can you do it. It's bad task for my with my bad English.
>

Okay, no problem.  I will pick the previous version and do this.  I
will post the patch in a day or so for your review.

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

Re: dropdb --force

[Pavel Stehule]

čt 7. 11. 2019 v 3:42 odesílatel Amit Kapila 
napsal:

> On Wed, Nov 6, 2019 at 11:46 PM Pavel Stehule 
> wrote:
> >
> > st 6. 11. 2019 v 14:59 odesílatel Tom Lane  napsal:
> >>
> >> Amit Kapila  writes:
> >> > I think there is still a window where the same problem can happen, say
> >> > the signal has been sent by SendProcSignal to the required process and
> >> > it releases the ProcArrayLock.  Now, the target process exits and a
> >> > new process gets the same pid before the signal is received.
> >>
> >> In principle, no use of Unix signals is ever safe against this sort
> >> of race condition --- process A can never know that process B didn't
> >> exit immediately before A does kill(B, n).  In practice, it's okay
> >> because the kernel is expected not to reassign a dead PID for some
> >> reasonable grace period [1].  I'd be inclined to lean more heavily
> >> on that expectation than anything internal to Postgres.  That is,
> >> remembering the PID we want to kill for some small number of
> >> microseconds is probably a safer API than anything that depends on
> >> the contents of the ProcArray, because there indeed *isn't* any
> >> guarantee that a ProcArray entry won't be recycled immediately.
> >>
>
> Right, this makes sense.  I think I was overly paranoid about this
> behavior even though that was used at a few other places as this patch
> might need to rely on many pids not being reused after the lock is
> released.
>
> >
> >
> > so we can return back to just simple killing.
> >
>
> I think so.  I think we might want to add a comment about this race
> condition and add or reference to comments in pg_signal_backend which
> mentions the same race condition.
>

Please, can you do it. It's bad task for my with my bad English.

Thank you

Pavel


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

Re: dropdb --force

[Amit Kapila]

On Wed, Nov 6, 2019 at 11:46 PM Pavel Stehule  wrote:
>
> st 6. 11. 2019 v 14:59 odesílatel Tom Lane  napsal:
>>
>> Amit Kapila  writes:
>> > I think there is still a window where the same problem can happen, say
>> > the signal has been sent by SendProcSignal to the required process and
>> > it releases the ProcArrayLock.  Now, the target process exits and a
>> > new process gets the same pid before the signal is received.
>>
>> In principle, no use of Unix signals is ever safe against this sort
>> of race condition --- process A can never know that process B didn't
>> exit immediately before A does kill(B, n).  In practice, it's okay
>> because the kernel is expected not to reassign a dead PID for some
>> reasonable grace period [1].  I'd be inclined to lean more heavily
>> on that expectation than anything internal to Postgres.  That is,
>> remembering the PID we want to kill for some small number of
>> microseconds is probably a safer API than anything that depends on
>> the contents of the ProcArray, because there indeed *isn't* any
>> guarantee that a ProcArray entry won't be recycled immediately.
>>

Right, this makes sense.  I think I was overly paranoid about this
behavior even though that was used at a few other places as this patch
might need to rely on many pids not being reused after the lock is
released.

>
>
> so we can return back to just simple killing.
>

I think so.  I think we might want to add a comment about this race
condition and add or reference to comments in pg_signal_backend which
mentions the same race condition.


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

Re: dropdb --force

[Pavel Stehule]

st 6. 11. 2019 v 14:59 odesílatel Tom Lane  napsal:

> Amit Kapila  writes:
> > I think there is still a window where the same problem can happen, say
> > the signal has been sent by SendProcSignal to the required process and
> > it releases the ProcArrayLock.  Now, the target process exits and a
> > new process gets the same pid before the signal is received.
>
> In principle, no use of Unix signals is ever safe against this sort
> of race condition --- process A can never know that process B didn't
> exit immediately before A does kill(B, n).  In practice, it's okay
> because the kernel is expected not to reassign a dead PID for some
> reasonable grace period [1].  I'd be inclined to lean more heavily
> on that expectation than anything internal to Postgres.  That is,
> remembering the PID we want to kill for some small number of
> microseconds is probably a safer API than anything that depends on
> the contents of the ProcArray, because there indeed *isn't* any
> guarantee that a ProcArray entry won't be recycled immediately.
>
> regards, tom lane
>
> [1] and also because the kernel *can't* recycle the PID until the
> parent process has reaped the zombie process-table entry.  Thus,
> for example, it's unconditionally safe for the postmaster to signal
> its children, because those PIDs can't move until the postmaster
> accepts the SIGCHLD signal and does a wait() for them.  Any
> interprocess signals between child processes are inherently a tad
> less safe.  But we've gotten away with interprocess SIGUSR1 for
> decades with no reported problems.  I don't really think that we
> need to move the goalposts for SIGINT, and I'm entirely not in
> favor of the sorts of complications that are being proposed here.
>

so we can return back to just simple killing.

Regards

Pavel

Re: dropdb --force

[Tom Lane]

Amit Kapila  writes:
> I think there is still a window where the same problem can happen, say
> the signal has been sent by SendProcSignal to the required process and
> it releases the ProcArrayLock.  Now, the target process exits and a
> new process gets the same pid before the signal is received.

In principle, no use of Unix signals is ever safe against this sort
of race condition --- process A can never know that process B didn't
exit immediately before A does kill(B, n).  In practice, it's okay
because the kernel is expected not to reassign a dead PID for some
reasonable grace period [1].  I'd be inclined to lean more heavily
on that expectation than anything internal to Postgres.  That is,
remembering the PID we want to kill for some small number of
microseconds is probably a safer API than anything that depends on
the contents of the ProcArray, because there indeed *isn't* any
guarantee that a ProcArray entry won't be recycled immediately.

regards, tom lane

[1] and also because the kernel *can't* recycle the PID until the
parent process has reaped the zombie process-table entry.  Thus,
for example, it's unconditionally safe for the postmaster to signal
its children, because those PIDs can't move until the postmaster
accepts the SIGCHLD signal and does a wait() for them.  Any
interprocess signals between child processes are inherently a tad
less safe.  But we've gotten away with interprocess SIGUSR1 for
decades with no reported problems.  I don't really think that we
need to move the goalposts for SIGINT, and I'm entirely not in
favor of the sorts of complications that are being proposed here.

Re: dropdb --force

[Amit Kapila]

On Sun, Nov 3, 2019 at 2:08 AM Pavel Stehule  wrote:
>

>> pá 25. 10. 2019 v 4:55 odesílatel Amit Kapila  
>> napsal:
>>>
>>> On Thu, Oct 24, 2019 at 8:22 PM Pavel Stehule  
>>> wrote:
>>> >
>>> > čt 24. 10. 2019 v 11:10 odesílatel Amit Kapila  
>>> > napsal:
>>> >>
>>> >> While making some changes in the patch, I noticed that in
>>> >> TerminateOtherDBBackends, there is a race condition where after we
>>> >> release the ProcArrayLock, the backend process to which we decided to
>>> >> send a signal exits by itself and the same pid can be assigned to
>>> >> another backend which is connected to some other database.  This leads
>>> >> to terminating a wrong backend.  I think we have some similar race
>>> >> condition at few other places like in pg_terminate_backend(),
>>> >> ProcSleep() and CountOtherDBBackends().  I think here the risk is a
>>> >> bit more because there could be a long list of pids.
>>> >>
>>> >> One idea could be that we write a new function similar to IsBackendPid
>>> >> which takes dbid and ensures that pid belongs to that database and use
>>> >> that before sending kill signal, but still it will not be completely
>>> >> safe.  But, I think it can be closer to cases like we already have in
>>> >> code.
>>> >>
>>> >> Another possible idea could be to use the SendProcSignal mechanism
>>> >> similar to how we have used it in CancelDBBackends() to allow the
>>> >> required backends to exit by themselves.  This might be safer.
>>> >>
>>> >> I am not sure if we can entirely eliminate this race condition and
>>> >> whether it is a good idea to accept such a race condition even though
>>> >> it exists in other parts of code.  What do you think?
>>> >>
>>> >> BTW, I have added/revised some comments in the code and done few other
>>> >> cosmetic changes, the result of which is attached.
>>> >
>>> >
>>> > Tomorrow I'll check variants that you mentioned.
>>> >
>>> > We sure so there are not any new connect to removed database, because we 
>>> > hold lock there.
>>> >
>>>
>>> Right.
>>>
>>> > So check if oid db is same should be enough.
>>> >
>>>
>>> We can do this before sending a kill signal but is it enough?  Because
>>> as soon as we release ProcArrayLock anytime the other process can exit
>>> and a new process can use its pid.  I think this is more of a
>>> theoretical risk and might not be easy to hit, but still, we can't
>>> ignore it.
>>
>>
>> yes, there is a theoretical risk probably - the released pid should near 
>> current fresh pid from range 0 .. pid_max.
>>
>> Probably the best solutions is enhancing SendProcSignal and using it here 
>> and fix CountOtherDBBackends.
>>
>> Alternative solution can be killing in block 50 processes and recheck. I'll 
>> try to write both and you can decide for one
>
>
> I am sending patch where kill was replaced by SendProcSignal. I tested it on 
> pg_bench with 400 connections, and it works without problems.
>

I think there is still a window where the same problem can happen, say
the signal has been sent by SendProcSignal to the required process and
it releases the ProcArrayLock.  Now, the target process exits and a
new process gets the same pid before the signal is received.

I am not sure but I think we can avoid such a race condition if we set
a flag (say killPending or something like that on the lines of
recoveryConflictPending) in proc and then check that flag before
allowing the process to die.  If something on these lines is feasible,
then I think there will be no race condition where we will kill the
wrong backend.  If we do this, then probably, just setting the flag
under ProcArrayLock is sufficient.  The signal can be sent outside the
lock.

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

Re: dropdb --force

[Pavel Stehule]

> I am sending patch where kill was replaced by SendProcSignal. I tested it
> on pg_bench with 400 connections, and it works without problems.
>

I tested it under high load and it is works. But it is slower than just
kill - under load the killing 400 connections needs about 1.5 sec.

Maybe for forced drop database we can increase max time limit to 10 seconds
(maybe more (statement timeout)) ? It is granted so we wait just on sigterm
handling. Other situations are finished by error. So in this case is very
probable so waiting will be successful and then we can wait long time.


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

Re: dropdb --force

[Pavel Stehule]

Hi

so 2. 11. 2019 v 17:18 odesílatel Pavel Stehule 
napsal:

>
>
> pá 25. 10. 2019 v 4:55 odesílatel Amit Kapila 
> napsal:
>
>> On Thu, Oct 24, 2019 at 8:22 PM Pavel Stehule 
>> wrote:
>> >
>> > čt 24. 10. 2019 v 11:10 odesílatel Amit Kapila 
>> napsal:
>> >>
>> >> While making some changes in the patch, I noticed that in
>> >> TerminateOtherDBBackends, there is a race condition where after we
>> >> release the ProcArrayLock, the backend process to which we decided to
>> >> send a signal exits by itself and the same pid can be assigned to
>> >> another backend which is connected to some other database.  This leads
>> >> to terminating a wrong backend.  I think we have some similar race
>> >> condition at few other places like in pg_terminate_backend(),
>> >> ProcSleep() and CountOtherDBBackends().  I think here the risk is a
>> >> bit more because there could be a long list of pids.
>> >>
>> >> One idea could be that we write a new function similar to IsBackendPid
>> >> which takes dbid and ensures that pid belongs to that database and use
>> >> that before sending kill signal, but still it will not be completely
>> >> safe.  But, I think it can be closer to cases like we already have in
>> >> code.
>> >>
>> >> Another possible idea could be to use the SendProcSignal mechanism
>> >> similar to how we have used it in CancelDBBackends() to allow the
>> >> required backends to exit by themselves.  This might be safer.
>> >>
>> >> I am not sure if we can entirely eliminate this race condition and
>> >> whether it is a good idea to accept such a race condition even though
>> >> it exists in other parts of code.  What do you think?
>> >>
>> >> BTW, I have added/revised some comments in the code and done few other
>> >> cosmetic changes, the result of which is attached.
>> >
>> >
>> > Tomorrow I'll check variants that you mentioned.
>> >
>> > We sure so there are not any new connect to removed database, because
>> we hold lock there.
>> >
>>
>> Right.
>>
>> > So check if oid db is same should be enough.
>> >
>>
>> We can do this before sending a kill signal but is it enough?  Because
>> as soon as we release ProcArrayLock anytime the other process can exit
>> and a new process can use its pid.  I think this is more of a
>> theoretical risk and might not be easy to hit, but still, we can't
>> ignore it.
>>
>
> yes, there is a theoretical risk probably - the released pid should near
> current fresh pid from range 0 .. pid_max.
>
> Probably the best solutions is enhancing SendProcSignal and using it here
> and fix CountOtherDBBackends.
>
> Alternative solution can be killing in block 50 processes and recheck.
> I'll try to write both and you can decide for one
>

I am sending patch where kill was replaced by SendProcSignal. I tested it
on pg_bench with 400 connections, and it works without problems.

Regards

Pavel

>
> Pavel
>
>
>> --
>> With Regards,
>> Amit Kapila.
>> EnterpriseDB: http://www.enterprisedb.com
>>
>
diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml
index 3ac06c984a..cef1b90421 100644
--- a/doc/src/sgml/ref/drop_database.sgml
+++ b/doc/src/sgml/ref/drop_database.sgml
@@ -21,7 +21,11 @@ PostgreSQL documentation
 
  
 
-DROP DATABASE [ IF EXISTS ] name
+DROP DATABASE [ IF EXISTS ] name [ [ WITH ] ( option [, ...] ) ] 
+
+where option can be:
+
+FORCE
 
  
 
@@ -32,9 +36,11 @@ DROP DATABASE [ IF EXISTS ] name
DROP DATABASE drops a database. It removes the
catalog entries for the database and deletes the directory
containing the data.  It can only be executed by the database owner.
-   Also, it cannot be executed while you or anyone else are connected
-   to the target database.  (Connect to postgres or any
-   other database to issue this command.)
+   It cannot be executed while you are connected to the target database.
+   (Connect to postgres or any other database to issue this
+   command.)
+   Also, if anyone else is connected to the target database, this command will
+   fail unless you use the FORCE option described below.
   
 
   
@@ -64,6 +70,25 @@ DROP DATABASE [ IF EXISTS ] name
  
 

+
+   
+FORCE
+
+ 
+  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.
+ 
+ 
+  This will fail if the current user has no permissions to terminate other
+  connections. Required permissions are the same as with 
+  pg_terminate_backend, described in
+  .  This will also fail if we
+  are not able to terminate connections.
+ 
+
+   
+
   
  
 
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 01d66212e9..38a2bfa969 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -811,7 +811,7 @@ createdb_failure_callback(int code, Datum arg)
  * DROP DATABASE
  */
 

Re: dropdb --force

[Pavel Stehule]

pá 25. 10. 2019 v 4:55 odesílatel Amit Kapila 
napsal:

> On Thu, Oct 24, 2019 at 8:22 PM Pavel Stehule 
> wrote:
> >
> > čt 24. 10. 2019 v 11:10 odesílatel Amit Kapila 
> napsal:
> >>
> >> While making some changes in the patch, I noticed that in
> >> TerminateOtherDBBackends, there is a race condition where after we
> >> release the ProcArrayLock, the backend process to which we decided to
> >> send a signal exits by itself and the same pid can be assigned to
> >> another backend which is connected to some other database.  This leads
> >> to terminating a wrong backend.  I think we have some similar race
> >> condition at few other places like in pg_terminate_backend(),
> >> ProcSleep() and CountOtherDBBackends().  I think here the risk is a
> >> bit more because there could be a long list of pids.
> >>
> >> One idea could be that we write a new function similar to IsBackendPid
> >> which takes dbid and ensures that pid belongs to that database and use
> >> that before sending kill signal, but still it will not be completely
> >> safe.  But, I think it can be closer to cases like we already have in
> >> code.
> >>
> >> Another possible idea could be to use the SendProcSignal mechanism
> >> similar to how we have used it in CancelDBBackends() to allow the
> >> required backends to exit by themselves.  This might be safer.
> >>
> >> I am not sure if we can entirely eliminate this race condition and
> >> whether it is a good idea to accept such a race condition even though
> >> it exists in other parts of code.  What do you think?
> >>
> >> BTW, I have added/revised some comments in the code and done few other
> >> cosmetic changes, the result of which is attached.
> >
> >
> > Tomorrow I'll check variants that you mentioned.
> >
> > We sure so there are not any new connect to removed database, because we
> hold lock there.
> >
>
> Right.
>
> > So check if oid db is same should be enough.
> >
>
> We can do this before sending a kill signal but is it enough?  Because
> as soon as we release ProcArrayLock anytime the other process can exit
> and a new process can use its pid.  I think this is more of a
> theoretical risk and might not be easy to hit, but still, we can't
> ignore it.
>

yes, there is a theoretical risk probably - the released pid should near
current fresh pid from range 0 .. pid_max.

Probably the best solutions is enhancing SendProcSignal and using it here
and fix CountOtherDBBackends.

Alternative solution can be killing in block 50 processes and recheck. I'll
try to write both and you can decide for one

Pavel


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

Re: dropdb --force

[Amit Kapila]

On Thu, Oct 24, 2019 at 8:22 PM Pavel Stehule  wrote:
>
> čt 24. 10. 2019 v 11:10 odesílatel Amit Kapila  
> napsal:
>>
>> While making some changes in the patch, I noticed that in
>> TerminateOtherDBBackends, there is a race condition where after we
>> release the ProcArrayLock, the backend process to which we decided to
>> send a signal exits by itself and the same pid can be assigned to
>> another backend which is connected to some other database.  This leads
>> to terminating a wrong backend.  I think we have some similar race
>> condition at few other places like in pg_terminate_backend(),
>> ProcSleep() and CountOtherDBBackends().  I think here the risk is a
>> bit more because there could be a long list of pids.
>>
>> One idea could be that we write a new function similar to IsBackendPid
>> which takes dbid and ensures that pid belongs to that database and use
>> that before sending kill signal, but still it will not be completely
>> safe.  But, I think it can be closer to cases like we already have in
>> code.
>>
>> Another possible idea could be to use the SendProcSignal mechanism
>> similar to how we have used it in CancelDBBackends() to allow the
>> required backends to exit by themselves.  This might be safer.
>>
>> I am not sure if we can entirely eliminate this race condition and
>> whether it is a good idea to accept such a race condition even though
>> it exists in other parts of code.  What do you think?
>>
>> BTW, I have added/revised some comments in the code and done few other
>> cosmetic changes, the result of which is attached.
>
>
> Tomorrow I'll check variants that you mentioned.
>
> We sure so there are not any new connect to removed database, because we hold 
> lock there.
>

Right.

> So check if oid db is same should be enough.
>

We can do this before sending a kill signal but is it enough?  Because
as soon as we release ProcArrayLock anytime the other process can exit
and a new process can use its pid.  I think this is more of a
theoretical risk and might not be easy to hit, but still, we can't
ignore it.

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

Re: dropdb --force

[Pavel Stehule]

čt 24. 10. 2019 v 11:10 odesílatel Amit Kapila 
napsal:

> On Wed, Oct 23, 2019 at 12:59 PM Amit Kapila 
> wrote:
> >
> > On Tue, Oct 22, 2019 at 4:51 PM Pavel Stehule 
> wrote:
> > >
> > > út 22. 10. 2019 v 5:09 odesílatel Amit Kapila 
> napsal:
> > >>
> > >>
> > >> CountOtherDBBackends is called from other places as well, so I don't
> > >> think it is advisable to change the sleep time in that function.
> > >> Also, I don't want to add a parameter for it.  I think you have a
> > >> point that in some cases we might end up sleeping for 100ms when we
> > >> could do with less sleeping time, but I think it is true to some
> > >> extent today as well.  I think we can anyway change it in the future
> > >> if there is a problem with the sleep timing, but for now, I think we
> > >> can just call CountOtherDBBackends after sending SIGTERM and call it
> > >> good.  You might want to add a futuristic note in the code.
> > >>
> > >
> > > ok.
> > >
> > > I removed sleeping from TerminateOtherDBBackends().
> > >
> > > If you want to change any logic there, please, do it without any
> hesitations. Maybe I don't see, what you think.
> > >
> >
> > Fair enough, I will see if I need to change anything.
> >
>
> While making some changes in the patch, I noticed that in
> TerminateOtherDBBackends, there is a race condition where after we
> release the ProcArrayLock, the backend process to which we decided to
> send a signal exits by itself and the same pid can be assigned to
> another backend which is connected to some other database.  This leads
> to terminating a wrong backend.  I think we have some similar race
> condition at few other places like in pg_terminate_backend(),
> ProcSleep() and CountOtherDBBackends().  I think here the risk is a
> bit more because there could be a long list of pids.
>
> One idea could be that we write a new function similar to IsBackendPid
> which takes dbid and ensures that pid belongs to that database and use
> that before sending kill signal, but still it will not be completely
> safe.  But, I think it can be closer to cases like we already have in
> code.
>
> Another possible idea could be to use the SendProcSignal mechanism
> similar to how we have used it in CancelDBBackends() to allow the
> required backends to exit by themselves.  This might be safer.
>
> I am not sure if we can entirely eliminate this race condition and
> whether it is a good idea to accept such a race condition even though
> it exists in other parts of code.  What do you think?
>
> BTW, I have added/revised some comments in the code and done few other
> cosmetic changes, the result of which is attached.
>

Tomorrow I'll check variants that you mentioned.

We sure so there are not any new connect to removed database, because we
hold lock there. So check if oid db is same should be enough.

Pavel

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

Re: dropdb --force

[Amit Kapila]

On Wed, Oct 23, 2019 at 12:59 PM Amit Kapila  wrote:
>
> On Tue, Oct 22, 2019 at 4:51 PM Pavel Stehule  wrote:
> >
> > út 22. 10. 2019 v 5:09 odesílatel Amit Kapila  
> > napsal:
> >>
> >>
> >> CountOtherDBBackends is called from other places as well, so I don't
> >> think it is advisable to change the sleep time in that function.
> >> Also, I don't want to add a parameter for it.  I think you have a
> >> point that in some cases we might end up sleeping for 100ms when we
> >> could do with less sleeping time, but I think it is true to some
> >> extent today as well.  I think we can anyway change it in the future
> >> if there is a problem with the sleep timing, but for now, I think we
> >> can just call CountOtherDBBackends after sending SIGTERM and call it
> >> good.  You might want to add a futuristic note in the code.
> >>
> >
> > ok.
> >
> > I removed sleeping from TerminateOtherDBBackends().
> >
> > If you want to change any logic there, please, do it without any 
> > hesitations. Maybe I don't see, what you think.
> >
>
> Fair enough, I will see if I need to change anything.
>

While making some changes in the patch, I noticed that in
TerminateOtherDBBackends, there is a race condition where after we
release the ProcArrayLock, the backend process to which we decided to
send a signal exits by itself and the same pid can be assigned to
another backend which is connected to some other database.  This leads
to terminating a wrong backend.  I think we have some similar race
condition at few other places like in pg_terminate_backend(),
ProcSleep() and CountOtherDBBackends().  I think here the risk is a
bit more because there could be a long list of pids.

One idea could be that we write a new function similar to IsBackendPid
which takes dbid and ensures that pid belongs to that database and use
that before sending kill signal, but still it will not be completely
safe.  But, I think it can be closer to cases like we already have in
code.

Another possible idea could be to use the SendProcSignal mechanism
similar to how we have used it in CancelDBBackends() to allow the
required backends to exit by themselves.  This might be safer.

I am not sure if we can entirely eliminate this race condition and
whether it is a good idea to accept such a race condition even though
it exists in other parts of code.  What do you think?

BTW, I have added/revised some comments in the code and done few other
cosmetic changes, the result of which is attached.


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


drop-database-force-20191024.amit.patch
Description: Binary data

Re: dropdb --force

[Amit Kapila]

On Tue, Oct 22, 2019 at 4:51 PM Pavel Stehule  wrote:
>
> út 22. 10. 2019 v 5:09 odesílatel Amit Kapila  
> napsal:
>>
>>
>> CountOtherDBBackends is called from other places as well, so I don't
>> think it is advisable to change the sleep time in that function.
>> Also, I don't want to add a parameter for it.  I think you have a
>> point that in some cases we might end up sleeping for 100ms when we
>> could do with less sleeping time, but I think it is true to some
>> extent today as well.  I think we can anyway change it in the future
>> if there is a problem with the sleep timing, but for now, I think we
>> can just call CountOtherDBBackends after sending SIGTERM and call it
>> good.  You might want to add a futuristic note in the code.
>>
>
> ok.
>
> I removed sleeping from TerminateOtherDBBackends().
>
> If you want to change any logic there, please, do it without any hesitations. 
> Maybe I don't see, what you think.
>

Fair enough, I will see if I need to change anything.  In the
meantime, can you look into thread related to CountDBSubscriptions[1]?
 I think the problem reported there will be more serious after your
patch, so it is better if we can fix it before this patch.

[1] - 
https://www.postgresql.org/message-id/CAA4eK1%2BqhLkCYG2oy9xug9ur_j%3DG2wQNRYAyd%2B-kZfZ1z42pLw%40mail.gmail.com

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

Re: dropdb --force

[Pavel Stehule]

út 22. 10. 2019 v 5:09 odesílatel Amit Kapila 
napsal:

> On Mon, Oct 21, 2019 at 2:15 PM Pavel Stehule 
> wrote:
> >
> > po 21. 10. 2019 v 10:25 odesílatel Amit Kapila 
> napsal:
> >>
> >>
> >> Sorry, but I am not able to understand the reason.  Are you worried
> >> about the comments atop CountOtherDBBackends which says it is used in
> >> Drop Database and related commands?
> >
> >
> > no, just now the code in dropdb looks like
> >
> > if (force)
> > TerminateOtherDBBackends(...);
> >
> > CountOtherDBBackends(...);
> >
> > if I call CountOtherDBBackends from TerminateOtherDBBackends, then code
> will look like
> >
> > if (force)
> >   TerminateOtherDBBackends(...);
> > else
> >   CountOtherDBBackends(...);
> >
> > That looks like CountOtherDBBackends is not called when force clause is
> active. And this is not true.
> >
>
> Hmm, can't we pass force as a parameter to TerminateOtherDBBackends()
> and then take the decision inside that function?  That will make the
> code of dropdb function a bit simpler.
>

I don't think so it is correct. Because without FORCE flag, you should not
to call TeminateOtherDBBackend ever.

Maybe I don't understand what is wrong.

if (force)
  terminate();

CountOtherDBBackends()
if (some numbers)
   ereport(ERROR, ..

This is fully correct for me.




> > So I prefer current relations between routines.
> >
> >
> >
> >>
> >> > But I can (and I have not any problem with it) remove or
> significantly decrease sleeping time in TerminateOtherDBBackends.
> >> >
> >> > 100 ms is maybe very much - but zero is maybe too low. If there will
> not be any time between TerminateOtherDBBackends and CountOtherDBBackends,
> then probably CountOtherDBBackends hit waiting 100ms.
> >> >
> >> > What about only 5 ms sleeping in TerminateOtherDBBackends?
> >> >
> >>
> >> I am not completely sure about what is the most appropriate thing to
> >> do, but I favor removing sleep from TerminateOtherDBBackends.  OTOH,
> >> there is nothing broken with the logic.  Anyone else wants to weigh in
> >> here?
> >
> >
> > ok. But when I remove it, should not be better to set waiting in
> CountOtherDBBackends to some smaller number than 100ms?
> >
>
> CountOtherDBBackends is called from other places as well, so I don't
> think it is advisable to change the sleep time in that function.
> Also, I don't want to add a parameter for it.  I think you have a
> point that in some cases we might end up sleeping for 100ms when we
> could do with less sleeping time, but I think it is true to some
> extent today as well.  I think we can anyway change it in the future
> if there is a problem with the sleep timing, but for now, I think we
> can just call CountOtherDBBackends after sending SIGTERM and call it
> good.  You might want to add a futuristic note in the code.
>
>
ok.

I removed sleeping from TerminateOtherDBBackends().

If you want to change any logic there, please, do it without any
hesitations. Maybe I don't see, what you think.

Regards

Pavel

>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml
index 3ac06c984a..9b9cabe71c 100644
--- a/doc/src/sgml/ref/drop_database.sgml
+++ b/doc/src/sgml/ref/drop_database.sgml
@@ -21,7 +21,11 @@ PostgreSQL documentation
 
  
 
-DROP DATABASE [ IF EXISTS ] name
+DROP DATABASE [ IF EXISTS ] name [ [ WITH ] ( option [, ...] ) ] 
+
+where option can be one of:
+
+FORCE
 
  
 
@@ -32,9 +36,11 @@ DROP DATABASE [ IF EXISTS ] name
DROP DATABASE drops a database. It removes the
catalog entries for the database and deletes the directory
containing the data.  It can only be executed by the database owner.
-   Also, it cannot be executed while you or anyone else are connected
-   to the target database.  (Connect to postgres or any
-   other database to issue this command.)
+   Also, it cannot be executed while you are connected to the target database.
+   (Connect to postgres or any other database to issue this
+   command.)
+   If anyone else is connected to the target database, this command will fail
+   unless you use the FORCE option described below.
   
 
   
@@ -64,6 +70,28 @@ DROP DATABASE [ IF EXISTS ] name
  
 

+
+   
+FORCE
+
+ 
+  Attempt to terminate all existing connections to the target database.
+  It doesn't terminate prepared transactions or active logical replication
+  slot(s).
+ 
+ 
+  This will fail, if current user has no permissions to terminate other
+  connections. Required permissions are the same as with 
+  pg_terminate_backend, described
+  in .
+
+  This will fail if we are not able to terminate connections or
+  when there are active prepared transactions or active logical replication
+  slots.
+ 
+
+   
+
   
  
 
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 01d66212e9..8e62359b4d 100644
--- 

Re: dropdb --force

[Amit Kapila]

On Mon, Oct 21, 2019 at 2:15 PM Pavel Stehule  wrote:
>
> po 21. 10. 2019 v 10:25 odesílatel Amit Kapila  
> napsal:
>>
>>
>> Sorry, but I am not able to understand the reason.  Are you worried
>> about the comments atop CountOtherDBBackends which says it is used in
>> Drop Database and related commands?
>
>
> no, just now the code in dropdb looks like
>
> if (force)
> TerminateOtherDBBackends(...);
>
> CountOtherDBBackends(...);
>
> if I call CountOtherDBBackends from TerminateOtherDBBackends, then code will 
> look like
>
> if (force)
>   TerminateOtherDBBackends(...);
> else
>   CountOtherDBBackends(...);
>
> That looks like CountOtherDBBackends is not called when force clause is 
> active. And this is not true.
>

Hmm, can't we pass force as a parameter to TerminateOtherDBBackends()
and then take the decision inside that function?  That will make the
code of dropdb function a bit simpler.

> So I prefer current relations between routines.
>
>
>
>>
>> > But I can (and I have not any problem with it) remove or significantly 
>> > decrease sleeping time in TerminateOtherDBBackends.
>> >
>> > 100 ms is maybe very much - but zero is maybe too low. If there will not 
>> > be any time between TerminateOtherDBBackends and CountOtherDBBackends, 
>> > then probably CountOtherDBBackends hit waiting 100ms.
>> >
>> > What about only 5 ms sleeping in TerminateOtherDBBackends?
>> >
>>
>> I am not completely sure about what is the most appropriate thing to
>> do, but I favor removing sleep from TerminateOtherDBBackends.  OTOH,
>> there is nothing broken with the logic.  Anyone else wants to weigh in
>> here?
>
>
> ok. But when I remove it, should not be better to set waiting in  
> CountOtherDBBackends to some smaller number than 100ms?
>

CountOtherDBBackends is called from other places as well, so I don't
think it is advisable to change the sleep time in that function.
Also, I don't want to add a parameter for it.  I think you have a
point that in some cases we might end up sleeping for 100ms when we
could do with less sleeping time, but I think it is true to some
extent today as well.  I think we can anyway change it in the future
if there is a problem with the sleep timing, but for now, I think we
can just call CountOtherDBBackends after sending SIGTERM and call it
good.  You might want to add a futuristic note in the code.


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

Re: dropdb --force

[Pavel Stehule]

po 21. 10. 2019 v 10:25 odesílatel Amit Kapila 
napsal:

> On Mon, Oct 21, 2019 at 12:24 PM Pavel Stehule 
> wrote:
> >
> > po 21. 10. 2019 v 8:38 odesílatel Amit Kapila 
> napsal:
> >>
> >> > If we don't wait in TerminateOtherDBBackends, then probably there
> should be necessary some cycles inside CountOtherDBBackends. I think so
> code like is correct
> >> >
> >> > 1. send SIGTERM to target processes
> >> > 2. put some time to processes for logout (100ms)
> >> > 3. check in loop (max 5 sec) on logout of all processes
> >> >
> >> > Maybe my feeling is wrong, but I think so it is good to wait few time
> instead to call CountOtherDBBackends immediately - the first iteration
> should to fail, and then first iteration is useless without chance on
> success.
> >> >
> >>
> >> I think the way I am suggesting by skipping the second step will allow
> >> sleeping only when required.  Consider a case where there are just one
> >> or two sessions connected to the database and they immediately exited
> >> after the signal is sent.  In such a case you don't need to sleep at
> >> all whereas, under your proposal, it will always sleep.  In the case
> >> where a large number of sessions are present and the first 100ms are
> >> not sufficient, we anyway need to wait dynamically.  So, I think the
> >> second step not only looks odd but also seems to be redundant.
> >
> >
> > I checked the code, and I think so calling  CountOtherDBBackends from
> TerminateOtherDBBackends is not good idea. CountOtherDBBackends should be
> called anywhere, TerminateOtherDBBackends only with FORCE flag. So I
> wouldn't to change code.
> >
>
> Sorry, but I am not able to understand the reason.  Are you worried
> about the comments atop CountOtherDBBackends which says it is used in
> Drop Database and related commands?
>

no, just now the code in dropdb looks like

if (force)
TerminateOtherDBBackends(...);

CountOtherDBBackends(...);

if I call CountOtherDBBackends from TerminateOtherDBBackends, then code
will look like

if (force)
  TerminateOtherDBBackends(...);
else
  CountOtherDBBackends(...);

That looks like CountOtherDBBackends is not called when force clause is
active. And this is not true.

So I prefer current relations between routines.




> > But I can (and I have not any problem with it) remove or significantly
> decrease sleeping time in TerminateOtherDBBackends.
> >
> > 100 ms is maybe very much - but zero is maybe too low. If there will not
> be any time between TerminateOtherDBBackends and CountOtherDBBackends, then
> probably CountOtherDBBackends hit waiting 100ms.
> >
> > What about only 5 ms sleeping in TerminateOtherDBBackends?
> >
>
> I am not completely sure about what is the most appropriate thing to
> do, but I favor removing sleep from TerminateOtherDBBackends.  OTOH,
> there is nothing broken with the logic.  Anyone else wants to weigh in
> here?
>

ok. But when I remove it, should not be better to set waiting in
CountOtherDBBackends to some smaller number than 100ms?

Pavel

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

Re: dropdb --force

[Pavel Stehule]

Hi


> When you say 'without any problem', do you mean to say that the drop
> database is successful?  In your tests were those sessions idle?  If
> so, I think we should try with busy sessions.  Also, if you write any
> script to do these tests, kindly share the same so that others can
> also repeat those tests.
>

I run pgbench on database with -i -S 100 and -c 1000. It is maximum for my
notebook. There was high load - about 50, and still DROP DATABASE FORCE is
working without any problems



> >(under low load (only pg_sleep was called).
> >
>
> I guess this is also possible because immediately after
> TerminateOtherDBBackends, we call CountOtherDBBackends which again
> give 5s to allow active connections to die. I am wondering why not we
> call CountOtherDBBackends from TerminateOtherDBBackends after sending
> the SIGTERM to all the sessions that are connected to the database
> being dropped?  Currently, it looks odd that first, we wait for 100ms
> after sending the signal and then separately wait for 5s in another
> function.
>

I checked code, and would not to change calls. Now I think the code is well
readable and has logical sense. But we can decrease sleep in
TerminateOtherDBBackends from 100 ms to 5 ms (just to increase chance to be
all killed processes done, and then waiting in CountOtherDBBackends 100ms
will not be hit.


> Other comments:
> 1.
> diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
> index 26a0bcf718..c8f545be18 100644
> --- a/src/test/regress/sql/psql.sql
> +++ b/src/test/regress/sql/psql.sql
> @@ -1182,3 +1182,11 @@ drop role regress_partitioning_role;
>
>  -- \d on toast table (use pg_statistic's toast table, which has a known
> name)
>  \d pg_toast.pg_toast_2619
> +
> +--
> +-- DROP DATABASE FORCE test of syntax (should not to raise syntax error)
> +--
> +drop database not_exists (force);
> +drop database not_exists with (force);
> +drop database if exists not_exists (force);
> +drop database if exists not_exists with (force);
>
> I don't think psql.sql is the right place to add such tests.
> Actually, there doesn't appear to be any database specific test file.
> However, if we want to add to any existing file, then maybe
> drop_if_exits.sql could be a better place for these tests as compare
> to psql.sql.
>

moved


> 2.
> + if (nprepared > 0)
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_IN_USE),
> + errmsg_plural("database \"%s\" is used by %d prepared transaction",
> +"database \"%s\" is used by %d prepared transactions",
> +nprepared,
> +get_database_name(databaseId), nprepared)));
>
> I think it is better if we mimic exactly what we have in the failure
> of  CountOtherDBBackends.
>

done

Regards

Pavel


> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml
index 3ac06c984a..9b9cabe71c 100644
--- a/doc/src/sgml/ref/drop_database.sgml
+++ b/doc/src/sgml/ref/drop_database.sgml
@@ -21,7 +21,11 @@ PostgreSQL documentation
 
  
 
-DROP DATABASE [ IF EXISTS ] name
+DROP DATABASE [ IF EXISTS ] name [ [ WITH ] ( option [, ...] ) ] 
+
+where option can be one of:
+
+FORCE
 
  
 
@@ -32,9 +36,11 @@ DROP DATABASE [ IF EXISTS ] name
DROP DATABASE drops a database. It removes the
catalog entries for the database and deletes the directory
containing the data.  It can only be executed by the database owner.
-   Also, it cannot be executed while you or anyone else are connected
-   to the target database.  (Connect to postgres or any
-   other database to issue this command.)
+   Also, it cannot be executed while you are connected to the target database.
+   (Connect to postgres or any other database to issue this
+   command.)
+   If anyone else is connected to the target database, this command will fail
+   unless you use the FORCE option described below.
   
 
   
@@ -64,6 +70,28 @@ DROP DATABASE [ IF EXISTS ] name
  
 

+
+   
+FORCE
+
+ 
+  Attempt to terminate all existing connections to the target database.
+  It doesn't terminate prepared transactions or active logical replication
+  slot(s).
+ 
+ 
+  This will fail, if current user has no permissions to terminate other
+  connections. Required permissions are the same as with 
+  pg_terminate_backend, described
+  in .
+
+  This will fail if we are not able to terminate connections or
+  when there are active prepared transactions or active logical replication
+  slots.
+ 
+
+   
+
   
  
 
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 01d66212e9..8e62359b4d 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -811,7 +811,7 @@ createdb_failure_callback(int code, Datum arg)
  * DROP DATABASE
  */
 void
-dropdb(const char *dbname, bool missing_ok)
+dropdb(const char *dbname, bool missing_ok, bool force)
 

Re: dropdb --force

[Amit Kapila]

On Mon, Oct 21, 2019 at 11:08 AM Pavel Stehule  wrote:
>
> po 21. 10. 2019 v 7:11 odesílatel Amit Kapila  
> napsal:
>>
>> >(under low load (only pg_sleep was called).
>> >
>>
>> I guess this is also possible because immediately after
>> TerminateOtherDBBackends, we call CountOtherDBBackends which again
>> give 5s to allow active connections to die. I am wondering why not we
>> call CountOtherDBBackends from TerminateOtherDBBackends after sending
>> the SIGTERM to all the sessions that are connected to the database
>> being dropped?  Currently, it looks odd that first, we wait for 100ms
>> after sending the signal and then separately wait for 5s in another
>> function.
>
>
> I'll look to this part, but I don't think so it is bad. 5s is maximum, not 
> minimum of waiting. So if sigterm is successful in first 100ms, then  
> CountOtherDBBackends doesn't add any time. If not, then it dynamically 
> waiting.
>
> If we don't wait in TerminateOtherDBBackends, then probably there should be 
> necessary some cycles inside CountOtherDBBackends. I think so code like is 
> correct
>
> 1. send SIGTERM to target processes
> 2. put some time to processes for logout (100ms)
> 3. check in loop (max 5 sec) on logout of all processes
>
> Maybe my feeling is wrong, but I think so it is good to wait few time instead 
> to call CountOtherDBBackends immediately - the first iteration should to 
> fail, and then first iteration is useless without chance on success.
>

I think the way I am suggesting by skipping the second step will allow
sleeping only when required.  Consider a case where there are just one
or two sessions connected to the database and they immediately exited
after the signal is sent.  In such a case you don't need to sleep at
all whereas, under your proposal, it will always sleep.  In the case
where a large number of sessions are present and the first 100ms are
not sufficient, we anyway need to wait dynamically.  So, I think the
second step not only looks odd but also seems to be redundant.

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

Re: dropdb --force

[Pavel Stehule]

po 21. 10. 2019 v 7:11 odesílatel Amit Kapila 
napsal:

> On Sun, Oct 20, 2019 at 2:06 AM Pavel Stehule 
> wrote:
> >
> > so 19. 10. 2019 v 12:37 odesílatel Amit Kapila 
> napsal:
> >>
> >> On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule 
> wrote:
> >> >
> >> >>
> >> >> Can we add few tests for this feature.
> >> >
> >> > there are not any other test for DROP DATABASE
> >> >
> >>
> >> I think there are no dedicated tests for it but in a few tests, we use
> >> it like in contrib\sepgsql\sql\alter.sql.  I am not sure if we can
> >> write a predictable test for force option because it will never be
> >> guaranteed to drop the database in the presence of other active
> >> sessions.
> >
> >
> > done - I push tests to /tests/regress/psql.sql
> >
> >>
> >> Few more comments:
> >> -
> >>
> >> 2.
> >> TerminateOtherDBBackends()
> >> {
> >> ..
> >> + /* We know so we have all necessary rights now */
> >> + foreach (lc, pids)
> >> + {
> >> + int pid = lfirst_int(lc);
> >> + PGPROC*proc = BackendPidGetProc(pid);
> >> +
> >> + if (proc != NULL)
> >> + {
> >> + /* If we have setsid(), signal the backend's whole process group */
> >> +#ifdef HAVE_SETSID
> >> + (void) kill(-pid, SIGTERM);
> >> +#else
> >> + (void) kill(pid, SIGTERM);
> >> +#endif
> >> + }
> >> + }
> >> +
> >> + /* sleep 100ms */
> >> + pg_usleep(100 * 1000L);
> >> ..
> >> }
> >>
> >> So here we are sending SIGTERM to all the processes and wait for 100ms
> >> to allow them to exit.  Have you tested this with many processes?  I
> >> think we can create 100~500 sessions or maybe more to the database
> >> being dropped and see what is behavior?  One thing to notice is that
> >> in function CountOtherDBBackends() we are sending SIGTERM to 10
> >> autovacuum processes at-a-time.  That has been introduced in commit
> >> 4abd7b49f1e9, but the reason for the same is not mentioned.  I am not
> >> sure if it is to avoid sending SIGTERM to many processes in quick
> >> succession.
> >
> >
> > I tested it on linux Linux nemesis
> >
> > 5.3.6-300.fc31.x86_64 #1 SMP Mon Oct 14 12:26:42 UTC 2019 x86_64 x86_64
> x86_64 GNU/Linux
> >
> > Tested with 1800 connections without any problem
> >
>
> When you say 'without any problem', do you mean to say that the drop
> database is successful?  In your tests were those sessions idle?  If
> so, I think we should try with busy sessions.  Also, if you write any
> script to do these tests, kindly share the same so that others can
> also repeat those tests.
>
>
sessions was active - but the function pg_sleep was called. Drop table
(mainly logout of these users was successful).

I had a script just

for i in {1..1000}; do (psql -c "select pg_sleep(1000)" omega &> /dev/null
&); done

I'll try to do some experiments - unfortunately,I have not a hw where I can
test very large number of connections.

But surely I can use pg_bench, and I can check pg_bench load



> >(under low load (only pg_sleep was called).
> >
>
> I guess this is also possible because immediately after
> TerminateOtherDBBackends, we call CountOtherDBBackends which again
> give 5s to allow active connections to die. I am wondering why not we
> call CountOtherDBBackends from TerminateOtherDBBackends after sending
> the SIGTERM to all the sessions that are connected to the database
> being dropped?  Currently, it looks odd that first, we wait for 100ms
> after sending the signal and then separately wait for 5s in another
> function.
>

I'll look to this part, but I don't think so it is bad. 5s is maximum, not
minimum of waiting. So if sigterm is successful in first 100ms, then
CountOtherDBBackends doesn't add any time. If not, then it dynamically
waiting.

If we don't wait in TerminateOtherDBBackends, then probably there should be
necessary some cycles inside CountOtherDBBackends. I think so code like is
correct

1. send SIGTERM to target processes
2. put some time to processes for logout (100ms)
3. check in loop (max 5 sec) on logout of all processes

Maybe my feeling is wrong, but I think so it is good to wait few time
instead to call CountOtherDBBackends immediately - the first iteration
should to fail, and then first iteration is useless without chance on
success.


> Other comments:
> 1.
> diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
> index 26a0bcf718..c8f545be18 100644
> --- a/src/test/regress/sql/psql.sql
> +++ b/src/test/regress/sql/psql.sql
> @@ -1182,3 +1182,11 @@ drop role regress_partitioning_role;
>
>  -- \d on toast table (use pg_statistic's toast table, which has a known
> name)
>  \d pg_toast.pg_toast_2619
> +
> +--
> +-- DROP DATABASE FORCE test of syntax (should not to raise syntax error)
> +--
> +drop database not_exists (force);
> +drop database not_exists with (force);
> +drop database if exists not_exists (force);
> +drop database if exists not_exists with (force);
>
> I don't think psql.sql is the right place to add such tests.
> Actually, there doesn't appear to be any database specific 

Re: dropdb --force

[Amit Kapila]

On Sun, Oct 20, 2019 at 2:06 AM Pavel Stehule  wrote:
>
> so 19. 10. 2019 v 12:37 odesílatel Amit Kapila  
> napsal:
>>
>> On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule  wrote:
>> >
>> >>
>> >> Can we add few tests for this feature.
>> >
>> > there are not any other test for DROP DATABASE
>> >
>>
>> I think there are no dedicated tests for it but in a few tests, we use
>> it like in contrib\sepgsql\sql\alter.sql.  I am not sure if we can
>> write a predictable test for force option because it will never be
>> guaranteed to drop the database in the presence of other active
>> sessions.
>
>
> done - I push tests to /tests/regress/psql.sql
>
>>
>> Few more comments:
>> -
>>
>> 2.
>> TerminateOtherDBBackends()
>> {
>> ..
>> + /* We know so we have all necessary rights now */
>> + foreach (lc, pids)
>> + {
>> + int pid = lfirst_int(lc);
>> + PGPROC*proc = BackendPidGetProc(pid);
>> +
>> + if (proc != NULL)
>> + {
>> + /* If we have setsid(), signal the backend's whole process group */
>> +#ifdef HAVE_SETSID
>> + (void) kill(-pid, SIGTERM);
>> +#else
>> + (void) kill(pid, SIGTERM);
>> +#endif
>> + }
>> + }
>> +
>> + /* sleep 100ms */
>> + pg_usleep(100 * 1000L);
>> ..
>> }
>>
>> So here we are sending SIGTERM to all the processes and wait for 100ms
>> to allow them to exit.  Have you tested this with many processes?  I
>> think we can create 100~500 sessions or maybe more to the database
>> being dropped and see what is behavior?  One thing to notice is that
>> in function CountOtherDBBackends() we are sending SIGTERM to 10
>> autovacuum processes at-a-time.  That has been introduced in commit
>> 4abd7b49f1e9, but the reason for the same is not mentioned.  I am not
>> sure if it is to avoid sending SIGTERM to many processes in quick
>> succession.
>
>
> I tested it on linux Linux nemesis
>
> 5.3.6-300.fc31.x86_64 #1 SMP Mon Oct 14 12:26:42 UTC 2019 x86_64 x86_64 
> x86_64 GNU/Linux
>
> Tested with 1800 connections without any problem
>

When you say 'without any problem', do you mean to say that the drop
database is successful?  In your tests were those sessions idle?  If
so, I think we should try with busy sessions.  Also, if you write any
script to do these tests, kindly share the same so that others can
also repeat those tests.

>(under low load (only pg_sleep was called).
>

I guess this is also possible because immediately after
TerminateOtherDBBackends, we call CountOtherDBBackends which again
give 5s to allow active connections to die. I am wondering why not we
call CountOtherDBBackends from TerminateOtherDBBackends after sending
the SIGTERM to all the sessions that are connected to the database
being dropped?  Currently, it looks odd that first, we wait for 100ms
after sending the signal and then separately wait for 5s in another
function.

Other comments:
1.
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 26a0bcf718..c8f545be18 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1182,3 +1182,11 @@ drop role regress_partitioning_role;

 -- \d on toast table (use pg_statistic's toast table, which has a known name)
 \d pg_toast.pg_toast_2619
+
+--
+-- DROP DATABASE FORCE test of syntax (should not to raise syntax error)
+--
+drop database not_exists (force);
+drop database not_exists with (force);
+drop database if exists not_exists (force);
+drop database if exists not_exists with (force);

I don't think psql.sql is the right place to add such tests.
Actually, there doesn't appear to be any database specific test file.
However, if we want to add to any existing file, then maybe
drop_if_exits.sql could be a better place for these tests as compare
to psql.sql.

2.
+ if (nprepared > 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_IN_USE),
+ errmsg_plural("database \"%s\" is used by %d prepared transaction",
+"database \"%s\" is used by %d prepared transactions",
+nprepared,
+get_database_name(databaseId), nprepared)));

I think it is better if we mimic exactly what we have in the failure
of  CountOtherDBBackends.

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

Re: dropdb --force

[Pavel Stehule]

so 19. 10. 2019 v 12:37 odesílatel Amit Kapila 
napsal:

> On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule 
> wrote:
> >
> >>
> >> Can we add few tests for this feature.
> >
> > there are not any other test for DROP DATABASE
> >
>
> I think there are no dedicated tests for it but in a few tests, we use
> it like in contrib\sepgsql\sql\alter.sql.  I am not sure if we can
> write a predictable test for force option because it will never be
> guaranteed to drop the database in the presence of other active
> sessions.
>

done - I push tests to /tests/regress/psql.sql


> Few more comments:
> -
> 1.
> + if (nprepared > 0)
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_IN_USE),
> + errmsg("database \"%s\" is used by %d prepared transaction(s)",
> + get_database_name(databaseId),
> + nprepared)));
> +
>
> You need to use errdetail_plural here to avoid translation problems,
> see docs[1] for a detailed explanation.  You can use function
> errdetail_busy_db.  Also, the indentation is not proper.
>

fixed


> 2.
> TerminateOtherDBBackends()
> {
> ..
> + /* We know so we have all necessary rights now */
> + foreach (lc, pids)
> + {
> + int pid = lfirst_int(lc);
> + PGPROC*proc = BackendPidGetProc(pid);
> +
> + if (proc != NULL)
> + {
> + /* If we have setsid(), signal the backend's whole process group */
> +#ifdef HAVE_SETSID
> + (void) kill(-pid, SIGTERM);
> +#else
> + (void) kill(pid, SIGTERM);
> +#endif
> + }
> + }
> +
> + /* sleep 100ms */
> + pg_usleep(100 * 1000L);
> ..
> }
>
> So here we are sending SIGTERM to all the processes and wait for 100ms
> to allow them to exit.  Have you tested this with many processes?  I
> think we can create 100~500 sessions or maybe more to the database
> being dropped and see what is behavior?  One thing to notice is that
> in function CountOtherDBBackends() we are sending SIGTERM to 10
> autovacuum processes at-a-time.  That has been introduced in commit
> 4abd7b49f1e9, but the reason for the same is not mentioned.  I am not
> sure if it is to avoid sending SIGTERM to many processes in quick
> succession.
>

I tested it on linux Linux nemesis

5.3.6-300.fc31.x86_64 #1 SMP Mon Oct 14 12:26:42 UTC 2019 x86_64 x86_64
x86_64 GNU/Linux

Tested with 1800 connections without any problem (under low load (only
pg_sleep was called).


> I think there should be more comments atop TerminateOtherDBBackends to
> explain the working of it and some assumptions of that function.
>

done


> 3.
> +opt_drop_option_list:
> + opt_with '(' drop_option_list ')' { $$ = $3; }
> + | /* EMPTY */
>
> I think it is better to keep opt_with as part of the main syntax
> rather than clubbing it with drop_option_list as we have in other
> cases in the code.
>

done


> 4.
> +drop_option: FORCE
> + {
> + $$ = makeDefElem("force", NULL, @1);
> + }
> + ;
>
> We generally keep the option name "FORCE" in the new line.
>

done


> 5.  I think it is better if can support tab-completion for this feature.
>

done

I am sending fresh patch

Regards

Pavel


> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml
index 3ac06c984a..9b9cabe71c 100644
--- a/doc/src/sgml/ref/drop_database.sgml
+++ b/doc/src/sgml/ref/drop_database.sgml
@@ -21,7 +21,11 @@ PostgreSQL documentation
 
  
 
-DROP DATABASE [ IF EXISTS ] name
+DROP DATABASE [ IF EXISTS ] name [ [ WITH ] ( option [, ...] ) ] 
+
+where option can be one of:
+
+FORCE
 
  
 
@@ -32,9 +36,11 @@ DROP DATABASE [ IF EXISTS ] name
DROP DATABASE drops a database. It removes the
catalog entries for the database and deletes the directory
containing the data.  It can only be executed by the database owner.
-   Also, it cannot be executed while you or anyone else are connected
-   to the target database.  (Connect to postgres or any
-   other database to issue this command.)
+   Also, it cannot be executed while you are connected to the target database.
+   (Connect to postgres or any other database to issue this
+   command.)
+   If anyone else is connected to the target database, this command will fail
+   unless you use the FORCE option described below.
   
 
   
@@ -64,6 +70,28 @@ DROP DATABASE [ IF EXISTS ] name
  
 

+
+   
+FORCE
+
+ 
+  Attempt to terminate all existing connections to the target database.
+  It doesn't terminate prepared transactions or active logical replication
+  slot(s).
+ 
+ 
+  This will fail, if current user has no permissions to terminate other
+  connections. Required permissions are the same as with 
+  pg_terminate_backend, described
+  in .
+
+  This will fail if we are not able to terminate connections or
+  when there are active prepared transactions or active logical replication
+  slots.
+ 
+
+   
+
   
  
 
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 

Re: dropdb --force

[Amit Kapila]

On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule  wrote:
>
>>
>> Can we add few tests for this feature.
>
> there are not any other test for DROP DATABASE
>

I think there are no dedicated tests for it but in a few tests, we use
it like in contrib\sepgsql\sql\alter.sql.  I am not sure if we can
write a predictable test for force option because it will never be
guaranteed to drop the database in the presence of other active
sessions.

Few more comments:
-
1.
+ if (nprepared > 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_IN_USE),
+ errmsg("database \"%s\" is used by %d prepared transaction(s)",
+ get_database_name(databaseId),
+ nprepared)));
+

You need to use errdetail_plural here to avoid translation problems,
see docs[1] for a detailed explanation.  You can use function
errdetail_busy_db.  Also, the indentation is not proper.

2.
TerminateOtherDBBackends()
{
..
+ /* We know so we have all necessary rights now */
+ foreach (lc, pids)
+ {
+ int pid = lfirst_int(lc);
+ PGPROC*proc = BackendPidGetProc(pid);
+
+ if (proc != NULL)
+ {
+ /* If we have setsid(), signal the backend's whole process group */
+#ifdef HAVE_SETSID
+ (void) kill(-pid, SIGTERM);
+#else
+ (void) kill(pid, SIGTERM);
+#endif
+ }
+ }
+
+ /* sleep 100ms */
+ pg_usleep(100 * 1000L);
..
}

So here we are sending SIGTERM to all the processes and wait for 100ms
to allow them to exit.  Have you tested this with many processes?  I
think we can create 100~500 sessions or maybe more to the database
being dropped and see what is behavior?  One thing to notice is that
in function CountOtherDBBackends() we are sending SIGTERM to 10
autovacuum processes at-a-time.  That has been introduced in commit
4abd7b49f1e9, but the reason for the same is not mentioned.  I am not
sure if it is to avoid sending SIGTERM to many processes in quick
succession.

I think there should be more comments atop TerminateOtherDBBackends to
explain the working of it and some assumptions of that function.

3.
+opt_drop_option_list:
+ opt_with '(' drop_option_list ')' { $$ = $3; }
+ | /* EMPTY */

I think it is better to keep opt_with as part of the main syntax
rather than clubbing it with drop_option_list as we have in other
cases in the code.

4.
+drop_option: FORCE
+ {
+ $$ = makeDefElem("force", NULL, @1);
+ }
+ ;

We generally keep the option name "FORCE" in the new line.

5.  I think it is better if can support tab-completion for this feature.

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

Re: dropdb --force

[Pavel Stehule]

ne 6. 10. 2019 v 10:19 odesílatel vignesh C  napsal:

> On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule 
> wrote:
> >
> >
> >
> > čt 3. 10. 2019 v 19:48 odesílatel vignesh C 
> napsal:
> >>
> >> On Wed, Oct 2, 2019 at 10:21 PM Pavel Stehule 
> 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)));
> >> + }
> >> +
> >
> >
> > I know - but somebody can call DropDatabase function outside parser. So
> is better check all possibilities.
> >
> >>
> >> We should change gram.y to accept any keyword and throw error from
> >> DropDatabase function.
> >> + */
> >> +drop_option: FORCE
> >> + {
> >> + $$ = makeDefElem("force", NULL, @1);
> >> + }
> >> + ;
> >
> >
> > I spent some time with thinking about it, and I think so this variant
> (with keyword) is well readable and very illustrative. This will be lost
> with generic variant.
> >
> > When the keyword FORCE already exists, then I prefer current state.
> >
> >>
> >>
> >> "This will also fail" should be "This will fail"
> >> + 
> >> +  This will fail, if current user has no permissions to terminate
> other
> >> +  connections. Required permissions are the same as with
> >> +  pg_terminate_backend, described
> >> +  in .
> >> +
> >> +  This will also fail if we are not able to terminate connections
> or
> >> +  when there are active prepared transactions or active logical
> replication
> >> +  slots.
> >> + 
> >
> >
> > fixed
> >
> >>
> >>
> >> Can we add few tests for this feature.
> >
> >
> > there are not any other test for DROP DATABASE
> >
> > We can check syntax later inside second patch (for -f option of dropdb
> command)
> >
> Changes in this patch looks fine to me.
> I'm not sure if we have forgotten to miss attaching the second patch
> or can you provide the link to second patch.
>

I plan to work on the second patch after committing of this first. Now we
are early on commit fest, and the complexity of this or second patch is not
too high be necessary to prepare patch series.

Regards

Pavel


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

Re: dropdb --force

[vignesh C]

On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule  wrote:
>
>
>
> čt 3. 10. 2019 v 19:48 odesílatel vignesh C  napsal:
>>
>> On Wed, Oct 2, 2019 at 10:21 PM Pavel Stehule  
>> 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)));
>> + }
>> +
>
>
> I know - but somebody can call DropDatabase function outside parser. So is 
> better check all possibilities.
>
>>
>> We should change gram.y to accept any keyword and throw error from
>> DropDatabase function.
>> + */
>> +drop_option: FORCE
>> + {
>> + $$ = makeDefElem("force", NULL, @1);
>> + }
>> + ;
>
>
> I spent some time with thinking about it, and I think so this variant (with 
> keyword) is well readable and very illustrative. This will be lost with 
> generic variant.
>
> When the keyword FORCE already exists, then I prefer current state.
>
>>
>>
>> "This will also fail" should be "This will fail"
>> + 
>> +  This will fail, if current user has no permissions to terminate other
>> +  connections. Required permissions are the same as with
>> +  pg_terminate_backend, described
>> +  in .
>> +
>> +  This will also fail if we are not able to terminate connections or
>> +  when there are active prepared transactions or active logical 
>> replication
>> +  slots.
>> + 
>
>
> fixed
>
>>
>>
>> Can we add few tests for this feature.
>
>
> there are not any other test for DROP DATABASE
>
> We can check syntax later inside second patch (for -f option of dropdb 
> command)
>
Changes in this patch looks fine to me.
I'm not sure if we have forgotten to miss attaching the second patch
or can you provide the link to second patch.

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

Re: dropdb --force

[Pavel Stehule]

čt 3. 10. 2019 v 19:48 odesílatel vignesh C  napsal:

> On Wed, Oct 2, 2019 at 10:21 PM Pavel Stehule 
> 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)));
> + }
> +
>

I know - but somebody can call DropDatabase function outside parser. So is
better check all possibilities.


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

I spent some time with thinking about it, and I think so this variant (with
keyword) is well readable and very illustrative. This will be lost with
generic variant.

When the keyword FORCE already exists, then I prefer current state.


>
> "This will also fail" should be "This will fail"
> + 
> +  This will fail, if current user has no permissions to terminate
> other
> +  connections. Required permissions are the same as with
> +  pg_terminate_backend, described
> +  in .
> +
> +  This will also fail if we are not able to terminate connections or
> +  when there are active prepared transactions or active logical
> replication
> +  slots.
> + 
>

fixed


>
> Can we add few tests for this feature.
>

there are not any other test for DROP DATABASE

We can check syntax later inside second patch (for -f option of dropdb
command)

Regards

Pavel

>
> Regards,
> Vignesh
> EnterpriseDB: http://www.enterprisedb.com
>
diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml
index 3ac06c984a..9b9cabe71c 100644
--- a/doc/src/sgml/ref/drop_database.sgml
+++ b/doc/src/sgml/ref/drop_database.sgml
@@ -21,7 +21,11 @@ PostgreSQL documentation
 
  
 
-DROP DATABASE [ IF EXISTS ] name
+DROP DATABASE [ IF EXISTS ] name [ [ WITH ] ( option [, ...] ) ] 
+
+where option can be one of:
+
+FORCE
 
  
 
@@ -32,9 +36,11 @@ DROP DATABASE [ IF EXISTS ] name
DROP DATABASE drops a database. It removes the
catalog entries for the database and deletes the directory
containing the data.  It can only be executed by the database owner.
-   Also, it cannot be executed while you or anyone else are connected
-   to the target database.  (Connect to postgres or any
-   other database to issue this command.)
+   Also, it cannot be executed while you are connected to the target database.
+   (Connect to postgres or any other database to issue this
+   command.)
+   If anyone else is connected to the target database, this command will fail
+   unless you use the FORCE option described below.
   
 
   
@@ -64,6 +70,28 @@ DROP DATABASE [ IF EXISTS ] name
  
 

+
+   
+FORCE
+
+ 
+  Attempt to terminate all existing connections to the target database.
+  It doesn't terminate prepared transactions or active logical replication
+  slot(s).
+ 
+ 
+  This will fail, if current user has no permissions to terminate other
+  connections. Required permissions are the same as with 
+  pg_terminate_backend, described
+  in .
+
+  This will fail if we are not able to terminate connections or
+  when there are active prepared transactions or active logical replication
+  slots.
+ 
+
+   
+
   
  
 
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 01d66212e9..8e62359b4d 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -811,7 +811,7 @@ createdb_failure_callback(int code, Datum arg)
  * DROP DATABASE
  */
 void
-dropdb(const char *dbname, bool missing_ok)
+dropdb(const char *dbname, bool missing_ok, bool force)
 {
 	Oid			db_id;
 	bool		db_istemplate;
@@ -896,6 +896,9 @@ dropdb(const char *dbname, bool missing_ok)
   nslots_active, nslots_active)));
 	}
 
+	if (force)
+		TerminateOtherDBBackends(db_id);
+
 	/*
 	 * Check for other backends in the target database.  (Because we hold the
 	 * database lock, no new ones can start after this.)
@@ -1003,6 +1006,30 @@ dropdb(const char *dbname, bool missing_ok)
 	ForceSyncCommit();
 }
 
+/*
+ * Process options and call dropdb function.
+ */
+void
+DropDatabase(ParseState *pstate, DropdbStmt *stmt)
+{
+	bool		force = false;
+	ListCell   *lc;
+
+	foreach(lc, stmt->options)
+	{
+		DefElem*opt = (DefElem *) lfirst(lc);
+
+		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)));
+	}
+
+	dropdb(stmt->dbname, 

Re: dropdb --force

[vignesh C]

On Thu, Oct 3, 2019 at 11:18 PM vignesh C  wrote:
>
> On Wed, Oct 2, 2019 at 10:21 PM Pavel Stehule  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"
> + 
> +  This will fail, if current user has no permissions to terminate other
> +  connections. Required permissions are the same as with
> +  pg_terminate_backend, described
> +  in .
> +
> +  This will also fail if we are not able to terminate connections or
> +  when there are active prepared transactions or active logical 
> replication
> +  slots.
> + 
>
> Can we add few tests for this feature.
>
Couple of minor comments:
DBNAME can be included
- * DROP DATABASE [ IF EXISTS ]
+ * DROP DATABASE [ IF EXISTS ] [ [ WITH ] ( options ) ]
can be
- * DROP DATABASE [ IF EXISTS ]
+ * DROP DATABASE [ IF EXISTS ] DBNAME [ [ WITH ] ( options ) ]

Should we include dbname in the below also?
+DROP DATABASE [ IF EXISTS ] name [ [ WITH ] ( option [, ...] ) ]
+
+where option can
be one of:

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

Re: dropdb --force

[vignesh C]

On Wed, Oct 2, 2019 at 10:21 PM Pavel Stehule  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"
+ 
+  This will fail, if current user has no permissions to terminate other
+  connections. Required permissions are the same as with
+  pg_terminate_backend, described
+  in .
+
+  This will also fail if we are not able to terminate connections or
+  when there are active prepared transactions or active logical replication
+  slots.
+ 

Can we add few tests for this feature.

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

Re: dropdb --force

[Pavel Stehule]

st 2. 10. 2019 v 5:20 odesílatel Amit Kapila 
napsal:

> On Thu, Sep 26, 2019 at 7:18 PM Pavel Stehule 
> wrote:
>
>>
>> út 24. 9. 2019 v 14:52 odesílatel Amit Kapila 
>> napsal:
>>
>>>
>>> One other minor comment:
>>> +
>>> +  This will also fail, if the connections do not terminate in 5
>>> seconds.
>>> + 
>>>
>>> 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, , ))
>> <--><-->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, _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 = [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 ]  class="parameter">name
> +DROP DATABASE [ IF EXISTS ]  class="parameter">name [ WITH (  class="parameter">option [, ...] ) ]
>
> 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 

Re: dropdb --force

[Amit Kapila]

On Thu, Sep 26, 2019 at 7:18 PM Pavel Stehule 
wrote:

>
> út 24. 9. 2019 v 14:52 odesílatel Amit Kapila 
> napsal:
>
>>
>> One other minor comment:
>> +
>> +  This will also fail, if the connections do not terminate in 5
>> seconds.
>> + 
>>
>> 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, , ))
> <--><-->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, _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 = [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 ] name
+DROP DATABASE [ IF EXISTS ] name [ WITH ( option [, ...] ) ]

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

Re: dropdb --force

[Amit Kapila]

On Thu, Sep 26, 2019 at 10:04 PM Peter Eisentraut
 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

Re: dropdb --force

[Pavel Stehule]

čt 26. 9. 2019 v 18:34 odesílatel Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> 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
>

Re: dropdb --force

[Peter Eisentraut]

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

Re: dropdb --force

[Pavel Stehule]

čt 26. 9. 2019 v 17:35 odesílatel Alvaro Herrera 
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 Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml
index 3ac06c984a..2c3857718f 100644
--- a/doc/src/sgml/ref/drop_database.sgml
+++ b/doc/src/sgml/ref/drop_database.sgml
@@ -21,7 +21,11 @@ PostgreSQL documentation
 
  
 
-DROP DATABASE [ IF EXISTS ] name
+DROP DATABASE [ IF EXISTS ] name [ WITH ( option [, ...] ) ] 
+
+where option can be one of:
+
+FORCE
 
  
 
@@ -32,9 +36,11 @@ DROP DATABASE [ IF EXISTS ] name
DROP DATABASE drops a database. It removes the
catalog entries for the database and deletes the directory
containing the data.  It can only be executed by the database owner.
-   Also, it cannot be executed while you or anyone else are connected
-   to the target database.  (Connect to postgres or any
-   other database to issue this command.)
+   Also, it cannot be executed while you are connected to the target database.
+   (Connect to postgres or any other database to issue this
+   command.)
+   If anyone else is connected to the target database, this command will fail
+   - unless you use the FORCE option described below.
   
 
   
@@ -64,6 +70,24 @@ DROP DATABASE [ IF EXISTS ] name
  
 

+
+   
+FORCE
+
+ 
+  Attempt to terminate all existing connections to the target database.
+ 
+ 
+  This will fail, if current user has no permissions to terminate other
+  connections. Required permissions are the same as with 
+  pg_terminate_backend, described
+  in .
+
+  This will also fail, if the connections do not terminate in 5 seconds.
+ 
+
+   
+
   
  
 
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 95881a8550..1a94671cdc 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -810,7 +810,7 @@ createdb_failure_callback(int code, Datum arg)
  * DROP DATABASE
  */
 void
-dropdb(const char *dbname, bool missing_ok)
+dropdb(const char *dbname, bool missing_ok, bool force)
 {
 	Oid			db_id;
 	bool		db_istemplate;
@@ -895,6 +895,9 @@ dropdb(const char *dbname, bool missing_ok)
   nslots_active, nslots_active)));
 	}
 
+	if (force)
+		TerminateOtherDBBackends(db_id);
+
 	/*
 	 * Check for other backends in the target database.  (Because we hold the
 	 * database lock, no new ones can start after this.)
@@ -1002,6 +1005,30 @@ dropdb(const char *dbname, bool missing_ok)
 	ForceSyncCommit();
 }
 
+/*
+ * Process options and call dropdb function.
+ */
+void
+DropDatabase(ParseState *pstate, DropdbStmt *stmt)
+{
+	bool		force = false;
+	ListCell   *lc;
+
+	foreach(lc, stmt->options)
+	{
+		DefElem*opt = (DefElem *) lfirst(lc);
+
+		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)));
+	}
+
+	dropdb(stmt->dbname, stmt->missing_ok, force);
+}
 
 /*
  * Rename database
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 3432bb921d..2f267e4bb6 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3868,6 +3868,7 @@ _copyDropdbStmt(const DropdbStmt *from)
 
 	COPY_STRING_FIELD(dbname);
 	COPY_SCALAR_FIELD(missing_ok);
+	COPY_NODE_FIELD(options);
 
 	return newnode;
 }
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 18cb014373..da0e1d139a 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1676,6 +1676,7 @@ _equalDropdbStmt(const DropdbStmt *a, const DropdbStmt *b)
 {
 	COMPARE_STRING_FIELD(dbname);
 	COMPARE_SCALAR_FIELD(missing_ok);
+	COMPARE_NODE_FIELD(options);
 
 	return true;
 }
diff --git 

Re: dropdb --force

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: dropdb --force

[Pavel Stehule]

st 25. 9. 2019 v 4:14 odesílatel Amit Kapila 
napsal:

> On Tue, Sep 24, 2019 at 6:22 PM Amit Kapila 
> wrote:
> > On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule 
> 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.
> > + 
> >
> > 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
>

Re: dropdb --force

[Pavel Stehule]

st 25. 9. 2019 v 6:38 odesílatel vignesh C  napsal:

> On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule 
> wrote:
> > fixed
> >
> > Thank you for check. I am sending updated patch
> >
> Thanks for fixing all the comments.
> Couple of suggestions:
> -DROP DATABASE [ IF EXISTS ]  class="parameter">name
> +DROP DATABASE [ ( option
> [, ...] ) ] [ IF EXISTS ]  class="parameter">name
> +
> +where option can
> be one of:
> +
> +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
>
diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml
index 3ac06c984a..11a31899d2 100644
--- a/doc/src/sgml/ref/drop_database.sgml
+++ b/doc/src/sgml/ref/drop_database.sgml
@@ -21,7 +21,11 @@ PostgreSQL documentation
 
  
 
-DROP DATABASE [ IF EXISTS ] name
+DROP DATABASE [ ( option [, ...] ) ] [ IF EXISTS ] name
+
+where option can be one of:
+
+FORCE
 
  
 
@@ -32,9 +36,11 @@ DROP DATABASE [ IF EXISTS ] name
DROP DATABASE drops a database. It removes the
catalog entries for the database and deletes the directory
containing the data.  It can only be executed by the database owner.
-   Also, it cannot be executed while you or anyone else are connected
-   to the target database.  (Connect to postgres or any
-   other database to issue this command.)
+   Also, it cannot be executed while you are connected to the target database.
+   (Connect to postgres or any other database to issue this
+   command.)
+   If anyone else is connected to the target database, this command will fail
+   - unless you use the FORCE option described below.
   
 
   
@@ -64,6 +70,24 @@ DROP DATABASE [ IF EXISTS ] name
  
 

+
+   
+FORCE
+
+ 
+  Attempt to terminate all existing connections to the target database.
+ 
+ 
+  This will fail, if current user has no permissions to terminate other
+  connections. Required permissions are the same as with 
+  pg_terminate_backend, described
+  in .
+
+  This will also fail, if the connections do not terminate in 5 seconds.
+ 
+
+   
+
   
  
 
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 95881a8550..1a94671cdc 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -810,7 +810,7 @@ createdb_failure_callback(int code, Datum arg)
  * DROP DATABASE
  */
 void
-dropdb(const char *dbname, bool missing_ok)
+dropdb(const char *dbname, bool missing_ok, bool force)
 {
 	Oid			db_id;
 	bool		db_istemplate;
@@ -895,6 +895,9 @@ dropdb(const char *dbname, bool missing_ok)
   nslots_active, nslots_active)));
 	}
 
+	if (force)
+		TerminateOtherDBBackends(db_id);
+
 	/*
 	 * Check for other backends in the target database.  (Because we hold the
 	 * database lock, no new ones can start after this.)
@@ -1002,6 +1005,30 @@ dropdb(const char *dbname, bool missing_ok)
 	ForceSyncCommit();
 }
 
+/*
+ * Process options and call dropdb function.
+ */
+void
+DropDatabase(ParseState *pstate, DropdbStmt *stmt)
+{
+	bool		force = false;
+	ListCell   *lc;
+
+	foreach(lc, stmt->options)
+	{
+		DefElem*opt = (DefElem *) lfirst(lc);
+
+		if (strcmp(opt->defname, "force") == 0)
+			force = true;
+		else
+			ereport(ERROR,
+	(errcode(ERRCODE_SYNTAX_ERROR),
+	 errmsg("unrecognized 

Re: dropdb --force

[Pavel Stehule]

út 24. 9. 2019 v 17:51 odesílatel vignesh C  napsal:

> On Tue, Sep 24, 2019 at 6:22 PM Amit Kapila 
> wrote:
> >
> > On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule 
> 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
>

Re: dropdb --force

[Pavel Stehule]

út 24. 9. 2019 v 14:52 odesílatel Amit Kapila 
napsal:

> On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule 
> 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.
> + 
>
> 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, , ))
<--><-->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
>

Re: dropdb --force

[vignesh C]

On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule  wrote:
> fixed
>
> Thank you for check. I am sending updated patch
>
Thanks for fixing all the comments.
Couple of suggestions:
-DROP DATABASE [ IF EXISTS ] name
+DROP DATABASE [ ( option
[, ...] ) ] [ IF EXISTS ] name
+
+where option can
be one of:
+
+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

Re: dropdb --force

[Amit Kapila]

On Tue, Sep 24, 2019 at 6:22 PM Amit Kapila  wrote:
> On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule  
> 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.
> + 
>
> 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

Re: dropdb --force

[vignesh C]

On Tue, Sep 24, 2019 at 6:22 PM Amit Kapila  wrote:
>
> On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule  
> 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

Re: dropdb --force

[Amit Kapila]

On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule  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.
+ 

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

Re: dropdb --force

[Pavel Stehule]

pá 20. 9. 2019 v 5:10 odesílatel Thomas Munro 
napsal:

> On Thu, Sep 19, 2019 at 6:44 AM Pavel Stehule 
> wrote:
> > I am sending updated version
>
> +appendPQExpBuffer(, "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
>

Re: dropdb --force

[Pavel Stehule]

pá 20. 9. 2019 v 7:58 odesílatel Dilip Kumar  napsal:

> On Thu, Sep 19, 2019 at 12:14 AM Pavel Stehule 
> 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();
>
> - appendPQExpBuffer(, "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(, "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
>
diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml
index 3ac06c984a..11a31899d2 100644
--- a/doc/src/sgml/ref/drop_database.sgml
+++ b/doc/src/sgml/ref/drop_database.sgml
@@ -21,7 +21,11 @@ PostgreSQL documentation
 
  
 
-DROP DATABASE [ IF EXISTS ] name
+DROP DATABASE [ ( option [, ...] ) ] [ IF EXISTS ] name
+
+where option can be one of:
+
+FORCE
 
  
 
@@ -32,9 +36,11 @@ DROP DATABASE [ IF EXISTS ] name
DROP DATABASE drops a database. It removes the
catalog entries for the database and deletes the directory
containing the data.  It can only be executed by the database owner.
-   Also, it cannot be executed while you or anyone else are connected
-   to the target database.  (Connect to postgres or any
-   other database to issue this command.)
+   Also, it cannot be executed while you are connected to the target database.
+   (Connect to postgres or any other database to issue this
+   command.)
+   If anyone else is connected to the target database, this command will fail
+   - unless you use the FORCE option described below.
   
 
   
@@ -64,6 +70,24 @@ DROP DATABASE [ IF EXISTS ] name
  
 

+
+   
+FORCE
+
+ 
+  Attempt to terminate all existing connections to the target database.
+ 
+ 
+  This will fail, if current user has no permissions to terminate other
+  connections. Required permissions are the same as with 
+  

Re: dropdb --force

[Dilip Kumar]

On Thu, Sep 19, 2019 at 12:14 AM Pavel Stehule  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();

- appendPQExpBuffer(, "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(, "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

Re: dropdb --force

[vignesh C]

On Thu, Sep 19, 2019 at 11:41 PM Pavel Stehule  wrote:
>
>
>
> čt 19. 9. 2019 v 13:52 odesílatel vignesh C  napsal:
>>
>> On Thu, Sep 19, 2019 at 12:14 AM Pavel Stehule  
>> 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

Re: dropdb --force

[vignesh C]

On Thu, Sep 19, 2019 at 12:14 AM Pavel Stehule  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

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
diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml
index 3ac06c984a..11a31899d2 100644
--- a/doc/src/sgml/ref/drop_database.sgml
+++ b/doc/src/sgml/ref/drop_database.sgml
@@ -21,7 +21,11 @@ PostgreSQL documentation
 
  
 
-DROP DATABASE [ IF EXISTS ] name
+DROP DATABASE [ ( option [, ...] ) ] [ IF EXISTS ] name
+
+where option can be one of:
+
+FORCE
 
  
 
@@ -32,9 +36,11 @@ DROP DATABASE [ IF EXISTS ] name
DROP DATABASE drops a database. It removes the
catalog entries for the database and deletes the directory
containing the data.  It can only be executed by the database owner.
-   Also, it cannot be executed while you or anyone else are connected
-   to the target database.  (Connect to postgres or any
-   other database to issue this command.)
+   Also, it cannot be executed while you are connected to the target database.
+   (Connect to postgres or any other database to issue this
+   command.)
+   If anyone else is connected to the target database, this command will fail
+   - unless you use the FORCE option described below.
   
 
   
@@ -64,6 +70,24 @@ DROP DATABASE [ IF EXISTS ] name
  
 

+
+   
+FORCE
+
+ 
+  Attempt to terminate all existing connections to the target database.
+ 
+ 
+  This will fail, if current user has no permissions to terminate other
+  connections. Required permissions are the same as with 
+  pg_terminate_backend, described
+  in .
+
+  This will also fail, if the connections do not terminate in 5 seconds.
+ 
+
+   
+
   
  
 
diff --git a/doc/src/sgml/ref/dropdb.sgml b/doc/src/sgml/ref/dropdb.sgml
index 3fbdb33716..5d10ad1c92 100644
--- a/doc/src/sgml/ref/dropdb.sgml
+++ b/doc/src/sgml/ref/dropdb.sgml
@@ -86,6 +86,20 @@ PostgreSQL documentation
   
  
 
+ 
+  -f
+  --force
+  
+   
+Force termination of connected backends before removing the database.
+   
+   
+This will add the FORCE option to the DROP
+DATABASE command sent to the server.
+   
+  
+ 
+
  
-V
--version
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 95881a8550..faa5bdbf71 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -810,7 +810,7 @@ createdb_failure_callback(int code, Datum arg)
  * DROP DATABASE
  */
 void
-dropdb(const char *dbname, bool missing_ok)
+dropdb(const char *dbname, bool missing_ok, bool force)
 {
 	Oid			db_id;
 	bool		db_istemplate;
@@ -895,6 +895,9 @@ dropdb(const char *dbname, bool missing_ok)
   nslots_active, nslots_active)));
 	}
 
+	if (force)
+		TerminateOtherDBBackends(db_id);
+
 	/*
 	 * Check for other backends in the target database.  (Because we hold the
 	 * database lock, no new ones can start after this.)
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 3432bb921d..2f267e4bb6 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3868,6 +3868,7 @@ _copyDropdbStmt(const DropdbStmt *from)
 
 	COPY_STRING_FIELD(dbname);
 	COPY_SCALAR_FIELD(missing_ok);
+	COPY_NODE_FIELD(options);
 
 	return newnode;
 }
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 18cb014373..da0e1d139a 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1676,6 +1676,7 @@ _equalDropdbStmt(const DropdbStmt *a, const DropdbStmt *b)
 {
 	COMPARE_STRING_FIELD(dbname);
 	COMPARE_SCALAR_FIELD(missing_ok);
+	COMPARE_NODE_FIELD(options);
 
 	return true;
 }
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 3f67aaf30e..f7bab61175 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -310,6 +310,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type 	vac_analyze_option_elem
 %type 	vac_analyze_option_list
 %type 	vac_analyze_option_arg
+%type 	drop_option
 %type 	opt_or_replace
 opt_grant_grant_option opt_grant_admin_option
 opt_nowait opt_if_exists opt_with_data
@@ -406,6 +407,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 TriggerTransitions TriggerReferencing
 publication_name_list
 vacuum_relation_list opt_vacuum_relation_list
+drop_option_list opt_drop_option_list
 
 %type 	group_by_list
 %type 	group_by_item empty_grouping_set rollup_clause 

Re: dropdb --force

[Pavel Stehule]

st 18. 9. 2019 v 19:11 odesílatel vignesh C  napsal:

> On Wed, Sep 18, 2019 at 9:41 AM Pavel Stehule 
> wrote:
> >
> >
> >
> > st 18. 9. 2019 v 5:59 odesílatel vignesh C  napsal:
> >>
> >> On Wed, Sep 18, 2019 at 8:30 AM Pavel Stehule 
> wrote:
> >> >
> >> >
> >> Hi Pavel,
> >>
> >> One Comment:
> >> In the documentation we say drop database will fail after 60 seconds
> >>   
> >> FORCE
> >> 
> >>  
> >>   Attempt to terminate all existing connections to the target
> database.
> >>  
> >>  
> >>   This will fail, if current user has no permissions to terminate
> other
> >>   connections. Required permissions are the same as with
> >>   pg_terminate_backend, described
> >>   in .
> >>
> >>   This will also fail, if the connections do not terminate in 60
> seconds.
> >>  
> >> 
> >>
> >
> >
> > 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
>

Re: dropdb --force

[vignesh C]

On Wed, Sep 18, 2019 at 9:41 AM Pavel Stehule  wrote:
>
>
>
> st 18. 9. 2019 v 5:59 odesílatel vignesh C  napsal:
>>
>> On Wed, Sep 18, 2019 at 8:30 AM Pavel Stehule  
>> wrote:
>> >
>> >
>> Hi Pavel,
>>
>> One Comment:
>> In the documentation we say drop database will fail after 60 seconds
>>   
>> FORCE
>> 
>>  
>>   Attempt to terminate all existing connections to the target database.
>>  
>>  
>>   This will fail, if current user has no permissions to terminate other
>>   connections. Required permissions are the same as with
>>   pg_terminate_backend, described
>>   in .
>>
>>   This will also fail, if the connections do not terminate in 60 seconds.
>>  
>> 
>>
>
>
> 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

Re: dropdb --force

[Pavel Stehule]

st 18. 9. 2019 v 5:59 odesílatel vignesh C  napsal:

> On Wed, Sep 18, 2019 at 8:30 AM Pavel Stehule 
> wrote:
> >
> >
> Hi Pavel,
>
> One Comment:
> In the documentation we say drop database will fail after 60 seconds
>   
> FORCE
> 
>  
>   Attempt to terminate all existing connections to the target database.
>  
>  
>   This will fail, if current user has no permissions to terminate other
>   connections. Required permissions are the same as with
>   pg_terminate_backend, described
>   in .
>
>   This will also fail, if the connections do not terminate in 60
> seconds.
>  
> 
>
>

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
>

Re: dropdb --force

[vignesh C]

On Wed, Sep 18, 2019 at 8:30 AM Pavel Stehule  wrote:
>
>
Hi Pavel,

One Comment:
In the documentation we say drop database will fail after 60 seconds
  
FORCE

 
  Attempt to terminate all existing connections to the target database.
 
 
  This will fail, if current user has no permissions to terminate other
  connections. Required permissions are the same as with
  pg_terminate_backend, described
  in .

  This will also fail, if the connections do not terminate in 60 seconds.
 

   


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