Fix Cache Pinning for Subtxns

Previously, all Subtxn aborts released all cache pins. This is
wrong because that can release pins that are still in use by
higher-level subtxns and top-level txns. Now, we release only
pins corresponding to the appropriate subtxn. That means that we
now track the subtxn a pin was created in.

src/cache.c

@@ -6,6 +6,11 @@
 /* List of pinned caches. A cache occurs once in this list for every pin
  * taken */
 static List *pinned_caches = NIL;
+typedef struct CachePin
+{
+	Cache	   *cache;
+	SubTransactionId subtxnid;
+} CachePin;
 
 void
 cache_init(Cache *cache)
@@ -67,26 +72,58 @@ extern Cache *
 cache_pin(Cache *cache)
 {
 	MemoryContext old = MemoryContextSwitchTo(CacheMemoryContext);
+	CachePin   *cp = palloc(sizeof(CachePin));
 
-	pinned_caches = lappend(pinned_caches, cache);
+	cp->cache = cache;
+	cp->subtxnid = GetCurrentSubTransactionId();
+	pinned_caches = lappend(pinned_caches, cp);
 	MemoryContextSwitchTo(old);
 	cache->refcount++;
 	return cache;
 }
 
-extern int
-cache_release(Cache *cache)
+static void
+remove_pin(Cache *cache, SubTransactionId subtxnid)
+{
+	ListCell   *lc,
+			   *prev = NULL;
+
+	foreach(lc, pinned_caches)
+	{
+		CachePin   *cp = lfirst(lc);
+
+		if (cp->cache == cache && cp->subtxnid == subtxnid)
+		{
+			pinned_caches = list_delete_cell(pinned_caches, lc, prev);
+			return;
+		}
+
+		prev = lc;
+	}
+
+	/* should never reach here: there should always be a pin to remove */
+	Assert(false);
+}
+
+static int
+cache_release_subtxn(Cache *cache, SubTransactionId subtxnid)
 {
 	int			refcount = cache->refcount - 1;
 
 	Assert(cache->refcount > 0);
 	cache->refcount--;
-	pinned_caches = list_delete_ptr(pinned_caches, cache);
+
+	remove_pin(cache, subtxnid);
 	cache_destroy(cache);
 
 	return refcount;
 }
 
+extern int
+cache_release(Cache *cache)
+{
+	return cache_release_subtxn(cache, GetCurrentSubTransactionId());
+}
 
 MemoryContext
 cache_memory_ctx(Cache *cache)
@@ -167,15 +204,40 @@ release_all_pinned_caches()
 	 */
 	foreach(lc, pinned_caches)
 	{
-		Cache	   *cache = lfirst(lc);
+		CachePin   *cp = lfirst(lc);
 
-		cache->refcount--;
-		cache_destroy(cache);
+		cp->cache->refcount--;
+		cache_destroy(cp->cache);
 	}
 	list_free(pinned_caches);
 	pinned_caches = NIL;
 }
 
+static void
+release_subtxn_pinned_caches(SubTransactionId subtxnid, bool abort)
+{
+	ListCell   *lc;
+
+	/* Need a copy because cache_release will modify pinned_caches */
+	List	   *pinned_caches_copy = list_copy(pinned_caches);
+
+	/* Only release caches created in subtxn */
+	foreach(lc, pinned_caches_copy)
+	{
+		CachePin   *cp = lfirst(lc);
+
+		if (cp->subtxnid == subtxnid)
+		{
+			/*
+			 * This assert makes sure that that we don't have a cache leak
+			 * when running with debugging
+			 */
+			Assert(abort);
+			cache_release_subtxn(cp->cache, subtxnid);
+		}
+	}
+}
+
 /*
  * Transaction end callback that cleans up any pinned caches. This is a
  * safeguard that protects against indefinitely pinned caches (memory leaks)
@@ -205,41 +267,53 @@ cache_xact_end(XactEvent event, void *arg)
 				 */
 				foreach(lc, pinned_caches)
 				{
-					Cache	   *cache = lfirst(lc);
+					CachePin   *cp = lfirst(lc);
 
 					/*
 					 * This assert makes sure that that we don't have a cache
 					 * leak when running with debugging
 					 */
-					Assert(!cache->release_on_commit);
+					Assert(!cp->cache->release_on_commit);
 
 					/*
 					 * This may still happen in optimized environments where
 					 * Assert is turned off. In that case, release.
 					 */
-					if (cache->release_on_commit)
-						cache_release(cache);
+					if (cp->cache->release_on_commit)
+						cache_release(cp->cache);
 				}
 			}
 			break;
 	}
 }
 
 static void
-cache_subxact_abort(SubXactEvent event, SubTransactionId mySubid,
+cache_subxact_abort(SubXactEvent event, SubTransactionId subtxn_id,
 					SubTransactionId parentSubid, void *arg)
 {
 	/*
 	 * Note that cache->release_on_commit is irrelevant here since can't have
 	 * cross-commit operations in subtxns
 	 */
-
 	/*
 	 * In subtxns, caches should have already been released, unless an abort
-	 * happened
+	 * happened. Be careful to only release caches that were created in the
+	 * same subtxn.
 	 */
-	Assert(SUBXACT_EVENT_ABORT_SUB == event || list_length(pinned_caches) == 0);
-	release_all_pinned_caches();
+
+	switch (event)
+	{
+		case SUBXACT_EVENT_START_SUB:
+		case SUBXACT_EVENT_PRE_COMMIT_SUB:
+			/* do nothing */
+			break;
+		case SUBXACT_EVENT_COMMIT_SUB:
+			release_subtxn_pinned_caches(subtxn_id, false);
+			break;
+		case SUBXACT_EVENT_ABORT_SUB:
+			release_subtxn_pinned_caches(subtxn_id, true);
+			break;
+	}
 }
 
 

test/expected/triggers.out

@@ -11,7 +11,8 @@ DECLARE
 BEGIN
     SELECT count(*) INTO cnt FROM hyper;
     RAISE WARNING 'FIRING trigger when: % level: % op: % cnt: % trigger_name %',
-          tg_when, tg_level, tg_op, cnt, tg_name;
+        tg_when, tg_level, tg_op, cnt, tg_name;
+
     IF TG_OP = 'DELETE' THEN
         RETURN OLD;
     END IF;
@@ -270,9 +271,14 @@ CREATE TABLE vehicles (
   vin_number CHAR(17),
   last_checkup TIMESTAMP
 );
+CREATE TABLE color (
+  color_id INTEGER PRIMARY KEY,
+  notes text
+);
 CREATE TABLE location (
   time TIMESTAMP NOT NULL,
   vehicle_id INTEGER REFERENCES vehicles (vehicle_id),
+  color_id INTEGER, --no reference since gonna populate a hypertable
   latitude FLOAT,
   longitude FLOAT
 );
@@ -284,27 +290,50 @@ BEGIN
     RETURN NEW;
 END
 $BODY$;
+CREATE OR REPLACE FUNCTION create_color_trigger_fn()
+    RETURNS TRIGGER LANGUAGE PLPGSQL AS
+$BODY$
+BEGIN
+    --test subtxns within triggers
+    BEGIN
+        INSERT INTO color VALUES(NEW.color_id, 'n/a');
+    EXCEPTION WHEN unique_violation THEN
+			-- Nothing to do, just continue
+	END;
+    RETURN NEW;
+END
+$BODY$;
 CREATE TRIGGER create_vehicle_trigger
     BEFORE INSERT OR UPDATE ON location
     FOR EACH ROW EXECUTE PROCEDURE create_vehicle_trigger_fn();
+CREATE TRIGGER create_color_trigger
+    BEFORE INSERT OR UPDATE ON location
+    FOR EACH ROW EXECUTE PROCEDURE create_color_trigger_fn();
 SELECT create_hypertable('location', 'time');
  create_hypertable 
 -------------------
 
 (1 row)
 
-INSERT INTO location VALUES('2017-01-01 01:02:03', 1, 40.7493226,-73.9771259);
-INSERT INTO location VALUES('2017-01-01 01:02:04', 1, 24.7493226,-73.9771259);
-INSERT INTO location VALUES('2017-01-01 01:02:03', 23, 40.7493226,-73.9771269);
-INSERT INTO location VALUES('2017-01-01 01:02:03', 53, 40.7493226,-73.9771269);
+--make color also a hypertable
+SELECT create_hypertable('color', 'color_id', chunk_time_interval=>10);
+ create_hypertable 
+-------------------
+
+(1 row)
+
+INSERT INTO location VALUES('2017-01-01 01:02:03', 1, 1, 40.7493226,-73.9771259);
+INSERT INTO location VALUES('2017-01-01 01:02:04', 1, 20, 24.7493226,-73.9771259);
+INSERT INTO location VALUES('2017-01-01 01:02:03', 23, 1, 40.7493226,-73.9771269);
+INSERT INTO location VALUES('2017-01-01 01:02:03', 53, 20, 40.7493226,-73.9771269);
 UPDATE location SET vehicle_id = 52 WHERE vehicle_id = 53;
 SELECT * FROM location;
-           time           | vehicle_id |  latitude  |  longitude  
---------------------------+------------+------------+-------------
- Sun Jan 01 01:02:03 2017 |          1 | 40.7493226 | -73.9771259
- Sun Jan 01 01:02:04 2017 |          1 | 24.7493226 | -73.9771259
- Sun Jan 01 01:02:03 2017 |         23 | 40.7493226 | -73.9771269
- Sun Jan 01 01:02:03 2017 |         52 | 40.7493226 | -73.9771269
+           time           | vehicle_id | color_id |  latitude  |  longitude  
+--------------------------+------------+----------+------------+-------------
+ Sun Jan 01 01:02:03 2017 |          1 |        1 | 40.7493226 | -73.9771259
+ Sun Jan 01 01:02:04 2017 |          1 |       20 | 24.7493226 | -73.9771259
+ Sun Jan 01 01:02:03 2017 |         23 |        1 | 40.7493226 | -73.9771269
+ Sun Jan 01 01:02:03 2017 |         52 |       20 | 40.7493226 | -73.9771269
 (4 rows)
 
 SELECT * FROM vehicles;
@@ -316,3 +345,10 @@ SELECT * FROM vehicles;
          52 |            | 
 (4 rows)
 
+SELECT * FROM color;
+ color_id | notes 
+----------+-------
+        1 | n/a
+       20 | n/a
+(2 rows)
+

test/sql/triggers.sql

@@ -12,7 +12,8 @@ DECLARE
 BEGIN
     SELECT count(*) INTO cnt FROM hyper;
     RAISE WARNING 'FIRING trigger when: % level: % op: % cnt: % trigger_name %',
-          tg_when, tg_level, tg_op, cnt, tg_name;
+        tg_when, tg_level, tg_op, cnt, tg_name;
+
     IF TG_OP = 'DELETE' THEN
         RETURN OLD;
     END IF;
@@ -204,9 +205,16 @@ CREATE TABLE vehicles (
   last_checkup TIMESTAMP
 );
 
+CREATE TABLE color (
+  color_id INTEGER PRIMARY KEY,
+  notes text
+);
+
+
 CREATE TABLE location (
   time TIMESTAMP NOT NULL,
   vehicle_id INTEGER REFERENCES vehicles (vehicle_id),
+  color_id INTEGER, --no reference since gonna populate a hypertable
   latitude FLOAT,
   longitude FLOAT
 );
@@ -220,18 +228,41 @@ BEGIN
 END
 $BODY$;
 
+
+CREATE OR REPLACE FUNCTION create_color_trigger_fn()
+    RETURNS TRIGGER LANGUAGE PLPGSQL AS
+$BODY$
+BEGIN
+    --test subtxns within triggers
+    BEGIN
+        INSERT INTO color VALUES(NEW.color_id, 'n/a');
+    EXCEPTION WHEN unique_violation THEN
+			-- Nothing to do, just continue
+	END;
+    RETURN NEW;
+END
+$BODY$;
+
 CREATE TRIGGER create_vehicle_trigger
     BEFORE INSERT OR UPDATE ON location
     FOR EACH ROW EXECUTE PROCEDURE create_vehicle_trigger_fn();
 
+CREATE TRIGGER create_color_trigger
+    BEFORE INSERT OR UPDATE ON location
+    FOR EACH ROW EXECUTE PROCEDURE create_color_trigger_fn();
+
+
 SELECT create_hypertable('location', 'time');
+--make color also a hypertable
+SELECT create_hypertable('color', 'color_id', chunk_time_interval=>10);
 
-INSERT INTO location VALUES('2017-01-01 01:02:03', 1, 40.7493226,-73.9771259);
-INSERT INTO location VALUES('2017-01-01 01:02:04', 1, 24.7493226,-73.9771259);
-INSERT INTO location VALUES('2017-01-01 01:02:03', 23, 40.7493226,-73.9771269);
-INSERT INTO location VALUES('2017-01-01 01:02:03', 53, 40.7493226,-73.9771269);
+INSERT INTO location VALUES('2017-01-01 01:02:03', 1, 1, 40.7493226,-73.9771259);
+INSERT INTO location VALUES('2017-01-01 01:02:04', 1, 20, 24.7493226,-73.9771259);
+INSERT INTO location VALUES('2017-01-01 01:02:03', 23, 1, 40.7493226,-73.9771269);
+INSERT INTO location VALUES('2017-01-01 01:02:03', 53, 20, 40.7493226,-73.9771269);
 
 UPDATE location SET vehicle_id = 52 WHERE vehicle_id = 53;
 
 SELECT * FROM location;
 SELECT * FROM vehicles;
+SELECT * FROM color;