Fix extension drop handling

Previously, the extension could end up in a bad state if it
was dropped as part of a cascade. This PR fixes that by
checking explicitly for the presence of the proxy table
to make sure we are not in the middle of an extension
drop. Fixes #73.

sql/cache.sql

@@ -46,13 +46,4 @@ FOR EACH STATEMENT EXECUTE PROCEDURE _timescaledb_cache.invalidate_relcache_trig
 
 
 
--- This function detects whether a CREATE EXTENSION or DROP EXTENSION is called
--- on this extension and takes the appropriate action.
-CREATE OR REPLACE FUNCTION _timescaledb_cache.extension_event_trigger()
-RETURNS EVENT_TRIGGER AS '$libdir/timescaledb', 'extension_event_trigger' LANGUAGE C;
 
-CREATE EVENT TRIGGER "0_extension_create" ON ddl_command_end WHEN TAG IN ('CREATE EXTENSION')
-EXECUTE PROCEDURE _timescaledb_cache.extension_event_trigger();
-
-CREATE EVENT TRIGGER "0_extension_drop" ON ddl_command_start WHEN TAG IN ('DROP EXTENSION')
-EXECUTE PROCEDURE _timescaledb_cache.extension_event_trigger();

src/cache.c

@@ -10,8 +10,8 @@ cache_init(Cache *cache)
 	}
 
 	/*
-	 * The cache object should have been created in its own context
-	 * so that cache_destroy can just delete the context to free everything.
+	 * The cache object should have been created in its own context so that
+	 * cache_destroy can just delete the context to free everything.
 	 */
 	Assert(MemoryContextContains(cache_memory_ctx(cache), cache));
 

src/cache_invalidate.c

@@ -45,17 +45,15 @@ inval_cache_callback(Datum arg, Oid relid)
 {
 	Catalog    *catalog;
 
-	if (!extension_is_loaded())
-		return;
-
-	if (!OidIsValid(relid) || extension_is_being_dropped(relid))
+	if (extension_invalidate(relid))
 	{
-		/* Extension was dropped or entire cache invalidated. Reset state. */
 		hypertable_cache_invalidate_callback();
-		extension_reset();
 		return;
 	}
 
+	if (!extension_is_loaded())
+		return;
+
 	catalog = catalog_get();
 
 	if (relid == catalog_get_cache_proxy_id(catalog, CACHE_TYPE_HYPERTABLE))

src/extension.c

@@ -21,157 +21,171 @@ static Oid	extension_proxy_oid = InvalidOid;
  * check for presence of the extension itself), we also need to track the
  * extension state to know when the metadata is valid.
  *
- * The metadata itself is initialized and cached lazily when calling, e.g.,
- * extension_is_loaded().
+ * We use a proxy_table to be notified of extension drops/creates. Namely,
+ * we rely on the fact that postgres will internally create RelCacheInvalidation
+ * events when any tables are created or dropped. We rely on the following properties
+ * of Postgres's dependency managment:
+ *	* The proxy table will be created before the extension itself.
+ *	* The proxy table will be dropped before the extension itself.
  */
 enum ExtensionState
 {
 	/*
-	 * INITIAL is the state for new backends or after an extension was
-	 * dropped. The extension could be loaded or not. Only a check can tell.
+	 * 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.
 	 */
-	EXTENSION_STATE_INITIAL,
+	EXTENSION_STATE_NOT_INSTALLED,
 
 	/*
-	 * CREATING only occurs on the backend that issues the CREATE EXTENSION
-	 * statement.
+	 * UNKNOWN state is used only if we cannot be sure what the state is. This
+	 * can happen in two cases: 1) at the start of a backend or 2) We got a
+	 * relcache event outside of a transaction and thus could not check the
+	 * cache for the presence/absence of the proxy table or extension.
 	 */
-	EXTENSION_STATE_CREATING,
+	EXTENSION_STATE_UNKNOWN,
+
+	/*
+	 * TRANSITIONING only occurs when the proxy table exists but the extension
+	 * does not. This can only happen in the middle of a create or drop
+	 * extension.
+	 */
+	EXTENSION_STATE_TRANSITIONING,
 
 	/*
 	 * CREATED means we know the extension is loaded, metadata is up-to-date,
-	 * and we therefore do not need a full check until we enter another state.
+	 * and we therefore do not need a full check until a RelCacheInvalidation
+	 * on the proxy table.
 	 */
 	EXTENSION_STATE_CREATED,
-
-	/*
-	 * DROPPING happens immediately after a DROP EXTENSION is issued. This
-	 * only happends on the backend that issues the actual extension drop. All
-	 * backends, including the issuing one, will be notified and move to
-	 * INITIAL. Note that DROPPING does NOT mean the extension is not present,
-	 * but rather that it will be gone by the end of the transaction that set
-	 * this state.
-	 */
-	EXTENSION_STATE_DROPPING,
 };
 
-static enum ExtensionState extstate = EXTENSION_STATE_INITIAL;
-static TransactionId drop_transaction_id = InvalidTransactionId;
+static enum ExtensionState extstate = EXTENSION_STATE_UNKNOWN;
 
+static bool
+proxy_table_exists()
+{
+	Oid			nsid = get_namespace_oid(CACHE_SCHEMA_NAME, true);
+	Oid			proxy_table = get_relname_relid(EXTENSION_PROXY_TABLE, nsid);
+
+	return OidIsValid(proxy_table);
+}
+
+static bool
+extension_exists()
+{
+	return OidIsValid(get_extension_oid(EXTENSION_NAME, true));
+}
+
+/* Returns the recomputed current state */
+static enum ExtensionState
+extension_new_state()
+{
+	if (!IsTransactionState())
+		return EXTENSION_STATE_UNKNOWN;
+
+	if (proxy_table_exists())
+	{
+		if (!extension_exists())
+			return EXTENSION_STATE_TRANSITIONING;
+		else
+			return EXTENSION_STATE_CREATED;
+	}
+	return EXTENSION_STATE_NOT_INSTALLED;
+}
+
+/* Sets a new state, returning whether the state has changed */
+static bool
+extension_set_state(enum ExtensionState newstate)
+{
+	if (newstate == extstate)
+	{
+		return false;
+	}
+	switch (newstate)
+	{
+		case EXTENSION_STATE_TRANSITIONING:
+		case EXTENSION_STATE_UNKNOWN:
+			break;
+		case EXTENSION_STATE_CREATED:
+			extension_proxy_oid = get_relname_relid(EXTENSION_PROXY_TABLE, get_namespace_oid(CACHE_SCHEMA_NAME, false));
+			catalog_reset();
+			break;
+		case EXTENSION_STATE_NOT_INSTALLED:
+			extension_proxy_oid = InvalidOid;
+			catalog_reset();
+			break;
+	}
+	extstate = newstate;
+	return true;
+}
+
+/* Updates the state based on the current state, returning whether there had been a change. */
+static bool
+extension_update_state()
+{
+	return extension_set_state(extension_new_state());
+}
+
+/*
+ *	Called upon all Relcache invalidate events.
+ *	Returns whether or not to invalidate the entire extension.
+ */
 bool
-extension_is_being_dropped(Oid relid)
+extension_invalidate(Oid relid)
 {
-	return relid == extension_proxy_oid;
-}
-
-static void
-extension_init(void)
-{
-	Oid			nsid = get_namespace_oid(CACHE_SCHEMA_NAME, false);
-
-	drop_transaction_id = InvalidTransactionId;
-	extension_proxy_oid = get_relname_relid(EXTENSION_PROXY_TABLE, nsid);
-	catalog_reset();
-}
-
-void
-extension_reset(void)
-{
-	extension_proxy_oid = InvalidOid;
-	catalog_reset();
-	extstate = EXTENSION_STATE_INITIAL;
-}
-
-PG_FUNCTION_INFO_V1(extension_event_trigger);
-
-Datum
-extension_event_trigger(PG_FUNCTION_ARGS)
-{
-	EventTriggerData *trigdata = (EventTriggerData *) fcinfo->context;
-
-	if (!CALLED_AS_EVENT_TRIGGER(fcinfo))
-		elog(ERROR, "not fired by event trigger manager");
-
-	if (strcmp(trigdata->event, "ddl_command_end") == 0 &&
-		strcmp(trigdata->tag, "CREATE EXTENSION") == 0 &&
-		IsA(trigdata->parsetree, CreateExtensionStmt) &&
-		strcmp(((CreateExtensionStmt *) trigdata->parsetree)->extname, EXTENSION_NAME) == 0)
+	switch (extstate)
 	{
-		extstate = EXTENSION_STATE_CREATING;
-	}
-	else if (strcmp(trigdata->event, "ddl_command_start") == 0 &&
-			 strcmp(trigdata->tag, "DROP EXTENSION") == 0 &&
-			 IsA(trigdata->parsetree, DropStmt))
-	{
-		DropStmt   *stmt = (DropStmt *) trigdata->parsetree;
-		const char *extname = strVal(linitial(linitial(stmt->objects)));
-
-		if (strcmp(extname, EXTENSION_NAME) == 0)
-		{
-			extstate = EXTENSION_STATE_DROPPING;
+		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();
+			return false;
+		case EXTENSION_STATE_CREATED:
 
 			/*
-			 * Save the transaction ID, so that we can avoid going from
-			 * INITIAL to CREATED in the transaction that issued DROP
-			 * EXTENSION.
+			 * 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.
 			 */
-			drop_transaction_id = GetCurrentTransactionId();
-
-			/*
-			 * Notify other backends that the extension was dropped. We do
-			 * this via the relcache invalidation mechanism in Postgres by
-			 * issuing an invalidation on a proxy table. Other backends will
-			 * see an invalidation event on the proxy table and then knows
-			 * they need to move back to INITIAL state.
-			 */
-			CacheInvalidateRelcacheByRelid(extension_proxy_oid);
-		}
+			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
+					 */
+					return true;
+				}
+			}
+			return false;
 	}
-
-	PG_RETURN_NULL();
 }
 
+
 bool
 extension_is_loaded(void)
 {
-	Oid			id;
-
-	/* The extension is always valid in CREATED state */
-	if (EXTENSION_STATE_CREATED == extstate)
-		return true;
-
-	if (!IsTransactionState())
-		return false;
-
-	/*
-	 * Do a full check for extension presence. If present, initialize the
-	 * cached extension state unless the extension is being dropped.
-	 */
-	id = get_extension_oid(EXTENSION_NAME, true);
-
-	if (OidIsValid(id))
+	if (EXTENSION_STATE_UNKNOWN == extstate || EXTENSION_STATE_TRANSITIONING == extstate)
 	{
-		if (creating_extension && id == CurrentExtensionObject)
-		{
-			/* Extension is still being created */
-			extstate = EXTENSION_STATE_CREATING;
-			return false;
-		}
-
-		/*
-		 * This check protects against resetting the extension state while
-		 * still in the transaction that is dropping the extension, which
-		 * could otherwise leave us with a state indicating the extension is
-		 * still present after it is dropped.
-		 */
-		if (extstate < EXTENSION_STATE_CREATED &&
-			!TransactionIdIsCurrentTransactionId(drop_transaction_id))
-		{
-			extstate = EXTENSION_STATE_CREATED;
-			extension_init();
-		}
-		return true;
+		/* status may have updated without a relcache invalidate event */
+		extension_update_state();
 	}
 
-	return false;
+	switch (extstate)
+	{
+		case EXTENSION_STATE_NOT_INSTALLED:
+		case EXTENSION_STATE_UNKNOWN:
+		case EXTENSION_STATE_TRANSITIONING:
+			return false;
+		case EXTENSION_STATE_CREATED:
+			return true;
+
+	}
 }

src/extension.h

@@ -1,8 +1,8 @@
 #ifndef TIMESCALEDB_EXTENSION_H
 #define TIMESCALEDB_EXTENSION_H
+#include <postgres.h>
 
-bool		extension_is_being_dropped(Oid relid);
-void		extension_reset(void);
+bool		extension_invalidate(Oid relid);
 bool		extension_is_loaded(void);
 
 #endif   /* TIMESCALEDB_EXTENSION_H */

test/expected/drop_extension.out

@@ -63,3 +63,22 @@ SELECT * FROM drop_test;
  Mon Mar 20 09:18:19.100462 2017 | 22.1 | dev1
 (1 row)
 
+--test drops thru cascades of other objects
+\c postgres
+\o /dev/null
+\ir include/create_single_db.sql
+SET client_min_messages = WARNING;
+DROP DATABASE IF EXISTS single;
+SET client_min_messages = NOTICE;
+CREATE DATABASE single;
+\c single
+CREATE EXTENSION IF NOT EXISTS timescaledb;
+\o
+drop schema public cascade;
+NOTICE:  drop cascades to extension timescaledb
+\dn
+List of schemas
+ Name | Owner 
+------+-------
+(0 rows)
+

test/expected/pg_dump.out

@@ -42,7 +42,7 @@ SELECT count(*)
      AND refobjid = (SELECT oid FROM pg_extension WHERE extname = 'timescaledb');
  count 
 -------
-   107
+   104
 (1 row)
 
 \c postgres
@@ -66,7 +66,7 @@ SELECT count(*)
      AND refobjid = (SELECT oid FROM pg_extension WHERE extname = 'timescaledb');
  count 
 -------
-   107
+   104
 (1 row)
 
 \c single

test/sql/drop_extension.sql

@@ -29,3 +29,12 @@ SELECT create_hypertable('drop_test', 'time', 'device', 2);
 SELECT * FROM _timescaledb_catalog.hypertable;
 INSERT INTO drop_test VALUES('Mon Mar 20 09:18:19.100462 2017', 22.1, 'dev1');
 SELECT * FROM drop_test;
+
+--test drops thru cascades of other objects
+\c postgres
+\o /dev/null
+\ir include/create_single_db.sql
+\o
+
+drop schema public cascade;
+\dn