Fix pg_dump test and make insert block trigger a non-internal trigger

Previously, the pg_dump test was broken because it is not possible to reference psql variables from inside bash commands run through psql. This is fixed by hardcoding the username passed to the bash commands inside the test. Also, we changed the insert block trigger preventing inserts into hypertable to a non-internal trigger, because internal triggers are not dumped by pg_dump. We need to dump the trigger so that it is already in place after a pg_restore, to prevent users from accidentally inserting rows into a hypertable while timescaledb_restoring=on.
timescale · Sep 4, 2018 · c71960c · c71960c
1 parent b928caa
commit c71960c
Show file tree

Hide file tree

Showing 10 changed files with 86 additions and 74 deletions.
diff --git a/sql/updates/0.10.1--0.11.0.sql b/sql/updates/0.10.1--0.11.0.sql
@@ -1,15 +1,3 @@
--- The trigger function must be defined so that we can add it to legacy tables
-CREATE OR REPLACE FUNCTION _timescaledb_internal.insert_blocker() RETURNS trigger
-AS '@MODULE_PATHNAME@', 'hypertable_insert_blocker' LANGUAGE C;
-
-CREATE FUNCTION _timescaledb_internal.insert_blocker_trigger_add(relid REGCLASS) RETURNS OID
-AS '@MODULE_PATHNAME@', 'hypertable_insert_blocker_trigger_add' LANGUAGE C VOLATILE STRICT;
-
-SELECT _timescaledb_internal.insert_blocker_trigger_add(h.relid)
-FROM (SELECT format('%I.%I', schema_name, table_name)::regclass AS relid FROM _timescaledb_catalog.hypertable) AS h;
-
-DROP FUNCTION _timescaledb_internal.insert_blocker_trigger_add(REGCLASS);
-
 -- Adaptive chunking
 CREATE OR REPLACE FUNCTION _timescaledb_internal.calculate_chunk_interval(
         dimension_id INTEGER,

diff --git a/sql/updates/0.11.0--0.11.1-dev.sql b/sql/updates/0.11.0--0.11.1-dev.sql
@@ -0,0 +1,12 @@
+-- Trigger that blocks INSERTs on the hypertable's root table
+CREATE OR REPLACE FUNCTION _timescaledb_internal.insert_blocker() RETURNS trigger
+AS '@MODULE_PATHNAME@', 'hypertable_insert_blocker' LANGUAGE C;
+
+-- Drop all pre-0.11.1 insert_blockers from hypertables and add the new, visible trigger
+CREATE FUNCTION _timescaledb_internal.insert_blocker_trigger_add(relid REGCLASS) RETURNS OID
+AS '@MODULE_PATHNAME@', 'hypertable_insert_blocker_trigger_add' LANGUAGE C VOLATILE STRICT;
+
+SELECT _timescaledb_internal.insert_blocker_trigger_add(h.relid)
+FROM (SELECT format('%I.%I', schema_name, table_name)::regclass AS relid FROM _timescaledb_catalog.hypertable) AS h;
+
+DROP FUNCTION _timescaledb_internal.insert_blocker_trigger_add(REGCLASS);
diff --git a/src/hypertable.c b/src/hypertable.c
@@ -1004,7 +1004,7 @@ hypertable_validate_constraints(Oid relid)
  * Functionality to block INSERTs on the hypertable's root table.
  *
  * The design considered implementing this either with RULES, constraints, or
- * triggers. An "internal" trigger was found to have the best trade-offs:
+ * triggers. A visible trigger was found to have the best trade-offs:
  *
  * - A RULE doesn't work since it rewrites the query and thus blocks INSERTs
  *   also on the hypertable.
@@ -1017,14 +1017,19 @@ hypertable_validate_constraints(Oid relid)
  *   (you can work around this, but is messy). This issue, b.t.w., broke one
  *   of the tests.
  *
- * - A trigger, especially an "internal" one, is transparent (doesn't show up
+ * - An internal trigger is transparent (doesn't show up
  *	 on \d+ <table>) and is automatically removed when the extension is
  *	 dropped (since it is part of the extension). Internal triggers aren't
  *	 inherited by chunks either, so we need no special handling to _not_
- *	 inherit the blocking trigger.
+ *	 inherit the blocking trigger. However, internal triggers are not exported
+ *   via pg_dump. Because a critical use case for this trigger is to ensure
+ *   no rows are inserted into hypertables by accident when a user forgets to
+ *   turn restoring off, having this trigger exported in pg_dump is essential.
+ *
+ * - A visible trigger unfortunately shows up in \d+ <table>, but is
+ *   included in a pg_dump. We also add logic to make sure this trigger is not
+ *   propagated to chunks.
  */
-#define INSERT_BLOCKER_NAME "insert_blocker"
-
 TS_FUNCTION_INFO_V1(hypertable_insert_blocker);
 
 Datum
@@ -1051,14 +1056,14 @@ hypertable_insert_blocker(PG_FUNCTION_ARGS)
 }
 
 /*
- * Get the insert blocker trigger on a table.
+ * Get the legacy insert blocker trigger on a table.
  *
- * Note that we cannot get the insert trigger by name since internal triggers
+ * Note that we cannot get the old insert trigger by name since internal triggers
  * are made unique by appending the trigger OID, which we do not
  * know. Instead, we have to search all triggers.
  */
 static Oid
-insert_blocker_trigger_get(Oid relid)
+old_insert_blocker_trigger_get(Oid relid)
 {
 	Relation	tgrel;
 	ScanKeyData skey[1];
@@ -1081,8 +1086,7 @@ insert_blocker_trigger_get(Oid relid)
 		Form_pg_trigger trig = (Form_pg_trigger) GETSTRUCT(tuple);
 
 		if (TRIGGER_TYPE_MATCHES(trig->tgtype, TRIGGER_TYPE_ROW, TRIGGER_TYPE_BEFORE, TRIGGER_TYPE_INSERT) &&
-			strncmp(INSERT_BLOCKER_NAME, NameStr(trig->tgname), strlen(INSERT_BLOCKER_NAME)) == 0 &&
-			trig->tgisinternal)
+			strncmp(OLD_INSERT_BLOCKER_NAME, NameStr(trig->tgname), strlen(OLD_INSERT_BLOCKER_NAME)) == 0  && trig->tgisinternal)
 		{
 			tgoid = HeapTupleGetOid(tuple);
 			break;
@@ -1112,25 +1116,19 @@ insert_blocker_trigger_add(Oid relid)
 		.type = T_CreateTrigStmt,
 		.row = true,
 		.timing = TRIGGER_TYPE_BEFORE,
-		.trigname = INSERT_BLOCKER_NAME,	/* Note, internal triggers get the
-											 * OID appended to the name */
+		.trigname = INSERT_BLOCKER_NAME,
 		.relation = makeRangeVar(schema, relname, -1),
-		.funcname = list_make2(makeString(INTERNAL_SCHEMA_NAME), makeString(INSERT_BLOCKER_NAME)),
+		.funcname = list_make2(makeString(INTERNAL_SCHEMA_NAME), makeString(OLD_INSERT_BLOCKER_NAME)),
 		.args = NIL,
 		.events = TRIGGER_TYPE_INSERT,
 	};
 
-	objaddr.objectId = insert_blocker_trigger_get(relid);
-
-	/* Trigger already exists. Do nothing */
-	if (OidIsValid(objaddr.objectId))
-		return objaddr.objectId;
-
 	/*
-	 * Create as an internal trigger; it won't show up with \d and won't be
-	 * inherited by chunks.
+	 * We create a user-visible trigger, so that it will get pg_dump'd with
+	 * the hypertable. This call will error out if a trigger with the same name already exists.
+     * (This is the desired behavior.)
 	 */
-	objaddr = CreateTrigger(&stmt, NULL, relid, InvalidOid, InvalidOid, InvalidOid, true);
+	objaddr = CreateTrigger(&stmt, NULL, relid, InvalidOid, InvalidOid, InvalidOid, false);
 
 	if (!OidIsValid(objaddr.objectId))
 		elog(ERROR, "could not create insert blocker trigger");
@@ -1141,9 +1139,9 @@ insert_blocker_trigger_add(Oid relid)
 TS_FUNCTION_INFO_V1(hypertable_insert_blocker_trigger_add);
 
 /*
- * This function is exposed to add the blocking trigger on legacy hypertables
- * that don't have the trigger. We can't do it from SQL code, because internal
- * triggers cannot be added from SQL.
+ * This function is exposed to drop the old blocking trigger on legacy hypertables.
+ * We can't do it from SQL code, because internal triggers cannot be dropped from SQL.
+ * After the legacy internal trigger is dropped, we add the new, visible trigger.
  *
  * In case the hypertable's root table has data in it, we bail out with an
  * error instructing the user to fix the issue first.
@@ -1152,6 +1150,7 @@ Datum
 hypertable_insert_blocker_trigger_add(PG_FUNCTION_ARGS)
 {
 	Oid			relid = PG_GETARG_OID(0);
+	Oid			old_trigger;
 
 	if (table_has_tuples(relid, AccessShareLock))
 		ereport(ERROR,
@@ -1167,6 +1166,19 @@ hypertable_insert_blocker_trigger_add(PG_FUNCTION_ARGS)
 						 "> SET timescaledb.restoring = 'OFF';\n"
 						 "> COMMIT;", get_rel_name(relid))));
 
+	/* Now drop the old trigger */
+	old_trigger = old_insert_blocker_trigger_get(relid);
+	if (OidIsValid(old_trigger))
+	{
+		ObjectAddress objaddr = {
+			.classId = TriggerRelationId,
+			.objectId = old_trigger
+		};
+
+		performDeletion(&objaddr, DROP_RESTRICT, 0);
+	}
+
+	/* Add the new trigger */
 	PG_RETURN_OID(insert_blocker_trigger_add(relid));
 }
 

diff --git a/src/hypertable.h b/src/hypertable.h
@@ -9,6 +9,9 @@
 #include "tablespace.h"
 #include "scanner.h"
 
+#define OLD_INSERT_BLOCKER_NAME	"insert_blocker"
+#define INSERT_BLOCKER_NAME "ts_insert_blocker"
+
 typedef struct SubspaceStore SubspaceStore;
 typedef struct Chunk Chunk;
 typedef struct HeapTupleData *HeapTuple;
@@ -22,7 +25,6 @@ typedef struct Hypertable
 	SubspaceStore *chunk_cache;
 } Hypertable;
 
-
 extern Oid	rel_get_owner(Oid relid);
 extern Hypertable *hypertable_get_by_id(int32 hypertable_id);
 extern Hypertable *hypertable_get_by_name(char *schema, char *name);

diff --git a/src/trigger.h b/src/trigger.h
@@ -7,7 +7,7 @@
 #include "chunk.h"
 
 #define trigger_is_chunk_trigger(trigger) \
-	((trigger) != NULL && TRIGGER_FOR_ROW((trigger)->tgtype) && !(trigger)->tgisinternal)
+	((trigger) != NULL && TRIGGER_FOR_ROW((trigger)->tgtype) && !(trigger)->tgisinternal && strcmp((trigger)->tgname, INSERT_BLOCKER_NAME) != 0)
 
 extern Trigger *trigger_by_name(Oid relid, const char *name, bool missing_ok);
 extern void trigger_create_on_chunk(Oid trigger_oid, char *chunk_schema_name, char *chunk_table_name);

diff --git a/test/expected/create_hypertable.out b/test/expected/create_hypertable.out
@@ -62,11 +62,11 @@ NOTICE:  adding not-null constraint to column "time"
 
 (1 row)
 
--- Check that the insert block trigger exists (cannot show name or definition since they contain OID)
-SELECT "Function" FROM test.show_triggers('test_schema.test_table', show_internal => true);
-               Function               
---------------------------------------
- _timescaledb_internal.insert_blocker
+-- Check that the insert block trigger exists
+SELECT * FROM test.show_triggers('test_schema.test_table');
+      Trigger      | Type |               Function               |                                                            Definition                                                            
+-------------------+------+--------------------------------------+----------------------------------------------------------------------------------------------------------------------------------
+ ts_insert_blocker |    7 | _timescaledb_internal.insert_blocker |  ts_insert_blocker BEFORE INSERT ON test_schema.test_table FOR EACH ROW EXECUTE PROCEDURE _timescaledb_internal.insert_blocker()
 (1 row)
 
 SELECT * FROM _timescaledb_internal.get_create_command('test_table');

diff --git a/test/expected/pg_dump.out b/test/expected/pg_dump.out
@@ -153,10 +153,11 @@ SELECT * FROM test.show_constraints('_timescaledb_internal._hyper_1_1_chunk');
 (3 rows)
 
 SELECT * FROM test.show_triggers('"test_schema"."two_Partitions"');
-     Trigger     | Type |   Function   |                                                  Definition                                                  
------------------+------+--------------+--------------------------------------------------------------------------------------------------------------
- restore_trigger |    7 | test_trigger |  restore_trigger BEFORE INSERT ON test_schema."two_Partitions" FOR EACH ROW EXECUTE PROCEDURE test_trigger()
-(1 row)
+      Trigger      | Type |               Function               |                                                               Definition                                                               
+-------------------+------+--------------------------------------+----------------------------------------------------------------------------------------------------------------------------------------
+ restore_trigger   |    7 | test_trigger                         |  restore_trigger BEFORE INSERT ON test_schema."two_Partitions" FOR EACH ROW EXECUTE PROCEDURE test_trigger()
+ ts_insert_blocker |    7 | _timescaledb_internal.insert_blocker |  ts_insert_blocker BEFORE INSERT ON test_schema."two_Partitions" FOR EACH ROW EXECUTE PROCEDURE _timescaledb_internal.insert_blocker()
+(2 rows)
 
 SELECT * FROM test.show_triggers('_timescaledb_internal._hyper_1_1_chunk');
      Trigger     | Type |   Function   |                                                       Definition                                                       
@@ -255,15 +256,11 @@ SELECT * FROM _timescaledb_catalog.chunk_constraint;
 (12 rows)
 
 \c postgres :ROLE_SUPERUSER
-\! pg_dump -h localhost -U :ROLE_SUPERUSER -Fc single > dump/single.sql
-pg_dump: [archiver (db)] connection to database "single" failed: FATAL:  role ":ROLE_SUPERUSER" does not exist
-\! dropdb -h localhost -U :ROLE_SUPERUSER single
-dropdb: could not connect to database template1: FATAL:  role ":ROLE_SUPERUSER" does not exist
-\! createdb -h localhost -U :ROLE_SUPERUSER single
-createdb: could not connect to database template1: FATAL:  role ":ROLE_SUPERUSER" does not exist
+\! pg_dump -h localhost -U super_user -Fc single > dump/single.sql
+\! dropdb -h localhost -U super_user single
+\! createdb -h localhost -U super_user single
 ALTER DATABASE single SET timescaledb.restoring='on';
-\! pg_restore -h localhost -U :ROLE_SUPERUSER -d single dump/single.sql
-pg_restore: [archiver] input file is too short (read 0, expected 5)
+\! pg_restore -h localhost -U super_user -d single dump/single.sql
 \c single
 -- Set to OFF for future DB sessions.
 ALTER DATABASE single SET timescaledb.restoring='off';
@@ -314,27 +311,27 @@ SELECT * FROM test.show_columns('_timescaledb_internal._hyper_1_1_chunk');
 SELECT * FROM test.show_indexes('"test_schema"."two_Partitions"');
                           Index                          |             Columns             | Expr | Unique | Primary | Exclusion | Tablespace 
 ---------------------------------------------------------+---------------------------------+------+--------+---------+-----------+------------
+ test_schema.timecustom_device_id_series_2_key           | {timeCustom,device_id,series_2} |      | t      | f       | f         | 
  test_schema."two_Partitions_device_id_timeCustom_idx"   | {device_id,timeCustom}          |      | f      | f       | f         | 
+ test_schema."two_Partitions_timeCustom_device_id_idx"   | {timeCustom,device_id}          |      | f      | f       | f         | 
+ test_schema."two_Partitions_timeCustom_idx"             | {timeCustom}                    |      | f      | f       | f         | 
  test_schema."two_Partitions_timeCustom_series_0_idx"    | {timeCustom,series_0}           |      | f      | f       | f         | 
  test_schema."two_Partitions_timeCustom_series_1_idx"    | {timeCustom,series_1}           |      | f      | f       | f         | 
  test_schema."two_Partitions_timeCustom_series_2_idx"    | {timeCustom,series_2}           |      | f      | f       | f         | 
  test_schema."two_Partitions_timeCustom_series_bool_idx" | {timeCustom,series_bool}        |      | f      | f       | f         | 
- test_schema."two_Partitions_timeCustom_device_id_idx"   | {timeCustom,device_id}          |      | f      | f       | f         | 
- test_schema."two_Partitions_timeCustom_idx"             | {timeCustom}                    |      | f      | f       | f         | 
- test_schema.timecustom_device_id_series_2_key           | {timeCustom,device_id,series_2} |      | t      | f       | f         | 
 (8 rows)
 
 SELECT * FROM test.show_indexes('_timescaledb_internal._hyper_1_1_chunk');
                                        Index                                        |             Columns             | Expr | Unique | Primary | Exclusion | Tablespace 
 ------------------------------------------------------------------------------------+---------------------------------+------+--------+---------+-----------+------------
+ _timescaledb_internal."1_1_timecustom_device_id_series_2_key"                      | {timeCustom,device_id,series_2} |      | t      | f       | f         | 
  _timescaledb_internal."_hyper_1_1_chunk_two_Partitions_device_id_timeCustom_idx"   | {device_id,timeCustom}          |      | f      | f       | f         | 
+ _timescaledb_internal."_hyper_1_1_chunk_two_Partitions_timeCustom_device_id_idx"   | {timeCustom,device_id}          |      | f      | f       | f         | 
+ _timescaledb_internal."_hyper_1_1_chunk_two_Partitions_timeCustom_idx"             | {timeCustom}                    |      | f      | f       | f         | 
  _timescaledb_internal."_hyper_1_1_chunk_two_Partitions_timeCustom_series_0_idx"    | {timeCustom,series_0}           |      | f      | f       | f         | 
  _timescaledb_internal."_hyper_1_1_chunk_two_Partitions_timeCustom_series_1_idx"    | {timeCustom,series_1}           |      | f      | f       | f         | 
  _timescaledb_internal."_hyper_1_1_chunk_two_Partitions_timeCustom_series_2_idx"    | {timeCustom,series_2}           |      | f      | f       | f         | 
  _timescaledb_internal."_hyper_1_1_chunk_two_Partitions_timeCustom_series_bool_idx" | {timeCustom,series_bool}        |      | f      | f       | f         | 
- _timescaledb_internal."_hyper_1_1_chunk_two_Partitions_timeCustom_device_id_idx"   | {timeCustom,device_id}          |      | f      | f       | f         | 
- _timescaledb_internal."_hyper_1_1_chunk_two_Partitions_timeCustom_idx"             | {timeCustom}                    |      | f      | f       | f         | 
- _timescaledb_internal."1_1_timecustom_device_id_series_2_key"                      | {timeCustom,device_id,series_2} |      | t      | f       | f         | 
 (8 rows)
 
 SELECT * FROM test.show_constraints('"test_schema"."two_Partitions"');
@@ -352,10 +349,11 @@ SELECT * FROM test.show_constraints('_timescaledb_internal._hyper_1_1_chunk');
 (3 rows)
 
 SELECT * FROM test.show_triggers('"test_schema"."two_Partitions"');
-     Trigger     | Type |   Function   |                                                  Definition                                                  
------------------+------+--------------+--------------------------------------------------------------------------------------------------------------
- restore_trigger |    7 | test_trigger |  restore_trigger BEFORE INSERT ON test_schema."two_Partitions" FOR EACH ROW EXECUTE PROCEDURE test_trigger()
-(1 row)
+      Trigger      | Type |               Function               |                                                               Definition                                                               
+-------------------+------+--------------------------------------+----------------------------------------------------------------------------------------------------------------------------------------
+ restore_trigger   |    7 | test_trigger                         |  restore_trigger BEFORE INSERT ON test_schema."two_Partitions" FOR EACH ROW EXECUTE PROCEDURE test_trigger()
+ ts_insert_blocker |    7 | _timescaledb_internal.insert_blocker |  ts_insert_blocker BEFORE INSERT ON test_schema."two_Partitions" FOR EACH ROW EXECUTE PROCEDURE _timescaledb_internal.insert_blocker()
+(2 rows)
 
 SELECT * FROM test.show_triggers('_timescaledb_internal._hyper_1_1_chunk');
      Trigger     | Type |   Function   |                                                       Definition                                                       

diff --git a/test/sql/create_hypertable.sql b/test/sql/create_hypertable.sql
@@ -52,8 +52,8 @@ GRANT CREATE ON SCHEMA chunk_schema TO :ROLE_DEFAULT_PERM_USER;
 SET ROLE :ROLE_DEFAULT_PERM_USER;
 select * from create_hypertable('test_schema.test_table', 'time', 'device_id', 2, chunk_time_interval=>_timescaledb_internal.interval_to_usec('1 month'), associated_schema_name => 'chunk_schema');
 
--- Check that the insert block trigger exists (cannot show name or definition since they contain OID)
-SELECT "Function" FROM test.show_triggers('test_schema.test_table', show_internal => true);
+-- Check that the insert block trigger exists
+SELECT * FROM test.show_triggers('test_schema.test_table');
 
 SELECT * FROM _timescaledb_internal.get_create_command('test_table');
 

diff --git a/test/sql/pg_dump.sql b/test/sql/pg_dump.sql
@@ -69,11 +69,11 @@ SELECT * FROM _timescaledb_catalog.chunk_constraint;
 
 \c postgres :ROLE_SUPERUSER
 
-\! pg_dump -h localhost -U :ROLE_SUPERUSER -Fc single > dump/single.sql
-\! dropdb -h localhost -U :ROLE_SUPERUSER single
-\! createdb -h localhost -U :ROLE_SUPERUSER single
+\! pg_dump -h localhost -U super_user -Fc single > dump/single.sql
+\! dropdb -h localhost -U super_user single
+\! createdb -h localhost -U super_user single
 ALTER DATABASE single SET timescaledb.restoring='on';
-\! pg_restore -h localhost -U :ROLE_SUPERUSER -d single dump/single.sql
+\! pg_restore -h localhost -U super_user -d single dump/single.sql
 \c single
 
 -- Set to OFF for future DB sessions.

diff --git a/test/sql/utils/testsupport.sql b/test/sql/utils/testsupport.sql
@@ -121,7 +121,7 @@ $BODY$
     c.contype,
     array(SELECT attname FROM pg_attribute a, unnest(conkey) k WHERE a.attrelid = rel AND k = a.attnum),
     c.conindid::regclass,
-    c.consrc,
+    pg_get_expr(c.conbin, c.conrelid),
     c.condeferrable,
     c.condeferred,
     c.convalidated
@@ -156,7 +156,7 @@ BEGIN
     c.contype,
     array(SELECT attname FROM pg_attribute a, unnest(conkey) k WHERE a.attrelid = cl.oid AND k = a.attnum),
     c.conindid::regclass,
-    c.consrc,
+    pg_get_expr(c.conbin, c.conrelid),
     c.condeferrable,
     c.condeferred,
     c.convalidated