Hi so 2. 11. 2019 v 17:18 odesílatel Pavel Stehule <pavel.steh...@gmail.com> napsal:
> > > pá 25. 10. 2019 v 4:55 odesílatel Amit Kapila <amit.kapil...@gmail.com> > napsal: > >> On Thu, Oct 24, 2019 at 8:22 PM Pavel Stehule <pavel.steh...@gmail.com> >> wrote: >> > >> > čt 24. 10. 2019 v 11:10 odesílatel Amit Kapila <amit.kapil...@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 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);