Обсуждение: dropdb --force

Hi,

I propose a simple patch (works-for-me), which adds --force (-f)
option to dropdb utility.

Pros:  This seems to be a desired option for many sysadmins, as this
thread proves: https://dba.stackexchange.com/questions/11893/force-drop-db-while-others-may-be-connected
Cons: another possible foot-gun for the unwary.

Obviously this patch needs some more work (see TODO note inside).

Please share opinions if this makes sense at all, and has any chance
going upstream.

The regression tests is simplistic, please help with an example of
multi-session test so I can follow.


Thanks,
Filip

Hi

> út 18. 12. 2018 v 16:11 odesílatel Filip Rembiałkowski <filip.rembialkowski@gmail.com> napsal:
>> Please share opinions if this makes sense at all, and has any chance
>> going upstream.

Clearly since Pavel has another implementation of the same concept,
there is some interest in this feature. :)

On Tue, Dec 18, 2018 at 5:20 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Still one my customer use a patch that implement FORCE on SQL level. It is necessary under higher load when is not
easyto synchronize clients. 

I think Filip's approach of setting pg_database.datallowconn='false'
is pretty clever to avoid the synchronization problem. But it's also a
good idea to expose this functionality via DROP DATABASE in SQL, like
Pavel's patch, not just the 'dropdb' binary.

If this is to be accepted into PostgreSQL core, I think the two
approaches should be combined on the server side.

Regards,
Marti Raudsepp

On Tue, Dec 18, 2018 at 01:25:32PM +0100, Filip Rembiałkowski wrote:
> Hi,
> 
> I propose a simple patch (works-for-me), which adds --force (-f)
> option to dropdb utility.

Nice!

I did something like this in user space back in 2010.

https://people.planetpostgresql.org/dfetter/index.php?/archives/63-Kick-em-out,-and-keep-em-out!.html

so there appears to be interest in such a feature.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Marti Raudsepp <marti@juffo.org> writes:
> I think Filip's approach of setting pg_database.datallowconn='false'
> is pretty clever to avoid the synchronization problem.

Some bull-in-a-china-shop has recently added logic that allows ignoring
datallowconn and connecting anyway, so I'm not sure that that'd provide
a production-quality solution.  On the other hand, maybe we could revert
BGWORKER_BYPASS_ALLOWCONN.

            regards, tom lane

I wonder if this idea from seven years ago might be useful:
https://postgr.es/m/1305688547-sup-7028@alvh.no-ip.org

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

Here is Pavel's patch rebased to master branch, added the dropdb
--force option, a test case & documentation.

I'm willing to work on it if needed. What are possible bad things that
could happen here? Is the documentation clear enough?

Thanks.


On Tue, Dec 18, 2018 at 4:34 PM Marti Raudsepp <marti@juffo.org> wrote:
>
> Hi
>
> > út 18. 12. 2018 v 16:11 odesílatel Filip Rembiałkowski <filip.rembialkowski@gmail.com> napsal:
> >> Please share opinions if this makes sense at all, and has any chance
> >> going upstream.
>
> Clearly since Pavel has another implementation of the same concept,
> there is some interest in this feature. :)
>
> On Tue, Dec 18, 2018 at 5:20 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> > Still one my customer use a patch that implement FORCE on SQL level. It is necessary under higher load when is not
easyto synchronize clients. 
>
> I think Filip's approach of setting pg_database.datallowconn='false'
> is pretty clever to avoid the synchronization problem. But it's also a
> good idea to expose this functionality via DROP DATABASE in SQL, like
> Pavel's patch, not just the 'dropdb' binary.
>
> If this is to be accepted into PostgreSQL core, I think the two
> approaches should be combined on the server side.
>
> Regards,
> Marti Raudsepp

On Wed, Mar 6, 2019 at 1:39 PM Filip Rembiałkowski
<filip.rembialkowski@gmail.com> wrote:
> Here is Pavel's patch rebased to master branch, added the dropdb
> --force option, a test case & documentation.

Hello,

cfbot.cputube.org says this fails on Windows, due to a missing semicolon here:

#ifdef HAVE_SETSID
kill(-(proc->pid), SIGTERM);
#else
kill(proc->pid, SIGTERM)
#endif

The test case failed on Linux, I didn't check why exactly:

Test Summary Report
-------------------
t/050_dropdb.pl (Wstat: 65280 Tests: 13 Failed: 2)
Failed tests: 12-13
Non-zero exit status: 255
Parse errors: Bad plan. You planned 11 tests but ran 13.

+/* Time to sleep after isuing SIGTERM to backends */
+#define TERMINATE_SLEEP_TIME 1

s/isuing/issuing/

But, hmm, this macro doesn't actually seem to be used in the patch.
Wait, is that because the retry loop forgot to actually include the
sleep?

+        /* without "force" flag raise exception immediately, or after
5 minutes */

Normally we call it an "error", not an "exception".

--
Thomas Munro
https://enterprisedb.com

Thank you. Updated patch attached.

On Sat, Mar 9, 2019 at 2:53 AM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Wed, Mar 6, 2019 at 1:39 PM Filip Rembiałkowski
> <filip.rembialkowski@gmail.com> wrote:
> > Here is Pavel's patch rebased to master branch, added the dropdb
> > --force option, a test case & documentation.
>
> Hello,
>
> cfbot.cputube.org says this fails on Windows, due to a missing semicolon here:
>
> #ifdef HAVE_SETSID
> kill(-(proc->pid), SIGTERM);
> #else
> kill(proc->pid, SIGTERM)
> #endif
>
> The test case failed on Linux, I didn't check why exactly:
>
> Test Summary Report
> -------------------
> t/050_dropdb.pl (Wstat: 65280 Tests: 13 Failed: 2)
> Failed tests: 12-13
> Non-zero exit status: 255
> Parse errors: Bad plan. You planned 11 tests but ran 13.
>
> +/* Time to sleep after isuing SIGTERM to backends */
> +#define TERMINATE_SLEEP_TIME 1
>
> s/isuing/issuing/
>
> But, hmm, this macro doesn't actually seem to be used in the patch.
> Wait, is that because the retry loop forgot to actually include the
> sleep?
>
> +        /* without "force" flag raise exception immediately, or after
> 5 minutes */
>
> Normally we call it an "error", not an "exception".
>
> --
> Thomas Munro
> https://enterprisedb.com

Hi,

On 2019-03-10 11:20:42 +0100, Filip Rembiałkowski wrote:
>  bool
> -CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared)
> +CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared, bool force_terminate)
>  {

That doesn't seem like a decent API to me.

Greetings,

Andres Freund

On 31.03.2019, 04:35 Andres Freund <andres@anarazel.de> wrote:

>
> >  bool
> > -CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared)
> > +CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared, bool force_terminate)
> >  {
>
> That doesn't seem like a decent API to me.

Only excuse is that naming was already a bit off, as the function
includes killing autovacuum workers.

Please advise what would be a good approach to improve it. I would
propose something like:


bool CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared);
// make it actually do what the name announces - only the count, no
side effects.

bool KillDBBackends(Oid databaseId, bool killAutovacuum, bool killBackends);
// try to kill off all the backends, return false when there are still any.



Also, there is a question - should the FORCE option rollback prepared
transactions?

Thanks!

Is this the intended behavior? SIGTERM is received.

test=# begin;
BEGIN
test=# create table test(a int);
CREATE TABLE


In another terminal drop the database.

test=# begin;
psql: FATAL:  terminating connection due to administrator command
server closed the connection unexpectedly
    This probably means the server terminated abnormally
    before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

Yes, I think it is because of this code Snippet

                    if (force_terminate)
                    {
                        /* try to terminate backend */
#ifdef HAVE_SETSID
                            kill(-(proc->pid), SIGTERM);
#else
                            kill(proc->pid, SIGTERM);



On Tue, Apr 9, 2019 at 3:06 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
>
> Is this the intended behavior? SIGTERM is received.
>
> test=# begin;
> BEGIN
> test=# create table test(a int);
> CREATE TABLE
>
>
> In another terminal drop the database.
>
> test=# begin;
> psql: FATAL:  terminating connection due to administrator command
> server closed the connection unexpectedly
>         This probably means the server terminated abnormally
>         before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> !>



-- 
Ibrar Ahmed

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

The feature works fine on my machine. The code is well-written.

The new status of this patch is: Ready for Committer

Also works fine according to my testing. Documentation is also clear.
Thanks for this useful patch.

Hi,
patch no longer applies (as of 12beta2). 

postgres@ubuntudev:~/pg_testing/source/postgresql-12beta2$ patch -p1 < drop-database-force-20190310_01.patch
patching file doc/src/sgml/ref/drop_database.sgml
patching file doc/src/sgml/ref/dropdb.sgml
patching file src/backend/commands/dbcommands.c
Hunk #1 succeeded at 489 (offset 2 lines).
Hunk #2 succeeded at 779 (offset 2 lines).
Hunk #3 succeeded at 787 (offset 2 lines).
Hunk #4 succeeded at 871 (offset 2 lines).
Hunk #5 succeeded at 1056 (offset 2 lines).
Hunk #6 succeeded at 1186 (offset 2 lines).
patching file src/backend/nodes/copyfuncs.c
Hunk #1 succeeded at 3852 (offset 10 lines).
patching file src/backend/nodes/equalfuncs.c
Hunk #1 succeeded at 1666 (offset 3 lines).
patching file src/backend/parser/gram.y
Hunk #1 succeeded at 10194 (offset 45 lines).
Hunk #2 succeeded at 10202 (offset 45 lines).
patching file src/backend/storage/ipc/procarray.c
Hunk #1 succeeded at 2906 (offset -14 lines).
Hunk #2 succeeded at 2948 (offset -14 lines).
patching file src/backend/tcop/utility.c
patching file src/bin/scripts/dropdb.c
Hunk #1 succeeded at 34 (offset 1 line).
Hunk #2 succeeded at 50 (offset 1 line).
Hunk #3 succeeded at 63 (offset 2 lines).
Hunk #4 succeeded at 88 (offset 2 lines).
Hunk #5 succeeded at 128 (offset 2 lines).
Hunk #6 succeeded at 136 (offset 2 lines).
Hunk #7 succeeded at 164 (offset 1 line).
patching file src/bin/scripts/t/050_dropdb.pl
patching file src/include/commands/dbcommands.h
patching file src/include/nodes/parsenodes.h
Hunk #1 succeeded at 3133 (offset 16 lines).
patching file src/include/storage/procarray.h
Hunk #1 FAILED at 112.
1 out of 1 hunk FAILED -- saving rejects to file src/include/storage/procarray.h.rej

Could you please update it ? Thanks.
Anthony

The new status of this patch is: Waiting on Author

On Thu, Jun 27, 2019 at 7:15 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> fixed

Hi Pavel,

FYI t/050_dropdb.pl fails consistently with this patch applied:

https://travis-ci.org/postgresql-cfbot/postgresql/builds/555234838


-- 
Thomas Munro
https://enterprisedb.com

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

Hi,

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

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

Ryan Lambert

The new status of this patch is: Ready for Committer

Pavel Stehule <pavel.stehule@gmail.com> writes:
> [ drop-database-force-20190708.patch ]

I took a brief look at this, but I don't think it's really close to
being committable.

* The documentation claims FORCE will fail if you don't have privileges
to terminate the other session(s) in the target DB.  This is a lie; the
code issues kill() without any regard for such niceties.  You could
perhaps make that better by going through pg_signal_backend().

* You've hacked CountOtherDBBackends to do something that's completely
outside the charter one would expect from its name, and not even
bothered to update its header comment.  This requires more attention
to not confusing future hackers; I'd say you can't even use that
function name anymore.

* You've also ignored the existing code in CountOtherDBBackends
that is careful *not* to issue a kill() while holding the critical
ProcArrayLock.  That problem would get enormously worse if you
tried to sub in pg_signal_backend() there, since that function
may do catalog accesses --- it's pretty likely that you could
get actual deadlocks, never mind just trashing performance.

* I really dislike the addition of more hard-wired delays and
timeouts to dropdb().  It's bad enough that CountOtherDBBackends
has got that.  Two layers of timeout are a rickety mess, and
who's to say that a 1-minute timeout is appropriate for anything?

* I'm concerned that the proposed syntax is not future-proof.
FORCE is not a reserved word, and we surely don't want to make
it one; but just appending it to the end of the command without
any decoration seems like a recipe for problems if anybody wants
to add other options later.  (Possible examples: RESTRICT/CASCADE,
or a user-defined timeout.)  Maybe some parentheses would help?
Or possibly I'm being overly paranoid, but ...


I hadn't been paying any attention to this thread before now,
but I'd assumed from the thread title that the idea was to implement
any attempted kills in the dropdb app, not on the backend side.
(As indeed it looks like the first version did.)  Maybe it would be
better to go back to that, instead of putting dubious behaviors
into the core server.

            regards, tom lane

On Thu, Jul 25, 2019 at 8:45 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> čt 25. 7. 2019 v 5:11 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> > [ drop-database-force-20190708.patch ]
>>
>> I took a brief look at this, but I don't think it's really close to
>> being committable.

Hi Pavel,

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

--
Thomas Munro
https://enterprisedb.com

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

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

Thanks,
Ryan

On 2019-Jul-25, Pavel Stehule wrote:

> čt 25. 7. 2019 v 5:11 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
> 
> > Pavel Stehule <pavel.stehule@gmail.com> writes:

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

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

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

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

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

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

On Wed, Sep 18, 2019 at 8:30 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
Hi Pavel,

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

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


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

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

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

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

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

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

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

On Thu, Sep 19, 2019 at 12:14 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> Hi
>
> I am sending updated version - the changes against last patch are two. I use pg_terminate_backed for killing other
terminateslike Tom proposed. I am not sure if it is 100% practical - on second hand the necessary right to kill other
sessionsis  almost available - and consistency with pg_terminal_backend has sense. More - native implementation is
significantlybetter then solution implemented outer. I fixed docs little bit - the timeout for logout (as reaction on
SIGTERMis 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

On Thu, Sep 19, 2019 at 6:44 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I am sending updated version

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

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

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

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

-- 
Thomas Munro
https://enterprisedb.com

On Thu, Sep 19, 2019 at 11:41 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> čt 19. 9. 2019 v 13:52 odesílatel vignesh C <vignesh21@gmail.com> napsal:
>>
>> On Thu, Sep 19, 2019 at 12:14 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> >
>> > Hi
>> >
>> > I am sending updated version - the changes against last patch are two. I use pg_terminate_backed for killing other
terminateslike Tom proposed. I am not sure if it is 100% practical - on second hand the necessary right to kill other
sessionsis  almost available - and consistency with pg_terminal_backend has sense. More - native implementation is
significantlybetter then solution implemented outer. I fixed docs little bit - the timeout for logout (as reaction on
SIGTERMis 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

On Thu, Sep 19, 2019 at 12:14 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> Hi
>
> I am sending updated version - the changes against last patch are two. I use pg_terminate_backed for killing other
terminateslike Tom proposed. I am not sure if it is 100% practical - on second hand the necessary right to kill other
sessionsis  almost available - and consistency with pg_terminal_backend has sense. More - native implementation is
significantlybetter then solution implemented outer. I fixed docs little bit - the timeout for logout (as reaction on
SIGTERMis only 5 sec). 
>

Few comments on the patch.

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

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

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


initPQExpBuffer(&sql);

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

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


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

Unrelated change

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

On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> Thank you for check. I am sending updated patch
>

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

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

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

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

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

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

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

On Tue, Sep 24, 2019 at 6:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule <pavel.stehule@gmail.com> 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

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

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


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

On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> fixed
>
> Thank you for check. I am sending updated patch
>
Thanks for fixing all the comments.
Couple of suggestions:
-DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
+DROP DATABASE [ ( <replaceable class="parameter">option</replaceable>
[, ...] ) ] [ IF EXISTS ] <replaceable
class="parameter">name</replaceable>
+
+<phrase>where <replaceable class="parameter">option</replaceable> can
be one of:</phrase>
+
+    FORCE

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

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

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

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

On 2019-Sep-26, Pavel Stehule wrote:

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

Well, you would have one of those:

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

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

You propose

DROP DATABASE (FORCE) [IF EXISTS] name

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

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

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

On Thu, Sep 26, 2019 at 10:04 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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

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

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

"This will also fail" should be "This will fail"
+     <para>
+      This will fail, if current user has no permissions to terminate other
+      connections. Required permissions are the same as with
+      <literal>pg_terminate_backend</literal>, described
+      in <xref linkend="functions-admin-signal"/>.
+
+      This will also fail if we are not able to terminate connections or
+      when there are active prepared transactions or active logical replication
+      slots.
+     </para>

Can we add few tests for this feature.

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

On Thu, Oct 3, 2019 at 11:18 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, Oct 2, 2019 at 10:21 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> >
> > Thank you for careful review. I hope so all issues are out.
> >
> >
> Thanks Pavel for fixing the comments.
> Few comments:
> The else part cannot be hit in DropDatabase function as gram.y expects FORCE.
> +
> + if (strcmp(opt->defname, "force") == 0)
> + force = true;
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("unrecognized DROP DATABASE option \"%s\"", opt->defname),
> + parser_errposition(pstate, opt->location)));
> + }
> +
>
> We should change gram.y to accept any keyword and throw error from
> DropDatabase function.
> + */
> +drop_option: FORCE
> + {
> + $$ = makeDefElem("force", NULL, @1);
> + }
> + ;
>
> "This will also fail" should be "This will fail"
> +     <para>
> +      This will fail, if current user has no permissions to terminate other
> +      connections. Required permissions are the same as with
> +      <literal>pg_terminate_backend</literal>, described
> +      in <xref linkend="functions-admin-signal"/>.
> +
> +      This will also fail if we are not able to terminate connections or
> +      when there are active prepared transactions or active logical replication
> +      slots.
> +     </para>
>
> Can we add few tests for this feature.
>
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 ] <replaceable
class="parameter">name</replaceable> [ [ WITH ] ( <replaceable
class="parameter">option</replaceable> [, ...] ) ]
+
+<phrase>where <replaceable class="parameter">option</replaceable> can
be one of:</phrase>

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

On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> čt 3. 10. 2019 v 19:48 odesílatel vignesh C <vignesh21@gmail.com> napsal:
>>
>> On Wed, Oct 2, 2019 at 10:21 PM Pavel Stehule <pavel.stehule@gmail.com> 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"
>> +     <para>
>> +      This will fail, if current user has no permissions to terminate other
>> +      connections. Required permissions are the same as with
>> +      <literal>pg_terminate_backend</literal>, described
>> +      in <xref linkend="functions-admin-signal"/>.
>> +
>> +      This will also fail if we are not able to terminate connections or
>> +      when there are active prepared transactions or active logical replication
>> +      slots.
>> +     </para>
>
>
> 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

On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule <pavel.stehule@gmail.com> 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

On Sun, Oct 20, 2019 at 2:06 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> so 19. 10. 2019 v 12:37 odesílatel Amit Kapila <amit.kapila16@gmail.com> napsal:
>>
>> On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule <pavel.stehule@gmail.com> 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

On Mon, Oct 21, 2019 at 11:08 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> po 21. 10. 2019 v 7:11 odesílatel Amit Kapila <amit.kapila16@gmail.com> 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
successfulin 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

On Mon, Oct 21, 2019 at 12:24 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> po 21. 10. 2019 v 8:38 odesílatel Amit Kapila <amit.kapila16@gmail.com> 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.
CountOtherDBBackendsshould 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?

> 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
andCountOtherDBBackends, 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?



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

On Mon, Oct 21, 2019 at 2:15 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> po 21. 10. 2019 v 10:25 odesílatel Amit Kapila <amit.kapila16@gmail.com> 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
TerminateOtherDBBackendsand 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

On Tue, Oct 22, 2019 at 4:51 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> út 22. 10. 2019 v 5:09 odesílatel Amit Kapila <amit.kapila16@gmail.com> 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

On Wed, Oct 23, 2019 at 12:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Oct 22, 2019 at 4:51 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> >
> > út 22. 10. 2019 v 5:09 odesílatel Amit Kapila <amit.kapila16@gmail.com> 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

On Thu, Oct 24, 2019 at 8:22 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> čt 24. 10. 2019 v 11:10 odesílatel Amit Kapila <amit.kapila16@gmail.com> 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

On Sun, Nov 3, 2019 at 2:08 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>

>> pá 25. 10. 2019 v 4:55 odesílatel Amit Kapila <amit.kapila16@gmail.com> napsal:
>>>
>>> On Thu, Oct 24, 2019 at 8:22 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>> >
>>> > čt 24. 10. 2019 v 11:10 odesílatel Amit Kapila <amit.kapila16@gmail.com> 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
workswithout 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

Amit Kapila <amit.kapila16@gmail.com> 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.

On Wed, Nov 6, 2019 at 11:46 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> st 6. 11. 2019 v 14:59 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>>
>> Amit Kapila <amit.kapila16@gmail.com> 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

On Thu, Nov 7, 2019 at 10:46 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> čt 7. 11. 2019 v 3:42 odesílatel Amit Kapila <amit.kapila16@gmail.com> napsal:
>>
>> On Wed, Nov 6, 2019 at 11:46 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> >
>> > st 6. 11. 2019 v 14:59 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>> >>
>> >> Amit Kapila <amit.kapila16@gmail.com> 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

On Thu, Nov 7, 2019 at 11:29 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> čt 7. 11. 2019 v 6:56 odesílatel Amit Kapila <amit.kapila16@gmail.com> 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

On Fri, Nov 8, 2019 at 4:13 PM Pavel Stehule <pavel.stehule@gmail.com> 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

On Fri, Nov 8, 2019 at 4:57 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> pá 8. 11. 2019 v 11:50 odesílatel Amit Kapila <amit.kapila16@gmail.com> 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

On Mon, Nov 11, 2019 at 8:45 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Nov 8, 2019 at 4:57 PM Pavel Stehule <pavel.stehule@gmail.com> 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

On Mon, Nov 11, 2019 at 1:57 PM Amit Kapila <amit.kapila16@gmail.com> 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

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

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

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

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

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

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

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

The above documentation can be changed similar to drop_database.sgml:
     <para>
      Attempt to terminate all existing connections to the target database.
      It doesn't terminate if prepared transactions, active logical replication
      slots or subscriptions are present in the target database.
     </para>
     <para>
      This will fail if the current user has no permissions to terminate other
      connections. Required permissions are the same as with
      <literal>pg_terminate_backend</literal>, described in
      <xref linkend="functions-admin-signal"/>.  This will also fail if we
      are not able to terminate connections.
     </para>

We can make the modification in the same location as earlier in the below case:
-    appendPQExpBuffer(&sql, "DROP DATABASE %s%s;",
-                      (if_exists ? "IF EXISTS " : ""), fmtId(dbname));
-
     /* Avoid trying to drop postgres db while we are connected to it. */
     if (maintenance_db == NULL && strcmp(dbname, "postgres") == 0)
         maintenance_db = "template1";
@@ -134,6 +136,12 @@ main(int argc, char *argv[])
                                       host, port, username, prompt_password,
                                       progname, echo);

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

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

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

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

On Fri, Nov 15, 2019 at 1:23 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> updated patch attached
>

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

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

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

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

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

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

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

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

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

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

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

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

On Mon, Nov 18, 2019 at 6:30 PM Pavel Stehule <pavel.stehule@gmail.com> 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

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

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

On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Nov 22, 2019 at 3:16 PM vignesh C <vignesh21@gmail.com> 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

On Sun, Nov 24, 2019 at 3:55 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila <amit.kapila16@gmail.com> 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

On Sun, Nov 24, 2019 at 5:06 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> ne 24. 11. 2019 v 11:25 odesílatel vignesh C <vignesh21@gmail.com> napsal:
>>
>> On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> >
>> > On Fri, Nov 22, 2019 at 3:16 PM vignesh C <vignesh21@gmail.com> 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

On Mon, Nov 25, 2019 at 11:22 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Sun, Nov 24, 2019 at 5:06 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> >
> >
> >
> > ne 24. 11. 2019 v 11:25 odesílatel vignesh C <vignesh21@gmail.com> napsal:
> >>
> >> On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >> >
> >> > On Fri, Nov 22, 2019 at 3:16 PM vignesh C <vignesh21@gmail.com> 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

On Tue, Nov 26, 2019 at 11:37 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Nov 25, 2019 at 11:22 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Sun, Nov 24, 2019 at 5:06 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> > >
> > >
> > >
> > > ne 24. 11. 2019 v 11:25 odesílatel vignesh C <vignesh21@gmail.com> napsal:
> > >>
> > >> On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >> >
> > >> > On Fri, Nov 22, 2019 at 3:16 PM vignesh C <vignesh21@gmail.com> 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

On Wed, Nov 27, 2019 at 10:15 AM vignesh C <vignesh21@gmail.com> 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

On Thu, Nov 28, 2019 at 8:54 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Nov 27, 2019 at 10:15 AM vignesh C <vignesh21@gmail.com> 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

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

On Fri, Nov 29, 2019 at 1:36 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
>
> On Fri, Nov 29, 2019 at 7:30 AM Michael Paquier <michael@paquier.xyz> 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&dt=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

On Fri, Nov 29, 2019 at 1:36 PM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
>
>
> On Fri, Nov 29, 2019 at 7:30 AM Michael Paquier <michael@paquier.xyz> 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&dt=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

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

On Sat, Nov 30, 2019 at 7:46 AM Michael Paquier <michael@paquier.xyz> 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