Do not clobber the baserel cache on UDF error

CHANGELOG.md

@@ -26,9 +26,11 @@ argument or resolve the type ambiguity by casting to the intended type.
 * #4756 Improve compression job IO performance
 * #4807 Fix segmentation fault during INSERT into compressed hypertable.
 * #4840 Fix performance regressions in the copy code
+* #4823 Fix a crash that could occur when using nested user-defined functions with hypertables.
 
 **Thanks**
 * @jvanns for reporting hypertable FK reference to vanilla PostgreSQL partitioned table doesn't seem to work
+* @jflambert for reporting a crash with nested user-defined functions.
 
 ## 2.8.1 (2022-10-06)
 

CMakeLists.txt

@@ -251,9 +251,13 @@ if(CMAKE_C_COMPILER_ID MATCHES "GNU|AppleClang|Clang")
     add_compile_options(-Wno-strict-overflow)
   endif()
 
+  # -Wclobbered produces false positives on gcc < 9
+  if(CMAKE_COMPILER_IS_GNUCC AND CMAKE_C_COMPILER_VERSION VERSION_LESS 9)
+    add_compile_options(-Wno-clobbered)
+  endif()
+
   if(CMAKE_COMPILER_IS_GNUCC)
     add_compile_options(
-      -Wno-clobbered
       # Seems to be broken in GCC 11 with designated initializers.
       -Wno-missing-field-initializers)
   endif()

src/bgw/job_stat.c

@@ -288,14 +288,21 @@ calculate_next_start_on_failure(TimestampTz finish_time, int consecutive_failure
 								bool launch_failure)
 {
 	float8 jitter = calculate_jitter_percent();
-	/* consecutive failures includes this failure */
-	TimestampTz res = 0;
+
+	/*
+	 * Have to be declared volatile because they are modified between
+	 * setjmp/longjmp calls.
+	 */
+	volatile TimestampTz res = 0;
 	volatile bool res_set = false;
-	TimestampTz last_finish = finish_time;
+	volatile TimestampTz last_finish = finish_time;
+
+	/* consecutive failures includes this failure */
 	float8 multiplier = (consecutive_failures > MAX_FAILURES_MULTIPLIER ? MAX_FAILURES_MULTIPLIER :
 																		  consecutive_failures);
-	MemoryContext oldctx;
 	Assert(consecutive_failures > 0 && multiplier < 63);
+
+	MemoryContext oldctx;
 	/* 2^(consecutive_failures) - 1, at most 2^20 */
 	int64 max_slots = (INT64CONST(1) << (int64) multiplier) - INT64CONST(1);
 	int64 rand_backoff = random() % (max_slots * USECS_PER_SEC);

src/chunk.c

@@ -3849,7 +3849,6 @@ ts_chunk_do_drop_chunks(Hypertable *ht, int64 older_than, int64 newer_than, int3
 	const char *schema_name, *table_name;
 	const int32 hypertable_id = ht->fd.id;
 	bool has_continuous_aggs;
-	List *data_nodes = NIL;
 	const MemoryContext oldcontext = CurrentMemoryContext;
 	ScanTupLock tuplock = {
 		.waitpolicy = LockWaitBlock,
@@ -3949,6 +3948,7 @@ ts_chunk_do_drop_chunks(Hypertable *ht, int64 older_than, int64 newer_than, int3
 		}
 	}
 
+	List *data_nodes = NIL;
 	List *dropped_chunk_names = NIL;
 	for (uint64 i = 0; i < num_chunks; i++)
 	{
@@ -4190,8 +4190,14 @@ ts_chunk_drop_chunks(PG_FUNCTION_ARGS)
 	List *dc_temp = NIL;
 	List *dc_names = NIL;
 	Oid relid = PG_ARGISNULL(0) ? InvalidOid : PG_GETARG_OID(0);
-	int64 older_than = PG_INT64_MAX;
-	int64 newer_than = PG_INT64_MIN;
+
+	/*
+	 * Marked volatile to suppress the -Wclobbered warning. The warning is
+	 * actually incorrect because these values are not used after longjmp.
+	 */
+	volatile int64 older_than = PG_INT64_MAX;
+	volatile int64 newer_than = PG_INT64_MIN;
+
 	bool verbose;
 	int elevel;
 	List *data_node_oids = NIL;

src/compression_with_clause.c

@@ -76,7 +76,6 @@ parse_segment_collist(char *inpstr, Hypertable *hypertable)
 	List *parsed;
 	ListCell *lc;
 	SelectStmt *select;
-	List *collist = NIL;
 	RawStmt *raw;
 
 	if (strlen(inpstr) == 0)
@@ -118,6 +117,7 @@ parse_segment_collist(char *inpstr, Hypertable *hypertable)
 	if (select->sortClause != NIL)
 		throw_segment_by_error(inpstr);
 
+	List *collist = NIL;
 	short index = 0;
 	foreach (lc, select->groupClause)
 	{
@@ -161,7 +161,6 @@ parse_order_collist(char *inpstr, Hypertable *hypertable)
 	List *parsed;
 	ListCell *lc;
 	SelectStmt *select;
-	List *collist = NIL;
 	RawStmt *raw;
 
 	if (strlen(inpstr) == 0)
@@ -202,6 +201,7 @@ parse_order_collist(char *inpstr, Hypertable *hypertable)
 	if (select->groupClause != NIL)
 		throw_order_by_error(inpstr);
 
+	List *collist = NIL;
 	short index = 0;
 	foreach (lc, select->sortClause)
 	{

src/planner/planner.c

@@ -158,11 +158,7 @@ void
 add_baserel_cache_entry_for_chunk(Oid chunk_reloid, uint32 chunk_status, Hypertable *hypertable)
 {
 	Assert(hypertable != NULL);
-
-	if (ts_baserel_info == NULL)
-	{
-		return;
-	}
+	Assert(ts_baserel_info != NULL);
 
 	bool found = false;
 	BaserelInfoEntry *entry = BaserelInfo_insert(ts_baserel_info, chunk_reloid, &found);
@@ -444,8 +440,12 @@ timescaledb_planner(Query *parse, int cursor_opts, ParamListInfo bound_params)
 {
 	PlannedStmt *stmt;
 	ListCell *lc;
-	bool reset_fetcher_type = false;
-	bool reset_baserel_info = false;
+	/*
+	 * Volatile is needed because these are the local variables that are
+	 * modified between setjmp/longjmp calls.
+	 */
+	volatile bool reset_fetcher_type = false;
+	volatile bool reset_baserel_info = false;
 
 	/*
 	 * If we are in an aborted transaction, reject all queries.
@@ -553,6 +553,7 @@ timescaledb_planner(Query *parse, int cursor_opts, ParamListInfo bound_params)
 				 * or portal memory contexts could also be suitable, but they
 				 * don't exist for SPI calls.
 				 */
+				MemoryContextStats(CurrentMemoryContext);
 				ts_baserel_info = BaserelInfo_create(CurrentMemoryContext,
 													 /* nelements = */ 1,
 													 /* private_data = */ NULL);
@@ -593,23 +594,34 @@ timescaledb_planner(Query *parse, int cursor_opts, ParamListInfo bound_params)
 				if (subplan)
 					ts_hypertable_modify_fixup_tlist(subplan);
 			}
+		}
 
-			if (reset_fetcher_type)
-			{
-				ts_data_node_fetcher_scan_type = AutoFetcherType;
-			}
+		if (reset_baserel_info)
+		{
+			Assert(ts_baserel_info != NULL);
+			BaserelInfo_destroy(ts_baserel_info);
+			ts_baserel_info = NULL;
+		}
 
-			if (reset_baserel_info && ts_baserel_info)
-			{
-				BaserelInfo_destroy(ts_baserel_info);
-				ts_baserel_info = NULL;
-			}
+		if (reset_fetcher_type)
+		{
+			ts_data_node_fetcher_scan_type = AutoFetcherType;
 		}
 	}
 	PG_CATCH();
 	{
-		ts_baserel_info = NULL;
-		ts_data_node_fetcher_scan_type = AutoFetcherType;
+		if (reset_baserel_info)
+		{
+			Assert(ts_baserel_info != NULL);
+			BaserelInfo_destroy(ts_baserel_info);
+			ts_baserel_info = NULL;
+		}
+
+		if (reset_fetcher_type)
+		{
+			ts_data_node_fetcher_scan_type = AutoFetcherType;
+		}
+
 		/* Pop the cache, but do not release since caches are auto-released on
 		 * error */
 		planner_hcache_pop(false);

src/telemetry/telemetry.c

@@ -759,7 +759,8 @@ ts_telemetry_main(const char *host, const char *path, const char *service)
 	Connection *conn;
 	HttpRequest *req;
 	HttpResponseState *rsp;
-	bool started = false;
+	/* Declared volatile to suppress the incorrect -Wclobbered warning. */
+	volatile bool started = false;
 	bool snapshot_set = false;
 	const char *volatile json = NULL;
 

test/expected/baserel_cache.out

@@ -0,0 +1,28 @@
+-- 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.
+-- Test that the baserel cache is not clobbered if there's an error
+-- in a SQL function.
+CREATE TABLE valid_ids
+(
+  id UUID PRIMARY KEY
+);
+CREATE FUNCTION DEFAULT_UUID(TEXT DEFAULT '') RETURNS UUID AS $$
+  BEGIN
+    RETURN COALESCE($1, '')::UUID;
+  EXCEPTION WHEN invalid_text_representation THEN
+    RETURN '00000000-0000-0000-0000-000000000000';
+  END;
+$$ LANGUAGE PLPGSQL IMMUTABLE;
+CREATE FUNCTION KNOWN_ID(UUID, TEXT) RETURNS UUID AS $$
+  SELECT COALESCE(
+    (SELECT id FROM valid_ids WHERE id = $1),
+    DEFAULT_UUID()
+  );
+$$ LANGUAGE SQL;
+SELECT KNOWN_ID(NULL, ''), KNOWN_ID(NULL, '');
+               known_id               |               known_id               
+--------------------------------------+--------------------------------------
+ 00000000-0000-0000-0000-000000000000 | 00000000-0000-0000-0000-000000000000
+(1 row)
+

test/sql/CMakeLists.txt

@@ -3,6 +3,7 @@ include(GenerateTestSchedule)
 set(TEST_FILES
     alter.sql
     alternate_users.sql
+    baserel_cache.sql
     broken_tables.sql
     chunks.sql
     chunk_adaptive.sql

test/sql/baserel_cache.sql

@@ -0,0 +1,27 @@
+-- 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.
+
+-- Test that the baserel cache is not clobbered if there's an error
+-- in a SQL function.
+CREATE TABLE valid_ids
+(
+  id UUID PRIMARY KEY
+);
+
+CREATE FUNCTION DEFAULT_UUID(TEXT DEFAULT '') RETURNS UUID AS $$
+  BEGIN
+    RETURN COALESCE($1, '')::UUID;
+  EXCEPTION WHEN invalid_text_representation THEN
+    RETURN '00000000-0000-0000-0000-000000000000';
+  END;
+$$ LANGUAGE PLPGSQL IMMUTABLE;
+
+CREATE FUNCTION KNOWN_ID(UUID, TEXT) RETURNS UUID AS $$
+  SELECT COALESCE(
+    (SELECT id FROM valid_ids WHERE id = $1),
+    DEFAULT_UUID()
+  );
+$$ LANGUAGE SQL;
+
+SELECT KNOWN_ID(NULL, ''), KNOWN_ID(NULL, '');

test/src/bgw/log.c

@@ -69,8 +69,6 @@ static emit_log_hook_type prev_emit_log_hook = NULL;
 static void
 emit_log_hook_callback(ErrorData *edata)
 {
-	bool started_txn = false;
-
 	/*
 	 * once proc_exit has started we may no longer be able to start transactions
 	 */
@@ -97,6 +95,8 @@ emit_log_hook_callback(ErrorData *edata)
 		 */
 		emit_log_hook = NULL;
 
+		bool started_txn = false;
+
 		if (!IsTransactionState())
 		{
 			StartTransactionCommand();

tsl/src/partialize_finalize.c

@@ -245,9 +245,11 @@ sanitize_serialized_partial(Oid deserialfnoid, bytea *serialized_partial)
  * deserialize from the internal format in which data is stored in bytea
  * parameter. Callers need to check deserialized_isnull . Only if this is set to false,
  * a valid value is returned.
+ * The serialized_partial argument is declared volatile to avoid the mistaken
+ * gcc-11 warning -Wclobbered.
  */
 static Datum
-inner_agg_deserialize(FACombineFnMeta *combine_meta, bytea *serialized_partial,
+inner_agg_deserialize(FACombineFnMeta *combine_meta, bytea *volatile serialized_partial,
 					  bool serialized_isnull, bool *deserialized_isnull)
 {
 	Datum deserialized = (Datum) 0;

tsl/src/remote/async.c

@@ -418,6 +418,7 @@ async_response_report_error(AsyncResponse *res, int elevel)
 					remote_result_elog(aresult->result, elevel);
 					break;
 				default:
+				{
 					PG_TRY();
 					{
 						elog(elevel, "unexpected response status %u", status);
@@ -428,6 +429,7 @@ async_response_report_error(AsyncResponse *res, int elevel)
 						PG_RE_THROW();
 					}
 					PG_END_TRY();
+				}
 			}
 			break;
 		}