Re: dropdb --force
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
č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
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
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
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
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
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
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
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
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
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
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
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
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
č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
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
č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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
č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
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
č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
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
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
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
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
> 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
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
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
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
č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
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
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
ú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
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
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
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
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
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
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
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
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
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
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
č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
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
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
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
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
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
č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
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
č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
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
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
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
ú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
ú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
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
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
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
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
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
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
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
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
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
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
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
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
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
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