From 02fce36aaab658ccc188a11350b2115f86156666 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 27 Oct 2022 15:59:31 +0400 Subject: [PATCH 1/2] Do not clobber the baserel cache on UDF error The baserel cache should only be allocated and freed by the top-level query. --- CHANGELOG.md | 2 ++ src/planner/planner.c | 48 ++++++++++++++++++++------------- test/expected/baserel_cache.out | 28 +++++++++++++++++++ test/sql/CMakeLists.txt | 1 + test/sql/baserel_cache.sql | 27 +++++++++++++++++++ 5 files changed, 88 insertions(+), 18 deletions(-) create mode 100644 test/expected/baserel_cache.out create mode 100644 test/sql/baserel_cache.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index c9e6d9c0e20..ffc94fa7402 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/src/planner/planner.c b/src/planner/planner.c index 9190836c451..d1ba4bca0dc 100644 --- a/src/planner/planner.c +++ b/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); diff --git a/test/expected/baserel_cache.out b/test/expected/baserel_cache.out new file mode 100644 index 00000000000..760d525f1ff --- /dev/null +++ b/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) + diff --git a/test/sql/CMakeLists.txt b/test/sql/CMakeLists.txt index ad3a9fe8f9b..7fc24bb281d 100644 --- a/test/sql/CMakeLists.txt +++ b/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 diff --git a/test/sql/baserel_cache.sql b/test/sql/baserel_cache.sql new file mode 100644 index 00000000000..611c5249a4e --- /dev/null +++ b/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, ''); From 175029c9cfe3bee5b1600a6db624f5b1a2c5c7ae Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 27 Oct 2022 18:04:27 +0400 Subject: [PATCH 2/2] Enable and fix -Wclobbered The one in job_stat.c could probably lead to errors. --- CMakeLists.txt | 6 +++++- src/bgw/job_stat.c | 15 +++++++++++---- src/chunk.c | 12 +++++++++--- src/compression_with_clause.c | 4 ++-- src/telemetry/telemetry.c | 3 ++- test/src/bgw/log.c | 4 ++-- tsl/src/partialize_finalize.c | 4 +++- tsl/src/remote/async.c | 2 ++ 8 files changed, 36 insertions(+), 14 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 62167e56ef0..e99fc6f19c3 100644 --- a/CMakeLists.txt +++ b/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() diff --git a/src/bgw/job_stat.c b/src/bgw/job_stat.c index 6fc8a6098ba..b560b88798a 100644 --- a/src/bgw/job_stat.c +++ b/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); diff --git a/src/chunk.c b/src/chunk.c index a2ec016cec0..ce0aaad8568 100644 --- a/src/chunk.c +++ b/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; diff --git a/src/compression_with_clause.c b/src/compression_with_clause.c index f0e6572fa06..843a8f9f12f 100644 --- a/src/compression_with_clause.c +++ b/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) { diff --git a/src/telemetry/telemetry.c b/src/telemetry/telemetry.c index f6142ba5328..bf2b98e7c62 100644 --- a/src/telemetry/telemetry.c +++ b/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; diff --git a/test/src/bgw/log.c b/test/src/bgw/log.c index 50ba53dcdef..0bda01ff4e2 100644 --- a/test/src/bgw/log.c +++ b/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(); diff --git a/tsl/src/partialize_finalize.c b/tsl/src/partialize_finalize.c index c6fce581d86..e4bdaaab763 100644 --- a/tsl/src/partialize_finalize.c +++ b/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; diff --git a/tsl/src/remote/async.c b/tsl/src/remote/async.c index 56486546568..e46d14ddb35 100644 --- a/tsl/src/remote/async.c +++ b/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; }