Stop using the extra field for now and other Windows bugs

Something is causing a heap corruption upon setting the license key to
default when we try to use the guc extra on windows. For now stop using
it and just rerun the validation function, if we get to the assign hook
we must have a valid key, so it will never fail.

Also Fixes error message on windows;
turns out windows does not like to print NULL strings.
Don't do that.

Fixes other minor windows bugs.

appveyor.yml

@@ -140,11 +140,21 @@ test_script:
 
     docker exec -e IGNORES="bgw_db_scheduler" -e TEST_TABLESPACE1_PATH="C:\Users\$env:UserName\Documents\tablespace1\" -e TEST_TABLESPACE2_PATH="C:\Users\$env:UserName\Documents\tablespace2\" -e TEST_SPINWAIT_ITERS=10000 -e USER=postgres -it pgregress /bin/bash -c "cd /timescaledb/build && make regresschecklocal"
 
-    #TESTS=drop_extension
+    $TESTS1 = $?
+
+    # TODO can we switch these to file swapping?
+
+    Add-Content "C:\Program Files\postgresql\10\data\postgresql.conf" "timescaledb.license_key = 'E1eyJlbmRfdGltZSI6IjIwMTgtMTAtMDEgKzAwMDAiLCAic3RhcnRfdGltZSI6IjIwMTgtMDktMDEgKzAwMDAiLCAiaWQiOiI0OTBGQjI2MC1BMjkyLTRBRDktOUFBMi0wMzYwODM1NzkxQjgiLCAia2luZCI6InRyaWFsIn0K'"
+
+    Restart-Service postgresql-x64-10
+
+    docker exec -e IGNORES="bgw_db_scheduler" -e TEST_TABLESPACE1_PATH="C:\Users\$env:UserName\Documents\tablespace1\" -e TEST_TABLESPACE2_PATH="C:\Users\$env:UserName\Documents\tablespace2\" -e TEST_SPINWAIT_ITERS=10000 -e USER=postgres -it pgregress /bin/bash -c "cd /timescaledb/build && make regresschecklocal-t"
+
+    if( -not $? -or -not $TESTS1 ) { exit 1 }
 
 on_failure:
   - ps: >-
-      docker exec -it pgregress cat /timescaledb/build/test/regression.diffs /timescaledb/build/test/pgtest/regressions.diffs
+      docker exec -it pgregress cat /timescaledb/build/test/regression.diffs /timescaledb/build/tsl/test/regression.diffs /timescaledb/build/test/pgtest/regressions.diffs
 
       Get-Content -Path "C:\Program Files\postgresql\10\data\pg_log\*"
 

src/chunk.h

@@ -75,7 +75,6 @@ extern Chunk *ts_chunk_find(Hyperspace *hs, Point *p);
 extern List *ts_chunk_find_all_oids(Hyperspace *hs, List *dimension_vecs, LOCKMODE lockmode);
 extern Chunk *ts_chunk_copy(Chunk *chunk);
 extern Chunk *ts_chunk_get_by_name_with_memory_context(const char *schema_name, const char *table_name, int16 num_constraints, MemoryContext mctx, bool fail_if_not_found);
-extern Chunk *ts_chunk_get_by_relid(Oid relid, int16 num_constraints, bool fail_if_not_found);
 extern TSDLLEXPORT Chunk *ts_chunk_get_by_id(int32 id, int16 num_constraints, bool fail_if_not_found);
 extern TSDLLEXPORT Chunk *ts_chunk_get_by_relid(Oid relid, int16 num_constraints, bool fail_if_not_found);
 extern bool ts_chunk_exists(const char *schema_name, const char *table_name);

src/cross_module_fn.c

@@ -63,7 +63,7 @@ ts_reorder_chunk(PG_FUNCTION_ARGS)
  */
 
 static void
-error_no_default_fn()
+error_no_default_fn(void)
 {
 	ereport(ERROR,
 			(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -73,7 +73,7 @@ error_no_default_fn()
 }
 
 static bool
-error_no_default_fn_bool_void()
+error_no_default_fn_bool_void(void)
 {
 	error_no_default_fn();
 	return false;

src/dimension.c

@@ -115,6 +115,8 @@ dimension_type(TupleInfo *ti)
 		return DIMENSION_TYPE_OPEN;
 
 	elog(ERROR, "invalid partitioning dimension");
+	/* suppress compiler warning on MSVC */
+	return DIMENSION_TYPE_ANY;
 }
 
 static void

src/export.h

@@ -58,7 +58,7 @@
 #endif
 
 #define TS_FUNCTION_INFO_V1(fn) \
-	TSDLLEXPORT Datum fn(PG_FUNCTION_ARGS); \
+	PGDLLEXPORT Datum fn(PG_FUNCTION_ARGS); \
 	PG_FUNCTION_INFO_V1(fn)
 
 #endif							/* TIMESCALEDB_EXPORT_H */

src/hypertable_restrict_info.c

@@ -209,6 +209,8 @@ dimension_restrict_info_add(DimensionRestrictInfo *dri, int strategy, DimensionV
 			return dimension_restrict_info_closed_add((DimensionRestrictInfoClosed *) dri, strategy, values);
 		default:
 			elog(ERROR, "unknown dimension type: %d", dri->dimension->type);
+			/* supress compiler warning on MSVC */
+			return false;
 	}
 }
 

src/license_guc.c

@@ -142,15 +142,45 @@ ts_license_update_check(char **newval, void **extra, GucSource source)
 	Assert(tsl_handle != NULL);
 	Assert(tsl_validate_license_fn != NULL);
 	Assert(tsl_startup_fn != NULL);
+#ifdef WIN32
+
+	/*
+	 * freeing the guc extra causes heap corruption on windows so instead we
+	 * re-parse in the assign hook
+	 */
+	extra = NULL;
+#endif
 
 	module_can_start = DirectFunctionCall2(
 										   tsl_validate_license_fn,
 										   CStringGetDatum(*newval),
-										   PointerGetDatum(extra));
+										   PointerGetDatum(extra)
+		);
 
 	return DatumGetBool(module_can_start);
 }
 
+#ifdef WIN32
+static void *
+revalidate_license(const char *newval)
+{
+	void	   *extra = NULL;
+	void	  **extra_p = &extra;
+
+	Assert(extra == NULL);
+
+	/*
+	 * Due to windows issues we cannot use the extra parameter. Instead
+	 * re-call the validation function, if we reach this point the license
+	 * must be valid so the function cannot fail cannot fail.
+	 */
+	DirectFunctionCall2(tsl_validate_license_fn,
+						CStringGetDatum(newval),
+						PointerGetDatum(extra_p));
+	return extra;
+}
+#endif
+
 /*
  * `ts_license_on_assign`
  * Links the cross-module function pointers.
@@ -161,6 +191,7 @@ ts_license_on_assign(const char *newval, void *extra)
 	if (!can_load)
 		return;
 
+	Assert(newval != NULL);
 	Assert(TS_LICENSE_TYPE_IS_VALID(newval));
 	if (TS_LICENSE_IS_APACHE_ONLY(newval))
 	{
@@ -170,7 +201,14 @@ ts_license_on_assign(const char *newval, void *extra)
 		return;
 	}
 
+	Assert(tsl_handle != NULL);
+	Assert(tsl_startup_fn != NULL);
 	DirectFunctionCall1(tsl_startup_fn, CharGetDatum(0));
+#ifdef WIN32
+	Assert(extra == NULL);
+	extra = revalidate_license(newval);
+#endif
+	Assert(extra != NULL);
 	ts_cm_functions->tsl_license_on_assign(newval, extra);
 }
 
@@ -245,7 +283,7 @@ loading_failed:
 
 /* SQL functions */
 
-Datum
+PGDLLEXPORT Datum
 ts_tsl_loaded(PG_FUNCTION_ARGS)
 {
 	if (TS_CURRENT_LICENSE_IS_APACHE_ONLY())
@@ -254,7 +292,7 @@ ts_tsl_loaded(PG_FUNCTION_ARGS)
 	PG_RETURN_BOOL(ts_cm_functions->check_tsl_loaded());
 }
 
-Datum
+PGDLLEXPORT Datum
 ts_enterprise_enabled(PG_FUNCTION_ARGS)
 {
 	if (TS_CURRENT_LICENSE_IS_APACHE_ONLY())
@@ -264,7 +302,7 @@ ts_enterprise_enabled(PG_FUNCTION_ARGS)
 }
 
 
-Datum
+PGDLLEXPORT Datum
 ts_current_license_key(PG_FUNCTION_ARGS)
 {
 	Assert(ts_guc_license_key != NULL);
@@ -276,7 +314,7 @@ ts_current_license_key(PG_FUNCTION_ARGS)
  * Apache only. This function allows us to bypass the test that usually disables
  * that.
  */
-Datum
+PGDLLEXPORT Datum
 ts_allow_downgrade_to_apache(PG_FUNCTION_ARGS)
 {
 	downgrade_to_apache_enabled = true;

src/utils.c

@@ -194,6 +194,9 @@ ts_interval_from_now_to_internal(Datum interval, Oid type_oid)
 					 ));
 
 	}
+	/* suppress compiler warnings on MSVC */
+	Assert(false);
+	return 0;
 }
 
 /* Returns approximate period in microseconds */

tsl/src/bgw_policy/reorder_api.c

@@ -152,7 +152,12 @@ reorder_remove_policy(PG_FUNCTION_ARGS)
 					 errmsg("cannot remove reorder policy, no such policy exists")));
 		else
 		{
-			ereport(NOTICE, (errmsg("reorder policy does not exist on hypertable \"%s\", skipping", get_rel_name(hypertable_oid))));
+			char	   *hypertable_name = get_rel_name(hypertable_oid);
+
+			if (hypertable_name != NULL)
+				ereport(NOTICE, (errmsg("reorder policy does not exist on hypertable \"%s\", skipping", hypertable_name)));
+			else
+				ereport(NOTICE, (errmsg("reorder policy does not exist on unnamed hypertable, skipping")));
 			PG_RETURN_NULL();
 		}
 	}

tsl/src/gapfill/exec.c

@@ -86,7 +86,7 @@ gapfill_datum_get_internal(Datum value, Oid type)
 			 * for other datatypes
 			 */
 			Assert(false);
-			break;
+			return 0;
 	}
 }
 
@@ -116,7 +116,7 @@ gapfill_internal_get_datum(int64 value, Oid type)
 			 * for other datatypes
 			 */
 			Assert(false);
-			break;
+			return Int64GetDatum(0);
 	}
 }
 

tsl/src/license.c

@@ -80,24 +80,32 @@ tsl_license_update_check(PG_FUNCTION_ARGS)
 	bool		license_deserialized;
 	char	   *license_key = NULL;
 	LicenseInfo **guc_extra = NULL;
+	LicenseInfo license_info = {0};
 
 	Assert(!PG_ARGISNULL(0));
 	Assert(!PG_ARGISNULL(1));
 
 	license_key = PG_GETARG_CSTRING(0);
 	guc_extra = (LicenseInfo **) PG_GETARG_POINTER(1);
-	Assert(guc_extra != NULL);
 
-	/*
-	 * According to the postgres guc documentation, string `extra`s MUST be
-	 * allocated with `malloc`. (postgres attempts to `free` unneeded
-	 * guc-extras with the system `free` upon transaction commit, and there's
-	 * no guarantee that any MemoryContext uses the correct allocator.)
-	 */
-	*guc_extra = malloc(sizeof(LicenseInfo));
+	license_deserialized = license_deserialize_enterprise(license_key, &license_info);
+	if (guc_extra != NULL)
+	{
+		/*
+		 * According to the postgres guc documentation, string `extra`s MUST
+		 * be allocated with `malloc`. (postgres attempts to `free` unneeded
+		 * guc-extras with the system `free` upon transaction commit, and
+		 * there's no guarantee that any MemoryContext uses the correct
+		 * allocator.) However, there is a bug in Windows which causes heap
+		 * corruption if we try to do that. Instead we currently rerun this
+		 * function during the assign, with the assumption that since we
+		 * called the function during the check hook it cannot fail now.
+		 */
+		*guc_extra = malloc(sizeof(LicenseInfo));
+		memcpy(*guc_extra, &license_info, sizeof(LicenseInfo));
+	}
 
-	license_deserialized = license_deserialize_enterprise(license_key, *guc_extra);
-	PG_RETURN_BOOL(license_deserialized && validate_license_info(*guc_extra));
+	PG_RETURN_BOOL(license_deserialized && validate_license_info(&license_info));
 }
 
 /*
@@ -106,7 +114,7 @@ tsl_license_update_check(PG_FUNCTION_ARGS)
 static bool
 license_deserialize_enterprise(char *license_key, LicenseInfo *license_out)
 {
-	LicenseInfo license_temp = {};
+	LicenseInfo license_temp = {0};
 	const LicenseInfo *license_info = NULL;
 	size_t		license_key_len = strlen(license_key);
 

tsl/test/expected/tsl_tables.out

@@ -275,7 +275,7 @@ ERROR:  relation "fake_table" does not exist at character 30
 \set ON_ERROR_STOP 0
 -- This should be noop
 select remove_reorder_policy(2, true);
-NOTICE:  reorder policy does not exist on hypertable "(null)", skipping
+NOTICE:  reorder policy does not exist on unnamed hypertable, skipping
  remove_reorder_policy 
 -----------------------
  

tsl/test/postgresql.conf

@@ -1,4 +1,5 @@
 shared_preload_libraries=timescaledb
 max_worker_processes=16
 timescaledb.telemetry_level=off
+# when changing this, also update the one in appveyor.yml
 timescaledb.license_key='E1eyJlbmRfdGltZSI6IjIwMTgtMTAtMDEgKzAwMDAiLCAic3RhcnRfdGltZSI6IjIwMTgtMDktMDEgKzAwMDAiLCAiaWQiOiI0OTBGQjI2MC1BMjkyLTRBRDktOUFBMi0wMzYwODM1NzkxQjgiLCAia2luZCI6InRyaWFsIn0K'