Fix relcache callback handling causing crashes

Fix a crash that could corrupt indexes when running VACUUM FULL
pg_class.

The crash happens when caches are queried/updated within a cache
invalidation function, which can lead to corruption and recursive
cache invalidations.

To solve the issue, make sure the relcache invalidation callback is
simple and never invokes the relcache or syscache directly or
indirectly.

Some background: The extension is preloaded and thus have planner
hooks installed irrespective of whether the extension is actually
installed or not in the current database. However, the hooks need to
be disabled as long as the extension is not installed. To avoid always
having to dynamically check for the presence of the extension, the
state is cached in the session.

However, the cached state needs to be updated if the extension changes
(altered/dropped/created). Therefore, the relcache invalidation
callback mechanism is (ab)used in TimescaleDB to signal updates to the
extension state across all active backends.

The signaling is implemented by installing a dummy table as part of
the extension and any invalidation on the relid for that table signals
a change in the extension state. However, as of this change, the
actual state is no longer determined in the callback itself, since it
requires use of the relcache and causes the bad behavior. Therefore,
the only thing that remains in the callback after this change is to
reset the extension state.

The actual state is instead resolved on-demand, but can still be
cached when the extension is in the installed state and the dummy
table is present with a known relid. However, if the extension is not
installed, the extension state can no longer be cached as there is no
way to signal other backends that the state should be reset when they
don't know the dummy table's relid, and cannot resolve it from within
the callback itself.

Fixes #3924

src/cache_invalidate.c

@@ -20,6 +20,7 @@
 
 #include "bgw/scheduler.h"
 #include "cross_module_fn.h"
+#include "cache_invalidate.h"
 
 /*
  * Notes on the way cache invalidation works.
@@ -59,45 +60,44 @@ cache_invalidate_relcache_all(void)
 	ts_bgw_job_cache_invalidate_callback();
 }
 
+static Oid hypertable_proxy_table_oid = InvalidOid;
+static Oid bgw_proxy_table_oid = InvalidOid;
+
+void
+ts_cache_invalidate_set_proxy_tables(Oid hypertable_proxy_oid, Oid bgw_proxy_oid)
+{
+	hypertable_proxy_table_oid = hypertable_proxy_oid;
+	bgw_proxy_table_oid = bgw_proxy_oid;
+}
+
 /*
  * This function is called when any relcache is invalidated.
  * Should route the invalidation to the correct cache.
+ *
+ * NOTE that the callback should not call any functions that could invoke the
+ * relcache or syscache to query information during the invalidation. That
+ * might lead to bad things happening.
  */
 static void
-cache_invalidate_callback(Datum arg, Oid relid)
+cache_invalidate_relcache_callback(Datum arg, Oid relid)
 {
-	static bool in_recursion = false;
-
-	if (ts_extension_invalidate(relid))
+	if (relid == InvalidOid)
 	{
 		cache_invalidate_relcache_all();
-		return;
 	}
-
-	if (!ts_extension_is_loaded())
-		return;
-
-	/* The cache invalidation can be called indirectly further down in the
-	 * call chain by calling `get_namespace_oid`, which can trigger a
-	 * recursive cache invalidation callback. To prevent infinite recursion,
-	 * we stop it here and instead consider the cache as invalidated, which
-	 * will allow the `get_namespace_oid` to retrieve the OID from the heap
-	 * table and return it properly. */
-	if (!in_recursion)
+	else if (ts_extension_is_proxy_table_relid(relid))
 	{
-		Catalog *catalog;
-		in_recursion = true;
-		catalog = ts_catalog_get();
-		in_recursion = false;
-
-		if (relid == ts_catalog_get_cache_proxy_id(catalog, CACHE_TYPE_HYPERTABLE))
-			ts_hypertable_cache_invalidate_callback();
-
-		if (relid == ts_catalog_get_cache_proxy_id(catalog, CACHE_TYPE_BGW_JOB))
-			ts_bgw_job_cache_invalidate_callback();
-
-		if (relid == InvalidOid)
-			cache_invalidate_relcache_all();
+		ts_extension_invalidate();
+		cache_invalidate_relcache_all();
+		ts_cache_invalidate_set_proxy_tables(InvalidOid, InvalidOid);
+	}
+	else if (relid == hypertable_proxy_table_oid)
+	{
+		ts_hypertable_cache_invalidate_callback();
+	}
+	else if (relid == bgw_proxy_table_oid)
+	{
+		ts_bgw_job_cache_invalidate_callback();
 	}
 }
 
@@ -182,7 +182,7 @@ _cache_invalidate_init(void)
 {
 	RegisterXactCallback(cache_invalidate_xact_end, NULL);
 	RegisterSubXactCallback(cache_invalidate_subxact_end, NULL);
-	CacheRegisterRelcacheCallback(cache_invalidate_callback, PointerGetDatum(NULL));
+	CacheRegisterRelcacheCallback(cache_invalidate_relcache_callback, PointerGetDatum(NULL));
 
 	/* Specific syscache callbacks */
 	CacheRegisterSyscacheCallback(FOREIGNSERVEROID,

src/extension.c

@@ -8,8 +8,10 @@
 #include <access/transam.h>
 #include <commands/event_trigger.h>
 #include <catalog/namespace.h>
+#include <catalog/objectaccess.h>
 #include <utils/lsyscache.h>
 #include <utils/inval.h>
+#include <fmgr.h>
 
 #include "compat/compat-msvc-enter.h" /* To label externs in extension.h and
 								 * miscadmin.h correctly */
@@ -62,7 +64,20 @@ static enum ExtensionState extstate = EXTENSION_STATE_UNKNOWN;
  * often need it during the planning, so we cache it here. We update it when
  * the extension status is updated.
  */
-Oid ts_extension_oid = InvalidOid;
+static Oid ts_extension_oid = InvalidOid;
+
+Oid
+ts_extension_get_oid(void)
+{
+	if (OidIsValid(ts_extension_oid))
+	{
+		Assert(ts_extension_oid == get_extension_oid(EXTENSION_NAME, true));
+		return ts_extension_oid;
+	}
+
+	ts_extension_oid = get_extension_oid(EXTENSION_NAME, false);
+	return ts_extension_oid;
+}
 
 static bool
 extension_loader_present()
@@ -139,8 +154,7 @@ extension_set_state(enum ExtensionState newstate)
 			break;
 		case EXTENSION_STATE_CREATED:
 			ts_extension_check_version(TIMESCALEDB_VERSION_MOD);
-			extension_proxy_oid = get_relname_relid(EXTENSION_PROXY_TABLE,
-													get_namespace_oid(CACHE_SCHEMA_NAME, false));
+			extension_proxy_oid = get_proxy_table_relid();
 			ts_catalog_reset();
 			break;
 		case EXTENSION_STATE_NOT_INSTALLED:
@@ -156,16 +170,23 @@ extension_set_state(enum ExtensionState newstate)
 static void
 extension_update_state()
 {
-	static bool in_recursion = false;
-	/* Since the state of the extension is determined by the snapshot of the transaction there
-	 * is no point processing recursive calls as the outer call will always set the correct state.
-	 * This also prevents deep recursion during `AcceptInvalidationMessages`.
+	enum ExtensionState new_state = extension_current_state();
+
+	/* Never actually set the state to "not installed" since there is no good
+	 * way to get out of it in case the extension is installed again in
+	 * another backend. After the extension has been dropped, the proxy table
+	 * no longer exists and when the extension is reinstalled, the proxy table
+	 * will have a different relid. Therefore, there is no way to identify the
+	 * invalidation on the proxy table when CREATE EXTENSION is issued in
+	 * another backend. Nor is it allowed to lookup the new relid in the
+	 * invalidation callback, since that may lead to bad behavior.
+	 *
+	 * Instead, set the state to "unknown" so that a "slow path" lookup of the
+	 * actual state has to be made next time the state is queried.
 	 */
-	if (in_recursion)
-		return;
+	if (new_state == EXTENSION_STATE_NOT_INSTALLED)
+		new_state = EXTENSION_STATE_UNKNOWN;
 
-	in_recursion = true;
-	enum ExtensionState new_state = extension_current_state();
 	extension_set_state(new_state);
 	/*
 	 * Update the extension oid. Note that it is only safe to run
@@ -182,7 +203,6 @@ extension_update_state()
 	{
 		ts_extension_oid = InvalidOid;
 	}
-	in_recursion = false;
 }
 
 Oid
@@ -239,49 +259,21 @@ ts_experimental_schema_name(void)
 }
 
 /*
- *	Called upon all Relcache invalidate events.
- *	Returns whether or not to invalidate the entire extension.
+ * Invalidate the state of the extension (i.e., whether the extension is
+ * installed or not in the current database).
+ *
+ * Since this function is called from a relcache invalidation callback, it
+ * must not, directly or indirectly, call functions that use the cache. This
+ * includes, e.g., table scans.
+ *
+ * Instead, the function just invalidates the state so that the true state is
+ * resolved lazily when needed.
  */
-bool
-ts_extension_invalidate(Oid relid)
+void
+ts_extension_invalidate(void)
 {
-	bool invalidate_all = false;
-
-	switch (extstate)
-	{
-		case EXTENSION_STATE_NOT_INSTALLED:
-			/* This event may mean we just added the proxy table */
-		case EXTENSION_STATE_UNKNOWN:
-			/* Can we recompute the state now? */
-		case EXTENSION_STATE_TRANSITIONING:
-			/* Has the create/drop extension finished? */
-			extension_update_state();
-			break;
-		case EXTENSION_STATE_CREATED:
-
-			/*
-			 * Here we know the proxy table oid so only listen to potential
-			 * drops on that oid. Note that an invalid oid passed in the
-			 * invalidation event applies to all tables.
-			 */
-			if (extension_proxy_oid == relid || !OidIsValid(relid))
-			{
-				extension_update_state();
-				if (EXTENSION_STATE_CREATED != extstate)
-				{
-					/*
-					 * note this state may be UNKNOWN but should be
-					 * conservative
-					 */
-					invalidate_all = true;
-				}
-			}
-			break;
-		default:
-			elog(ERROR, "unknown state: %d", extstate);
-			break;
-	}
-	return invalidate_all;
+	extstate = EXTENSION_STATE_UNKNOWN;
+	extension_proxy_oid = InvalidOid;
 }
 
 bool
@@ -307,6 +299,8 @@ ts_extension_is_loaded(void)
 	switch (extstate)
 	{
 		case EXTENSION_STATE_CREATED:
+			Assert(OidIsValid(ts_extension_oid));
+			Assert(OidIsValid(extension_proxy_oid));
 			return true;
 		case EXTENSION_STATE_NOT_INSTALLED:
 		case EXTENSION_STATE_UNKNOWN:
@@ -353,3 +347,9 @@ ts_extension_get_version(void)
 {
 	return extension_version();
 }
+
+bool
+ts_extension_is_proxy_table_relid(Oid relid)
+{
+	return relid == extension_proxy_oid;
+}

src/extension.h

@@ -6,10 +6,12 @@
 #ifndef TIMESCALEDB_EXTENSION_H
 #define TIMESCALEDB_EXTENSION_H
 #include <postgres.h>
+#include <nodes/parsenodes.h>
+
 #include "extension_constants.h"
 #include "export.h"
 
-extern bool ts_extension_invalidate(Oid relid);
+extern void ts_extension_invalidate(void);
 extern TSDLLEXPORT bool ts_extension_is_loaded(void);
 extern void ts_extension_check_version(const char *so_version);
 extern void ts_extension_check_server_version(void);
@@ -18,7 +20,10 @@ extern TSDLLEXPORT char *ts_extension_schema_name(void);
 extern const char *ts_experimental_schema_name(void);
 extern const char *ts_extension_get_so_name(void);
 extern TSDLLEXPORT const char *ts_extension_get_version(void);
-
-extern TSDLLEXPORT Oid ts_extension_oid;
+extern bool ts_drop_owned_statement_drops_timescaledb_extension(const DropOwnedStmt *stmt);
+extern bool ts_extension_is_proxy_table_relid(Oid relid);
+extern TSDLLEXPORT Oid ts_extension_get_oid(void);
+extern void _ts_extension_init(void);
+extern void _ts_extension_fini(void);
 
 #endif /* TIMESCALEDB_EXTENSION_H */

src/extension_utils.c

@@ -37,10 +37,11 @@ enum ExtensionState
 {
 	/*
 	 * NOT_INSTALLED means that this backend knows that the extension is not
-	 * present.  In this state we know that the proxy table is not present.
-	 * Thus, the only way to get out of this state is a RelCacheInvalidation
-	 * indicating that the proxy table was added. This is the state returned
-	 * during DROP EXTENSION.
+	 * present. In this state we know that the proxy table is not present.
+	 * This state is never saved since there is no real way to get out of it
+	 * since we cannot signal via the proxy table as its relid is not known
+	 * post installation without a full lookup, which is not allowed in the
+	 * relcache calllback.
 	 */
 	EXTENSION_STATE_NOT_INSTALLED,
 
@@ -110,14 +111,15 @@ extension_version(void)
 	return sql_version;
 }
 
-static bool inline proxy_table_exists()
+static Oid
+get_proxy_table_relid()
 {
 	Oid nsid = get_namespace_oid(CACHE_SCHEMA_NAME, true);
 
 	if (!OidIsValid(nsid))
 		return false;
 
-	return OidIsValid(get_relname_relid(EXTENSION_PROXY_TABLE, nsid));
+	return get_relname_relid(EXTENSION_PROXY_TABLE, nsid);
 }
 
 static bool inline extension_exists()
@@ -142,6 +144,8 @@ static bool inline extension_is_transitioning()
 static enum ExtensionState
 extension_current_state()
 {
+	Oid proxy_relid;
+
 	/*
 	 * NormalProcessingMode necessary to avoid accessing cache before its
 	 * ready (which may result in an infinite loop). More concretely we need
@@ -162,7 +166,10 @@ extension_current_state()
 	 * EXTENSION proxy_table_exists() will return false right away, while
 	 * extension_exists will return true until the end of the command
 	 */
-	if (proxy_table_exists())
+
+	proxy_relid = get_proxy_table_relid();
+
+	if (OidIsValid(proxy_relid))
 	{
 		Assert(extension_exists());
 		return EXTENSION_STATE_CREATED;

src/planner.c

@@ -400,13 +400,9 @@ timescaledb_planner(Query *parse, int cursor_opts, ParamListInfo bound_params)
 	{
 		PreprocessQueryContext context = { 0 };
 		context.rootquery = parse;
+
 		if (ts_extension_is_loaded())
 		{
-			/*
-			 * Some debug checks.
-			 */
-			Assert(ts_extension_oid == get_extension_oid(EXTENSION_NAME, /* missing_ok = */ false));
-
 			/*
 			 * Preprocess the hypertables in the query and warm up the caches.
 			 */

src/ts_catalog/catalog.c

@@ -21,6 +21,7 @@
 #include "compat/compat.h"
 #include "ts_catalog/catalog.h"
 #include "extension.h"
+#include "cache_invalidate.h"
 
 static const TableInfoDef catalog_table_names[_MAX_CATALOG_TABLES + 1] = {
 	[HYPERTABLE] = {
@@ -311,6 +312,7 @@ const static InternalFunctionDef internal_function_definitions[_MAX_INTERNAL_FUN
 static const char *cache_proxy_table_names[_MAX_CACHE_TYPES] = {
 	[CACHE_TYPE_HYPERTABLE] = "cache_inval_hypertable",
 	[CACHE_TYPE_BGW_JOB] = "cache_inval_bgw_job",
+	[CACHE_TYPE_EXTENSION] = "cache_inval_extension",
 };
 
 /* Catalog information for the current database. */
@@ -474,6 +476,9 @@ ts_catalog_get(void)
 			get_relname_relid(cache_proxy_table_names[i],
 							  s_catalog.extension_schema_id[TS_CACHE_SCHEMA]);
 
+	ts_cache_invalidate_set_proxy_tables(s_catalog.caches[CACHE_TYPE_HYPERTABLE].inval_proxy_id,
+										 s_catalog.caches[CACHE_TYPE_BGW_JOB].inval_proxy_id);
+
 	for (i = 0; i < _MAX_INTERNAL_FUNCTIONS; i++)
 	{
 		InternalFunctionDef def = internal_function_definitions[i];
@@ -507,6 +512,8 @@ ts_catalog_reset(void)
 {
 	s_catalog.initialized = false;
 	database_info.database_id = InvalidOid;
+
+	ts_cache_invalidate_set_proxy_tables(InvalidOid, InvalidOid);
 }
 
 static CatalogTable

src/ts_catalog/catalog.h

@@ -1272,6 +1272,7 @@ typedef enum CacheType
 {
 	CACHE_TYPE_HYPERTABLE,
 	CACHE_TYPE_BGW_JOB,
+	CACHE_TYPE_EXTENSION,
 	_MAX_CACHE_TYPES
 } CacheType;