Fix relcache callback handling causing crashes

.github/workflows/linux-32bit-build-and-test.yaml

@@ -116,3 +116,13 @@ jobs:
       with:
         name: PostgreSQL log linux-i386 PG${{ matrix.pg }}
         path: postgres.log
+
+    - name: Save TAP test logs
+      if: always()
+      uses: actions/upload-artifact@v3
+      with:
+        name: TAP test logs ${{ matrix.os }} ${{ matrix.name }} ${{ matrix.pg }}
+        path: |
+          build/test/tmp_check/log
+          build/tsl/test/tmp_check/log
+

.github/workflows/linux-build-and-test.yaml

@@ -151,18 +151,18 @@ jobs:
 
     - name: Save regression diffs
       if: always() && steps.collectlogs.outputs.regression_diff == 'true'
-      uses: actions/upload-artifact@v2
+      uses: actions/upload-artifact@v3
       with:
         name: Regression diff ${{ matrix.os }} ${{ matrix.name }} ${{ matrix.pg }}
         path: regression.log
 
     - name: Save postmaster.log
       if: always()
-      uses: actions/upload-artifact@v2
+      uses: actions/upload-artifact@v3
       with:
         name: PostgreSQL log ${{ matrix.os }} ${{ matrix.name }} ${{ matrix.pg }}
         path: postgres.log
-
+      
     - name: Stack trace
       if: always() && steps.collectlogs.outputs.coredumps == 'true'
       run: |
@@ -172,7 +172,17 @@ jobs:
 
     - name: Coredumps
       if: always() && steps.collectlogs.outputs.coredumps == 'true'
-      uses: actions/upload-artifact@v2
+      uses: actions/upload-artifact@v3
       with:
         name: Coredumps ${{ matrix.os }} ${{ matrix.name }} ${{ matrix.pg }}
         path: coredumps
+
+    - name: Save TAP test logs
+      if: always()
+      uses: actions/upload-artifact@v3
+      with:
+        name: TAP test logs ${{ matrix.os }} ${{ matrix.name }} ${{ matrix.pg }}
+        path: |
+          build/test/tmp_check/log
+          build/tsl/test/tmp_check/log
+

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 (!OidIsValid(relid))
 	{
 		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/cache_invalidate.h

@@ -0,0 +1,13 @@
+/*
+ * This file and its contents are licensed under the Apache License 2.0.
+ * Please see the included NOTICE for copyright information and
+ * LICENSE-APACHE for a copy of the license.
+ */
+#ifndef TIMESCALEDB_CACHE_INVALIDATE_H
+#define TIMESCALEDB_CACHE_INVALIDATE_H
+
+#include <postgres.h>
+
+extern void ts_cache_invalidate_set_proxy_tables(Oid hypertable_proxy_oid, Oid bgw_proxy_oid);
+
+#endif /* TIMESCALEDB_CACHE_INVALIDATE_H */

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,26 @@ ts_extension_get_version(void)
 {
 	return extension_version();
 }
+
+bool
+ts_extension_is_proxy_table_relid(Oid relid)
+{
+	return relid == extension_proxy_oid;
+}
+
+#ifdef TS_DEBUG
+static const char *extstate_str[] = {
+	[EXTENSION_STATE_UNKNOWN] = "unknown",
+	[EXTENSION_STATE_TRANSITIONING] = "transitioning",
+	[EXTENSION_STATE_CREATED] = "created",
+	[EXTENSION_STATE_NOT_INSTALLED] = "not installed",
+};
+
+TS_FUNCTION_INFO_V1(ts_extension_get_state);
+
+Datum
+ts_extension_get_state(PG_FUNCTION_ARGS)
+{
+	PG_RETURN_TEXT_P(cstring_to_text(extstate_str[extstate]));
+}
+#endif

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 */