Hi
so 2. 11. 2019 v 17:18 odesílatel Pavel Stehule <[email protected]>
napsal:
>
>
> pá 25. 10. 2019 v 4:55 odesílatel Amit Kapila <[email protected]>
> napsal:
>
>> On Thu, Oct 24, 2019 at 8:22 PM Pavel Stehule <[email protected]>
>> wrote:
>> >
>> > čt 24. 10. 2019 v 11:10 odesílatel Amit Kapila <[email protected]>
>> napsal:
>> >>
>> >> While making some changes in the patch, I noticed that in
>> >> TerminateOtherDBBackends, there is a race condition where after we
>> >> release the ProcArrayLock, the backend process to which we decided to
>> >> send a signal exits by itself and the same pid can be assigned to
>> >> another backend which is connected to some other database. This leads
>> >> to terminating a wrong backend. I think we have some similar race
>> >> condition at few other places like in pg_terminate_backend(),
>> >> ProcSleep() and CountOtherDBBackends(). I think here the risk is a
>> >> bit more because there could be a long list of pids.
>> >>
>> >> One idea could be that we write a new function similar to IsBackendPid
>> >> which takes dbid and ensures that pid belongs to that database and use
>> >> that before sending kill signal, but still it will not be completely
>> >> safe. But, I think it can be closer to cases like we already have in
>> >> code.
>> >>
>> >> Another possible idea could be to use the SendProcSignal mechanism
>> >> similar to how we have used it in CancelDBBackends() to allow the
>> >> required backends to exit by themselves. This might be safer.
>> >>
>> >> I am not sure if we can entirely eliminate this race condition and
>> >> whether it is a good idea to accept such a race condition even though
>> >> it exists in other parts of code. What do you think?
>> >>
>> >> BTW, I have added/revised some comments in the code and done few other
>> >> cosmetic changes, the result of which is attached.
>> >
>> >
>> > Tomorrow I'll check variants that you mentioned.
>> >
>> > We sure so there are not any new connect to removed database, because
>> we hold lock there.
>> >
>>
>> Right.
>>
>> > So check if oid db is same should be enough.
>> >
>>
>> We can do this before sending a kill signal but is it enough? Because
>> as soon as we release ProcArrayLock anytime the other process can exit
>> and a new process can use its pid. I think this is more of a
>> theoretical risk and might not be easy to hit, but still, we can't
>> ignore it.
>>
>
> yes, there is a theoretical risk probably - the released pid should near
> current fresh pid from range 0 .. pid_max.
>
> Probably the best solutions is enhancing SendProcSignal and using it here
> and fix CountOtherDBBackends.
>
> Alternative solution can be killing in block 50 processes and recheck.
> I'll try to write both and you can decide for one
>
I am sending patch where kill was replaced by SendProcSignal. I tested it
on pg_bench with 400 connections, and it works without problems.
Regards
Pavel
>
> Pavel
>
>
>> --
>> With Regards,
>> Amit Kapila.
>> EnterpriseDB: http://www.enterprisedb.com
>>
>
diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml
index 3ac06c984a..cef1b90421 100644
--- a/doc/src/sgml/ref/drop_database.sgml
+++ b/doc/src/sgml/ref/drop_database.sgml
@@ -21,7 +21,11 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
-DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
+DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> [ [ WITH ] ( <replaceable class="parameter">option</replaceable> [, ...] ) ]
+
+<phrase>where <replaceable class="parameter">option</replaceable> can be:</phrase>
+
+ FORCE
</synopsis>
</refsynopsisdiv>
@@ -32,9 +36,11 @@ DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
<command>DROP DATABASE</command> drops a database. It removes the
catalog entries for the database and deletes the directory
containing the data. It can only be executed by the database owner.
- Also, it cannot be executed while you or anyone else are connected
- to the target database. (Connect to <literal>postgres</literal> or any
- other database to issue this command.)
+ It cannot be executed while you are connected to the target database.
+ (Connect to <literal>postgres</literal> or any other database to issue this
+ command.)
+ Also, if anyone else is connected to the target database, this command will
+ fail unless you use the <literal>FORCE</literal> option described below.
</para>
<para>
@@ -64,6 +70,25 @@ DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
</para>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term><literal>FORCE</literal></term>
+ <listitem>
+ <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>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</refsect1>
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 01d66212e9..38a2bfa969 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -811,7 +811,7 @@ createdb_failure_callback(int code, Datum arg)
* DROP DATABASE
*/
void
-dropdb(const char *dbname, bool missing_ok)
+dropdb(const char *dbname, bool missing_ok, bool force)
{
Oid db_id;
bool db_istemplate;
@@ -896,6 +896,13 @@ dropdb(const char *dbname, bool missing_ok)
nslots_active, nslots_active)));
}
+ /*
+ * Attempt to terminate all existing connections to the target database if
+ * the user has requested to do so.
+ */
+ if (force)
+ TerminateOtherDBBackends(db_id);
+
/*
* Check for other backends in the target database. (Because we hold the
* database lock, no new ones can start after this.)
@@ -1003,6 +1010,30 @@ dropdb(const char *dbname, bool missing_ok)
ForceSyncCommit();
}
+/*
+ * Process options and call dropdb function.
+ */
+void
+DropDatabase(ParseState *pstate, DropdbStmt *stmt)
+{
+ bool force = false;
+ ListCell *lc;
+
+ foreach(lc, stmt->options)
+ {
+ DefElem *opt = (DefElem *) lfirst(lc);
+
+ if (strcmp(opt->defname, "force") == 0)
+ force = true;
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("unrecognized DROP DATABASE option \"%s\"", opt->defname),
+ parser_errposition(pstate, opt->location)));
+ }
+
+ dropdb(stmt->dbname, stmt->missing_ok, force);
+}
/*
* Rename database
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 3432bb921d..2f267e4bb6 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3868,6 +3868,7 @@ _copyDropdbStmt(const DropdbStmt *from)
COPY_STRING_FIELD(dbname);
COPY_SCALAR_FIELD(missing_ok);
+ COPY_NODE_FIELD(options);
return newnode;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 18cb014373..da0e1d139a 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1676,6 +1676,7 @@ _equalDropdbStmt(const DropdbStmt *a, const DropdbStmt *b)
{
COMPARE_STRING_FIELD(dbname);
COMPARE_SCALAR_FIELD(missing_ok);
+ COMPARE_NODE_FIELD(options);
return true;
}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 3f67aaf30e..2f7bd662e8 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -310,6 +310,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <defelt> vac_analyze_option_elem
%type <list> vac_analyze_option_list
%type <node> vac_analyze_option_arg
+%type <defelt> drop_option
%type <boolean> opt_or_replace
opt_grant_grant_option opt_grant_admin_option
opt_nowait opt_if_exists opt_with_data
@@ -406,6 +407,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
TriggerTransitions TriggerReferencing
publication_name_list
vacuum_relation_list opt_vacuum_relation_list
+ drop_option_list
%type <list> group_by_list
%type <node> group_by_item empty_grouping_set rollup_clause cube_clause
@@ -10213,7 +10215,7 @@ AlterDatabaseSetStmt:
/*****************************************************************************
*
- * DROP DATABASE [ IF EXISTS ]
+ * DROP DATABASE [ IF EXISTS ] dbname [ [ WITH ] ( options ) ]
*
* This is implicitly CASCADE, no need for drop behavior
*****************************************************************************/
@@ -10223,6 +10225,7 @@ DropdbStmt: DROP DATABASE database_name
DropdbStmt *n = makeNode(DropdbStmt);
n->dbname = $3;
n->missing_ok = false;
+ n->options = NULL;
$$ = (Node *)n;
}
| DROP DATABASE IF_P EXISTS database_name
@@ -10230,10 +10233,48 @@ DropdbStmt: DROP DATABASE database_name
DropdbStmt *n = makeNode(DropdbStmt);
n->dbname = $5;
n->missing_ok = true;
+ n->options = NULL;
$$ = (Node *)n;
}
+ | DROP DATABASE database_name opt_with '(' drop_option_list ')'
+ {
+ DropdbStmt *n = makeNode(DropdbStmt);
+ n->dbname = $3;
+ n->missing_ok = false;
+ n->options = $6;
+ $$ = (Node *)n;
+ }
+ | DROP DATABASE IF_P EXISTS database_name opt_with '(' drop_option_list ')'
+ {
+ DropdbStmt *n = makeNode(DropdbStmt);
+ n->dbname = $5;
+ n->missing_ok = true;
+ n->options = $8;
+ $$ = (Node *)n;
+ }
+ ;
+
+drop_option_list:
+ drop_option
+ {
+ $$ = list_make1((Node *) $1);
+ }
+ | drop_option_list ',' drop_option
+ {
+ $$ = lappend($1, (Node *) $3);
+ }
;
+/*
+ * Currently only the FORCE option is supported, but the syntax is designed
+ * to be extensible so that we can add more options in the future if required.
+ */
+drop_option:
+ FORCE
+ {
+ $$ = makeDefElem("force", NULL, @1);
+ }
+ ;
/*****************************************************************************
*
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 3da53074b1..aad15591ad 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -52,6 +52,8 @@
#include "access/xact.h"
#include "access/xlog.h"
#include "catalog/catalog.h"
+#include "catalog/pg_authid.h"
+#include "commands/dbcommands.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "storage/proc.h"
@@ -2970,6 +2972,101 @@ CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared)
return true; /* timed out, still conflicts */
}
+/*
+ * Terminate existing connections to the specified database. This routine
+ * is used by the DROP DATABASE command when user has asked to forcefully
+ * drop the database.
+ *
+ * The current backend is always ignored; it is caller's responsibility to
+ * check whether the current backend uses the given DB, if it's important.
+ *
+ * It doesn't allow to terminate the connections even if there is a one
+ * backend with the prepared transaction in the target database.
+ */
+void
+TerminateOtherDBBackends(Oid databaseId)
+{
+ ProcArrayStruct *arrayP = procArray;
+ int nprepared = 0;
+ int nothers = 0;
+ int i;
+
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
+
+ /* count number of prepared transactions */
+ for (i = 0; i < procArray->numProcs; i++)
+ {
+ int pgprocno = arrayP->pgprocnos[i];
+ PGPROC *proc = &allProcs[pgprocno];
+
+ if (proc->databaseId != databaseId)
+ continue;
+ if (proc == MyProc)
+ continue;
+
+ if (proc->pid != 0)
+ {
+ /*
+ * Check whether we have the necessary rights to terminate other
+ * sessions. We don't terminate any session untill we ensure that we
+ * have rights on all the sessions to be terminated. These checks are
+ * the same as we do in pg_terminate_backend.
+ *
+ * Only allow superusers to signal superuser-owned backends.
+ */
+ if (superuser_arg(proc->roleId) && !superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be a superuser to terminate superuser process"))));
+
+ /* Users can signal backends they have role membership in. */
+ if (!has_privs_of_role(GetUserId(), proc->roleId) &&
+ !has_privs_of_role(GetUserId(), DEFAULT_ROLE_SIGNAL_BACKENDID))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be a member of the role whose process is being terminated or member of pg_signal_backend"))));
+
+ nothers++;
+ }
+ else
+ nprepared++;
+ }
+
+ if (nprepared > 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_IN_USE),
+ errmsg("database \"%s\" is being used by prepared transaction",
+ get_database_name(databaseId)),
+ errdetail_plural("There is %d prepared transaction using the database.",
+ "There are %d prepared transactions using the database.",
+ nprepared,
+ nprepared)));
+
+ /* Now at end we can safely send SIGTERM */
+ if (nothers > 0)
+ {
+ for (i = 0; i < procArray->numProcs; i++)
+ {
+ int pgprocno = arrayP->pgprocnos[i];
+ PGPROC *proc = &allProcs[pgprocno];
+
+ if (proc->databaseId != databaseId)
+ continue;
+ if (proc == MyProc)
+ continue;
+
+ if (proc->pid != 0)
+ {
+ SendProcSignal(proc->pid,
+ PROCSIG_SIGTERM_INTERRUPT,
+ proc->backendId);
+ }
+ }
+ }
+
+ LWLockRelease(ProcArrayLock);
+}
+
/*
* ProcArraySetReplicationSlotXmin
*
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 7605b2c367..1b74d7ea60 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -292,6 +292,9 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN))
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
+ if (CheckProcSignal(PROCSIG_SIGTERM_INTERRUPT))
+ HandleSigtermInterrupt();
+
SetLatch(MyLatch);
latch_sigusr1_handler();
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 4bec40aa28..42263797ef 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2789,6 +2789,37 @@ die(SIGNAL_ARGS)
errno = save_errno;
}
+/*
+ * Shutdown signal from postmaster: abort transaction and exit
+ * at soonest convenient time
+ */
+void
+HandleSigtermInterrupt(void)
+{
+ int save_errno = errno;
+
+ /* Don't joggle the elbow of proc_exit */
+ if (!proc_exit_inprogress)
+ {
+ InterruptPending = true;
+ ProcDiePending = true;
+ }
+
+ /* If we're still here, waken anything waiting on the process latch */
+ SetLatch(MyLatch);
+
+ /*
+ * If we're in single user mode, we want to quit immediately - we can't
+ * rely on latches as they wouldn't work when stdin/stdout is a file.
+ * Rather ugly, but it's unlikely to be worthwhile to invest much more
+ * effort just for the benefit of single user mode.
+ */
+ if (DoingCommandRead && whereToSendOutput != DestRemote)
+ ProcessInterrupts();
+
+ errno = save_errno;
+}
+
/*
* Query-cancel signal from postmaster: abort current transaction
* at soonest convenient time
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index f2269ad35c..f3c01b4e3e 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -596,13 +596,9 @@ standard_ProcessUtility(PlannedStmt *pstmt,
break;
case T_DropdbStmt:
- {
- DropdbStmt *stmt = (DropdbStmt *) parsetree;
-
- /* no event triggers for global objects */
- PreventInTransactionBlock(isTopLevel, "DROP DATABASE");
- dropdb(stmt->dbname, stmt->missing_ok);
- }
+ /* no event triggers for global objects */
+ PreventInTransactionBlock(isTopLevel, "DROP DATABASE");
+ DropDatabase(pstate, (DropdbStmt *) parsetree);
break;
/* Query-level asynchronous notification */
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 2b1e3cda4a..3e2d9813d8 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2844,6 +2844,11 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_FUNCTION_ARG(prev2_wd);
else if (Matches("DROP", "FOREIGN"))
COMPLETE_WITH("DATA WRAPPER", "TABLE");
+ else if (Matches("DROP", "DATABASE", MatchAny))
+ COMPLETE_WITH("WITH (");
+ else if (HeadMatches("DROP", "DATABASE") &&
+ (ends_with(prev_wd, '(') || ends_with(prev_wd, ',')))
+ COMPLETE_WITH("FORCE");
/* DROP INDEX */
else if (Matches("DROP", "INDEX"))
diff --git a/src/include/commands/dbcommands.h b/src/include/commands/dbcommands.h
index 154c8157ee..d1e91a2455 100644
--- a/src/include/commands/dbcommands.h
+++ b/src/include/commands/dbcommands.h
@@ -20,7 +20,8 @@
#include "nodes/parsenodes.h"
extern Oid createdb(ParseState *pstate, const CreatedbStmt *stmt);
-extern void dropdb(const char *dbname, bool missing_ok);
+extern void dropdb(const char *dbname, bool missing_ok, bool force);
+extern void DropDatabase(ParseState *pstate, DropdbStmt *stmt);
extern ObjectAddress RenameDatabase(const char *oldname, const char *newname);
extern Oid AlterDatabase(ParseState *pstate, AlterDatabaseStmt *stmt, bool isTopLevel);
extern Oid AlterDatabaseSet(AlterDatabaseSetStmt *stmt);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index d93a79a554..ff626cbe61 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3145,6 +3145,7 @@ typedef struct DropdbStmt
NodeTag type;
char *dbname; /* database to drop */
bool missing_ok; /* skip error if db is missing? */
+ List *options; /* currently only FORCE is supported */
} DropdbStmt;
/* ----------------------
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index da8b672096..8f67b860e7 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -113,6 +113,7 @@ extern void CancelDBBackends(Oid databaseid, ProcSignalReason sigmode, bool conf
extern int CountUserBackends(Oid roleid);
extern bool CountOtherDBBackends(Oid databaseId,
int *nbackends, int *nprepared);
+extern void TerminateOtherDBBackends(Oid databaseId);
extern void XidCacheRemoveRunningXids(TransactionId xid,
int nxids, const TransactionId *xids,
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index 05b186a05c..c4e4574f5a 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -33,6 +33,7 @@ typedef enum
PROCSIG_NOTIFY_INTERRUPT, /* listen/notify interrupt */
PROCSIG_PARALLEL_MESSAGE, /* message from cooperating parallel backend */
PROCSIG_WALSND_INIT_STOPPING, /* ask walsenders to prepare for shutdown */
+ PROCSIG_SIGTERM_INTERRUPT, /* sigterm interrupt */
/* Recovery conflict reasons */
PROCSIG_RECOVERY_CONFLICT_DATABASE,
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index ec21f7e45c..967422f537 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -62,6 +62,7 @@ extern void assign_max_stack_depth(int newval, void *extra);
extern void die(SIGNAL_ARGS);
extern void quickdie(SIGNAL_ARGS) pg_attribute_noreturn();
+extern void HandleSigtermInterrupt(void);
extern void StatementCancelHandler(SIGNAL_ARGS);
extern void FloatExceptionHandler(SIGNAL_ARGS) pg_attribute_noreturn();
extern void RecoveryConflictInterrupt(ProcSignalReason reason); /* called from SIGUSR1
diff --git a/src/test/regress/expected/drop_if_exists.out b/src/test/regress/expected/drop_if_exists.out
index 6a17467717..eba0f3f8c7 100644
--- a/src/test/regress/expected/drop_if_exists.out
+++ b/src/test/regress/expected/drop_if_exists.out
@@ -330,3 +330,14 @@ HINT: Specify the argument list to select the routine unambiguously.
-- cleanup
DROP PROCEDURE test_ambiguous_procname(int);
DROP PROCEDURE test_ambiguous_procname(text);
+--
+-- DROP DATABASE FORCE test of syntax (should not to raise syntax error)
+--
+drop database not_exists (force);
+ERROR: database "not_exists" does not exist
+drop database not_exists with (force);
+ERROR: database "not_exists" does not exist
+drop database if exists not_exists (force);
+NOTICE: database "not_exists" does not exist, skipping
+drop database if exists not_exists with (force);
+NOTICE: database "not_exists" does not exist, skipping
diff --git a/src/test/regress/sql/drop_if_exists.sql b/src/test/regress/sql/drop_if_exists.sql
index 8a791b1ef2..03aebb0ee5 100644
--- a/src/test/regress/sql/drop_if_exists.sql
+++ b/src/test/regress/sql/drop_if_exists.sql
@@ -295,3 +295,11 @@ DROP ROUTINE IF EXISTS test_ambiguous_procname;
-- cleanup
DROP PROCEDURE test_ambiguous_procname(int);
DROP PROCEDURE test_ambiguous_procname(text);
+
+--
+-- 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);
