Date: Wed, 26 Dec 2018 16:08:34 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: removal of dangling temp tables
Message-ID: <20181226190834.wsk2wzott5yzrjiq@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2g2g5wvzs6ruvdiz"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20181216065847.GJ5012@paquier.xyz>
User-Agent: NeoMutt/20180716
Precedence: bulk


--2g2g5wvzs6ruvdiz
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2018-Dec-16, Michael Paquier wrote:

> On Sat, Dec 15, 2018 at 09:51:31AM -0500, Tom Lane wrote:
> > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> >> Oh, we already have it!  Sorry, I overlooked it.  With that, it seems
> >> the patch is fairly simple ... I wonder about the locking implications
> >> in autovacuum, though -- the value is set in backends without acquiring
> >> a lock.
> > 
> > I was wondering about that too.  But I think it's probably OK.  If
> > autovacuum observes that (a) a table is old enough to pose a wraparound
> > hazard and (b) its putatively owning backend hasn't yet set
> > tempNamespaceId, then I think it's safe to conclude that that table is
> > removable, despite the theoretical race condition.
> 
> This relies on the fact that the flag gets set by a backend within a
> transaction context, and autovacuum would not see yet temp relations
> associated to it at the moment of the scan of pg_class if the backend
> has not committed yet its namespace creation via the creation of the
> first temp table it uses.

Makes sense, thanks.

I think there are two possible ways forward.  The conservative one is to
just apply the attached patch to branches 9.4-10.  That will let
autovacuum drop tables when the backend is in another database. It may
not solve the problem for the bunch of users that have only one database
that takes the majority of connections, but I think it's worth doing
nonetheless.  I tested the 9.4 instance and it works fine; tables are
deleted as soon as I make the session connection to another database.

The more aggressive action is to backpatch 943576bddcb5 ("Make
autovacuum more aggressive to remove orphaned temp tables") which is
currently only in pg11.  We would put the new PGPROC member at the end
of the struct, to avoid ABI incompatibilities, but it'd bring trouble
for extensions that put PGPROC in arrays.  I checked the code of some
known extensions; found that pglogical uses PGPROC, but only pointers to
it, so it wouldn't be damaged by the proposed change AFAICS.


Another possibly useful change is to make DISCARD ALL and DISCARD TEMP
delete everything in what would be the backend's temp namespace, even if
it hasn't been initialized yet.  This would cover the case where a
connection pooler keeps the connection open for a very long time, which
I think is a common case.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--2g2g5wvzs6ruvdiz
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="autovac-temp-REL9_4_STABLE.patch"
Content-Transfer-Encoding: quoted-printable

commit 5aa717be9fe7288cbec043cdea0fb757433f3462=1B[m
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org>
AuthorDate: Wed Dec 26 15:12:44 2018 -0300
CommitDate: Wed Dec 26 15:12:45 2018 -0300

    Improve autovacuum logic about temp tables nearing XID wraparound

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/a=
utovacuum.c
index c18285d690..62a06add56 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2108,11 +2108,18 @@ do_autovacuum(void)
 		if (classForm->relpersistence =3D=3D RELPERSISTENCE_TEMP)
 		{
 			int			backendID;
+			PGPROC	   *proc;
=20
 			backendID =3D GetTempNamespaceBackendId(classForm->relnamespace);
=20
-			/* We just ignore it if the owning backend is still active */
-			if (backendID =3D=3D MyBackendId || BackendIdGetProc(backendID) =3D=3D =
NULL)
+			/*
+			 * We just ignore it if the owning backend is still active in the
+			 * same database.
+			 */
+			if (backendID !=3D InvalidBackendId &&
+				(backendID =3D=3D MyBackendId ||
+				 (proc =3D BackendIdGetProc(backendID)) =3D=3D NULL ||
+				 proc->databaseId !=3D MyDatabaseId))
 			{
 				/*
 				 * We found an orphan temp table (which was probably left

--2g2g5wvzs6ruvdiz--