Re: dropdb --force

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