st 2. 10. 2019 v 5:20 odesílatel Amit Kapila <[email protected]>
napsal:
> On Thu, Sep 26, 2019 at 7:18 PM Pavel Stehule <[email protected]>
> wrote:
>
>>
>> út 24. 9. 2019 v 14:52 odesílatel Amit Kapila <[email protected]>
>> napsal:
>>
>>>
>>> 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?
>>>
>>
>> Yes, is there.
>>
>> The force flag ensure sending SIGTERM to related clients. Nothing more.
>> There are still check
>>
>> -->if (CountOtherDBBackends(db_id, ¬herbackends, &npreparedxacts))
>> <--><-->ereport(ERROR,
>> <--><--><--><-->(errcode(ERRCODE_OBJECT_IN_USE),
>> <--><--><--><--> errmsg("database \"%s\" is being accessed by other
>> users",
>> <--><--><--><--><--><-->dbname),
>> <--><--><--><--> errdetail_busy_db(notherbackends, npreparedxacts)));
>>
>> that can fails after 5 sec. Sending signal doesn't ensure nothing, so I
>> am for no changes in these check.
>>
>
> I think 5 seconds is a hard-coded value that can even change in the
> future. So, it is better to write something more generic like "This will
> also fail if we are not able to terminate connections" or something like
> that. This part of the documentation might change after addressing the
> other comments below.
>
done
> One more point I would like to add here is that I think it is worth
>> considering to split this patch by keeping the changes in dropdb
>> utility as a separate patch. Even though the code is not very much
>> but I think it can be a separate patch atop the main patch which
>> contains the core server changes.
>>
>
> > I did it - last patch contains server side only. I expect so client side
> (very small patch) can be nex
>
> I still see the code related to dropdb utility in the patch. See,
> diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
> index dacd8e5f1d..1bb80fda74 100644
> --- a/src/bin/scripts/dropdb.c
> +++ b/src/bin/scripts/dropdb.c
> @@ -34,6 +34,7 @@ main(int argc, char *argv[])
> {"interactive", no_argument, NULL, 'i'},
> {"if-exists", no_argument, &if_exists, 1},
> {"maintenance-db", required_argument, NULL, 2},
> + {"force", no_argument, NULL, 'f'},
> {NULL, 0, NULL, 0}
> };
>
removed
> Few other comments:
> --------------------------------
> 1.
> +void
> +TerminateOtherDBBackends(Oid databaseId)
> +{
> + ProcArrayStruct *arrayP = procArray;
> + List *pids = NIL;
> + int i;
> +
> + LWLockAcquire(ProcArrayLock, LW_SHARED);
> +
> + for (i = 0; i < procArray->numProcs; i++)
> + {
> + int pgprocno = arrayP->pgprocnos[i];
> + PGPROC *proc = &allProcs[pgprocno];
> +
> + if (proc->databaseId != databaseId)
> + continue;
> + if (proc == MyProc)
> + continue;
> +
> + if (proc->pid != 0)
> + pids = lappend_int(pids, proc->pid);
> + }
>
> So here we are terminating only connection which doesn't have any prepared
> transaction. The behavior of this patch with the prepared transactions
> will be terrible. Basically, after terminating all the connections via
> TerminateOtherDBBackends, we will give error once CountOtherDBBackends is
> invoked. I have tested the same and it gives an error like below:
>
> postgres=# drop database db1 With (Force);
> ERROR: database "db1" is being accessed by other users
> DETAIL: There is 1 prepared transaction using the database.
>
> I think we have two options here:
> (a) Document that even with force option, if there are any prepared
> transactions in the same database, we won't drop the database. Along with
> this, fix the code such that we don't terminate other connections if the
> prepared transactions are active.
> (b) Rollback the prepared transactions.
>
I not use prepared transactions often, and then I have not own strong
opinion about it. Original parch didn't touch this area, so I think we can
continue in this direction (minimally for start).
I did precheck of opened prepared transactions, and when I find any opened,
then I raise a exception (before when I try to terminate other processes).
I updated doc about possible stops.
> I think (a) is a better option because if the number of prepared
> transactions is large, then it might take time and I am not sure if it is
> worth to add the complexity of rolling back all the prepared xacts. OTOH,
> if you or others think option (b) is good and doesn't add much complexity,
> then I think it is worth exploring that option.
>
> I think we should definitely do something to deal with this even if you
> don't like the proposed options because the current behavior of the patch
> seems worse than either of these options.
>
> 2.
> -DROP DATABASE [ IF EXISTS ] <replaceable
> class="parameter">name</replaceable>
> +DROP DATABASE [ IF EXISTS ] <replaceable
> class="parameter">name</replaceable> [ WITH ( <replaceable
> class="parameter">option</replaceable> [, ...] ) ]
>
> It is better to keep WITH clause as optional similar to Copy command.
> This might address Peter E's concern related to WITH clause.
>
WITH keyword is optional now
>
> 3.
> - * DROP DATABASE [ IF EXISTS ]
> + * DROP DATABASE [ ( options ) ] [ IF EXISTS ]
>
fixed
> You seem to forget to change the syntax in the above comments after
> changing the patch.
>
> 4.
> + If anyone else is connected to the target database, this command will
> fail
> + - unless you use the <literal>FORCE</literal> option described below.
>
> I don't understand the significance of using '-' before unless. I think
> we can remove it.
>
fixed
> Changed the patch status as 'Waiting on Author'.
>
Thank you for careful review. I hope so all issues are out.
Regards
Pavel
>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml
index 3ac06c984a..11d0be0eb5 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 one of:</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.)
+ Also, 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.)
+ 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,28 @@ 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 prepared transactions or active logical replication
+ slot(s).
+ </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 we are not able to terminate connections or
+ when there are active prepared transactions or active logical replication
+ slots.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</refsect1>
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 01d66212e9..8e62359b4d 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -811,7 +811,7 @@ createdb_failure_callback(int code, Datum arg)
* DROP DATABASE
*/
void
-dropdb(const char *dbname, bool missing_ok)
+dropdb(const char *dbname, bool missing_ok, bool force)
{
Oid db_id;
bool db_istemplate;
@@ -896,6 +896,9 @@ dropdb(const char *dbname, bool missing_ok)
nslots_active, nslots_active)));
}
+ if (force)
+ TerminateOtherDBBackends(db_id);
+
/*
* Check for other backends in the target database. (Because we hold the
* database lock, no new ones can start after this.)
@@ -1003,6 +1006,30 @@ dropdb(const char *dbname, bool missing_ok)
ForceSyncCommit();
}
+/*
+ * Process options and call dropdb function.
+ */
+void
+DropDatabase(ParseState *pstate, DropdbStmt *stmt)
+{
+ bool force = false;
+ ListCell *lc;
+
+ foreach(lc, stmt->options)
+ {
+ DefElem *opt = (DefElem *) lfirst(lc);
+
+ if (strcmp(opt->defname, "force") == 0)
+ force = true;
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("unrecognized DROP DATABASE option \"%s\"", opt->defname),
+ parser_errposition(pstate, opt->location)));
+ }
+
+ dropdb(stmt->dbname, 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..25f4d7fdd6 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 opt_drop_option_list
%type <list> group_by_list
%type <node> group_by_item empty_grouping_set rollup_clause cube_clause
@@ -10213,27 +10215,54 @@ AlterDatabaseSetStmt:
/*****************************************************************************
*
- * DROP DATABASE [ IF EXISTS ]
+ * DROP DATABASE [ IF EXISTS ] [ [ WITH ] ( options ) ]
*
* This is implicitly CASCADE, no need for drop behavior
*****************************************************************************/
-DropdbStmt: DROP DATABASE database_name
+DropdbStmt: DROP DATABASE database_name opt_drop_option_list
{
DropdbStmt *n = makeNode(DropdbStmt);
n->dbname = $3;
n->missing_ok = false;
+ n->options = $4;
$$ = (Node *)n;
}
- | DROP DATABASE IF_P EXISTS database_name
+ | DROP DATABASE IF_P EXISTS database_name opt_drop_option_list
{
DropdbStmt *n = makeNode(DropdbStmt);
n->dbname = $5;
n->missing_ok = true;
+ n->options = $6;
$$ = (Node *)n;
}
;
+opt_drop_option_list:
+ opt_with '(' drop_option_list ')' { $$ = $3; }
+ | /* EMPTY */ { $$ = NIL; }
+ ;
+
+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 syntax is designed
+ * to be extensible, and then we use same patterns like on other places.
+ */
+drop_option: FORCE
+ {
+ $$ = makeDefElem("force", NULL, @1);
+ }
+ ;
/*****************************************************************************
*
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 8abcfdf841..16a4cd13f0 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,102 @@ CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared)
return true; /* timed out, still conflicts */
}
+/*
+ * Terminate other db connections. This routine is used by
+ * DROP DATABASE FORCE to eliminate all others clients from
+ * database.
+ */
+void
+TerminateOtherDBBackends(Oid databaseId)
+{
+ ProcArrayStruct *arrayP = procArray;
+ List *pids = NIL;
+ int nprepared = 0;
+ int i;
+
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
+
+ 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)
+ pids = lappend_int(pids, proc->pid);
+ else
+ nprepared++;
+ }
+
+ LWLockRelease(ProcArrayLock);
+
+ 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)));
+
+ if (pids)
+ {
+ ListCell *lc;
+
+ /*
+ * Similar code to pg_terminate_backend, but we check rights first
+ * here, and only when we have all necessary rights we start to
+ * terminate other clients. In this case we should not raise
+ * some warnings - like "PID %d is not a PostgreSQL server process",
+ * because for this purpose - already finished session is not
+ * problem.
+ */
+ foreach (lc, pids)
+ {
+ int pid = lfirst_int(lc);
+ PGPROC *proc = BackendPidGetProc(pid);
+
+ if (proc != NULL)
+ {
+ /* 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"))));
+ }
+ }
+
+ /* 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);
+ }
+}
+
/*
* ProcArraySetReplicationSlotXmin
*
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index c6faa6619d..960b0df0e1 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/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,
