From: Andres Freund <af@cybertec.de>
To: pgsql-hackers@postgresql.org
Subject: Re: Small Bug in GetConflictingVirtualXIDs
Date: Sun, 27 Dec 2009 20:11:26 +0100
User-Agent: KMail/1.13.0 (Linux/2.6.33-rc2-andres-dirty; KDE/4.3.85; x86_64; ;
	)
Cc: Simon Riggs <simon@2ndquadrant.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Alvaro Herrera <alvherre@commandprompt.com>
References: <200912210402.37850.andres@anarazel.de>
	<200912220319.10264.af@cybertec.de>
	<1261478551.7442.4332.camel@ebony>
In-Reply-To: <1261478551.7442.4332.camel@ebony>
MIME-Version: 1.0
Content-Type: Multipart/Mixed;
  boundary="Boundary-00=_eF7NLwptckBTAk6"
Message-Id: <200912272011.27003.af@cybertec.de>

--Boundary-00=_eF7NLwptckBTAk6
Content-Type: Text/Plain;
  charset="iso-8859-15"
Content-Transfer-Encoding: 7bit

On Tuesday 22 December 2009 11:42:30 Simon Riggs wrote:
> On Tue, 2009-12-22 at 03:19 +0100, Andres Freund wrote:
> > On Monday 21 December 2009 16:48:52 Simon Riggs wrote:
> > > Giving the drop database a snapshot is not the answer. I expect Andres
> > > to be able to fix this with a simple patch that would not effect the
> > > case of normal running.
> >
> > Actually its less simply than I had thought at first - I don't think the
> > code ever handled that correctly.
> > I might be wrong there, my knowledge of the involved code is a bit
> > sparse... The whole conflict resolution builds on the concept of waiting
> > for an VXid, but an idle backend does not have a valid vxid. Thats
> > correct, right?
> I don't see any mileage in making Startup process wait for an idle
> session, so no real reason to wait for others either.
So here is a small patch implementing that behaviour.

Andres

--Boundary-00=_eF7NLwptckBTAk6
Content-Type: text/x-patch; charset="UTF-8";
	name="0001-Fix-the-bug-in-HS-that-it-is-possible-to-stay-connec.patch"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment;
	filename="0001-Fix-the-bug-in-HS-that-it-is-possible-to-stay-connec.patch"

=46rom 36f57e6f6b1d25d7433d9706682e53cc79b45a61 Mon Sep 17 00:00:00 2001
=46rom: Andres Freund <andres@anarazel.de>
Date: Sun, 27 Dec 2009 19:14:21 +0100
Subject: [PATCH] Fix the bug in HS that it is possible to stay connected to=
 a database
 on a slave after it has been deleted.

Instead immediately kill sessions using such a database - it is
unlikely that those sessions will quit and its unlikely as well that
its a significant issue to get thrown of a database which is dropped
on the master.
=2D--
 src/backend/commands/dbcommands.c   |   22 ++++++++--------------
 src/backend/storage/ipc/procarray.c |   25 +++++++++++++++++++++++++
 src/include/storage/procarray.h     |    1 +
 3 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcom=
mands.c
index 316222f..85d1d64 100644
=2D-- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -1945,22 +1945,16 @@ dbase_redo(XLogRecPtr lsn, XLogRecord *record)
=20
 		if (InHotStandby)
 		{
=2D			VirtualTransactionId *database_users;
=2D
 			/*
=2D			 * Find all users connected to this database and ask them
=2D			 * politely to immediately kill their sessions before processing
=2D			 * the drop database record, after the usual grace period.
=2D			 * We don't wait for commit because drop database is
=2D			 * non-transactional.
+			 * We dont do the usual GetConflictingVirtualXIDs/
+			 * ResolveRecoveryConflictWithVirutalXIDs dance here because we are
+			 * not only conflicting against concurrently running transactions
+			 * but against concurrently running backends using this database.
+			 *
+			 * We do not wait for those sessions to exit by free will because
+			 * quite likely that will never happen. So force them.
 			 */
=2D		    database_users =3D GetConflictingVirtualXIDs(InvalidTransactionId,
=2D													   xlrec->db_id,
=2D													   false);
=2D
=2D			ResolveRecoveryConflictWithVirtualXIDs(database_users,
=2D												   "drop database",
=2D												   CONFLICT_MODE_FATAL);
+			CancelDBBackends(xlrec->db_id);
 		}
=20
 		/* Drop pages for this database that are in the shared buffer cache */
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/=
procarray.c
index 8e2de35..07c7899 100644
=2D-- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1827,6 +1827,31 @@ CountDBBackends(Oid databaseid)
 }
=20
 /*
+ * CancelDBBackends --- cancel backends that are using specified database
+ */
+void
+CancelDBBackends(Oid databaseid)
+{
+	ProcArrayStruct *arrayP =3D procArray;
+	int			index;
+
+	/* tell all backends to die */
+	LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+	for (index =3D 0; index < arrayP->numProcs; index++)
+	{
+		volatile PGPROC *proc =3D arrayP->procs[index];
+
+		if (proc->databaseId =3D=3D databaseid){
+			if (proc->recoveryConflictMode < CONFLICT_MODE_FATAL){
+				proc->recoveryConflictMode =3D CONFLICT_MODE_FATAL;
+			}
+			kill(proc->pid, SIGINT);
+		}
+	}
+	LWLockRelease(ProcArrayLock);
+}
+
+/*
  * CountUserBackends --- count backends that are used by specified user
  */
 int
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarra=
y.h
index 1cee639..707396b 100644
=2D-- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -63,6 +63,7 @@ extern pid_t CancelVirtualTransaction(VirtualTransactionI=
d vxid,
=20
 extern int	CountActiveBackends(void);
 extern int	CountDBBackends(Oid databaseid);
+extern void	CancelDBBackends(Oid databaseid);
 extern int	CountUserBackends(Oid roleid);
 extern bool CountOtherDBBackends(Oid databaseId,
 					 int *nbackends, int *nprepared);
=2D-=20
1.6.5.12.gd65df24


--Boundary-00=_eF7NLwptckBTAk6--