Date: Thu, 14 Nov 2019 22:31:24 +0100
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Mark Dilger <hornschnorter@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Missing dependency tracking for TableFunc nodes
Message-ID: <20191114213124.i6xwx24y26iklzds@development>
References: <18427.1573508501@sss.pgh.pa.us>
 <997cbcae-7739-f871-84a0-1192380abb54@gmail.com>
 <15228.1573695479@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="nev5ied3k6kuxcrp"
Content-Disposition: inline
In-Reply-To: <15228.1573695479@sss.pgh.pa.us>
User-Agent: NeoMutt/20180716
Precedence: bulk


--nev5ied3k6kuxcrp
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline

On Wed, Nov 13, 2019 at 08:37:59PM -0500, Tom Lane wrote:
>Mark Dilger <hornschnorter@gmail.com> writes:
>> On 11/11/19 1:41 PM, Tom Lane wrote:
>>> I happened to notice that find_expr_references_walker has not
>>> been taught anything about TableFunc nodes, which means it will
>>> miss the type and collation OIDs embedded in such a node.
>
>> I can consistently generate errors like the following in master:
>>    ERROR:  cache lookup failed for statistics object 31041
>
>This is surely a completely different issue --- there are not,
>one hopes, any extended-stats OIDs embedded in views or other
>query trees.
>
>I concur with Tomas' suspicion that this must be a race condition
>during DROP STATISTICS.  If we're going to allow people to do that
>separately from dropping the table(s), there has to be some kind of
>locking around it, and it sounds like there's not :-(
>

I think the right thing to do is simply acquire AE lock on the relation
in RemoveStatisticsById, per the attached patch. It's possible we'll
need to do something more complicated once join stats are added, but
for now this should be enough (and backpatchable).

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 

--nev5ied3k6kuxcrp
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="stats-locking.patch"

diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index 0ab43deb71..1126e7a585 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -30,6 +30,7 @@
 #include "commands/defrem.h"
 #include "miscadmin.h"
 #include "statistics/statistics.h"
+#include "storage/lmgr.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
 #include "utils/inval.h"
@@ -533,6 +534,21 @@ RemoveStatisticsById(Oid statsOid)
 	Form_pg_statistic_ext statext;
 	Oid			relid;
 
+	/* Lookup and lock relation the statistics is defined on. */
+	tup = SearchSysCache1(STATEXTOID, ObjectIdGetDatum(statsOid));
+
+	if (!HeapTupleIsValid(tup))
+		elog(ERROR, "cache lookup failed for statistics object %u", statsOid);
+
+	statext = (Form_pg_statistic_ext) GETSTRUCT(tup);
+	relid = statext->stxrelid;
+
+	/* lock the relation in access exclusive mode */
+	LockRelationOid(relid, AccessExclusiveLock);
+
+	ReleaseSysCache(tup);
+
+
 	/*
 	 * First delete the pg_statistic_ext_data tuple holding the actual
 	 * statistical data.

--nev5ied3k6kuxcrp--