From 5618df3a8998d30cff594b3844f59fbc675ff66f Mon Sep 17 00:00:00 2001 From: Alex Kasko Date: Wed, 22 Apr 2026 22:36:45 +0100 Subject: [PATCH] Fix two data races, add ThreadSan CI runs This PR fixes two minor data races and adds thread sanitizer test runs to CI. --- .github/workflows/IntegrationTests.yml | 9 ++++++++- src/include/storage/postgres_table_entry.hpp | 2 +- src/postgres_scanner.cpp | 1 + src/storage/postgres_insert.cpp | 2 +- src/storage/postgres_table_entry.cpp | 7 ++++--- 5 files changed, 15 insertions(+), 6 deletions(-) diff --git a/.github/workflows/IntegrationTests.yml b/.github/workflows/IntegrationTests.yml index 80f30c449..7159e7e40 100644 --- a/.github/workflows/IntegrationTests.yml +++ b/.github/workflows/IntegrationTests.yml @@ -257,6 +257,10 @@ jobs: needs: linux-tests runs-on: ubuntu-latest + strategy: + matrix: + threadsan: [0, 1] + env: CMAKE_BUILD_PARALLEL_LEVEL: 2 CC: 'ccache gcc' @@ -286,7 +290,7 @@ jobs: working-directory: ./duckdb run: | DUCKDB_VERSION=$(git rev-parse --short HEAD) - KEY="${{ runner.os }}-${{ runner.arch }}-$DUCKDB_VERSION"-relassert + KEY="${{ runner.os }}-${{ runner.arch }}-$DUCKDB_VERSION"-relassert-${{ matrix.threadsan }} echo "value=${KEY}" >> "${GITHUB_OUTPUT}" - name: Restore Cache @@ -311,8 +315,11 @@ jobs: vcpkgGitCommitId: 84bab45d415d22042bd0b9081aea57f362da3f35 - name: Build extension + env: + THREADSAN: ${{ matrix.threadsan }} run: | make relassert + cat ./build/relassert/src/CMakeFiles/duckdb.dir/flags.make cd build ln -s relassert release diff --git a/src/include/storage/postgres_table_entry.hpp b/src/include/storage/postgres_table_entry.hpp index 02da22ff9..b97677c39 100644 --- a/src/include/storage/postgres_table_entry.hpp +++ b/src/include/storage/postgres_table_entry.hpp @@ -64,7 +64,7 @@ class PostgresTableEntry : public TableCatalogEntry { //! We would in this case remap them to "ID" and "id:1", while postgres_names store the original names vector postgres_names; //! The approximate number of pages a table consumes in Postgres - int64_t approx_num_pages; + std::atomic approx_num_pages; }; } // namespace duckdb diff --git a/src/postgres_scanner.cpp b/src/postgres_scanner.cpp index 73023cfeb..6679746cd 100644 --- a/src/postgres_scanner.cpp +++ b/src/postgres_scanner.cpp @@ -451,6 +451,7 @@ static unique_ptr GetLocalState(ClientContext &context, } if (bind_data.pages_approx == 0 || bind_data.requires_materialization) { PostgresInitInternal(context, &bind_data, *local_state, 0, POSTGRES_TID_MAX); + lock_guard parallel_lock(gstate.lock); gstate.page_idx = POSTGRES_TID_MAX; } else if (!PostgresParallelStateNext(context, input.bind_data.get(), *local_state, gstate)) { local_state->done = true; diff --git a/src/storage/postgres_insert.cpp b/src/storage/postgres_insert.cpp index 6b129061d..5310b1d53 100644 --- a/src/storage/postgres_insert.cpp +++ b/src/storage/postgres_insert.cpp @@ -141,7 +141,7 @@ SinkFinalizeType PostgresInsert::Finalize(Pipeline &pipeline, Event &event, Clie idx_t bytes_per_page = 8192; idx_t bytes_per_row = gstate.table.GetColumns().LogicalColumnCount() * 8; idx_t rows_per_page = MaxValue(1, bytes_per_page / bytes_per_row); - gstate.table.approx_num_pages += gstate.insert_count / rows_per_page; + gstate.table.approx_num_pages.fetch_add(gstate.insert_count / rows_per_page, std::memory_order_acq_rel); return SinkFinalizeType::READY; } diff --git a/src/storage/postgres_table_entry.cpp b/src/storage/postgres_table_entry.cpp index 0ce84c86a..a746a456b 100644 --- a/src/storage/postgres_table_entry.cpp +++ b/src/storage/postgres_table_entry.cpp @@ -18,14 +18,14 @@ PostgresTableEntry::PostgresTableEntry(Catalog &catalog, SchemaCatalogEntry &sch postgres_types.push_back(PostgresUtils::CreateEmptyPostgresType(col.GetType())); postgres_names.push_back(col.GetName()); } - approx_num_pages = 0; + approx_num_pages.store(0, std::memory_order_release); } PostgresTableEntry::PostgresTableEntry(Catalog &catalog, SchemaCatalogEntry &schema, PostgresTableInfo &info) : TableCatalogEntry(catalog, schema, *info.create_info), postgres_types(std::move(info.postgres_types)), postgres_names(std::move(info.postgres_names)) { D_ASSERT(postgres_types.size() == columns.LogicalColumnCount()); - approx_num_pages = info.approx_num_pages; + approx_num_pages.store(info.approx_num_pages, std::memory_order_release); } unique_ptr PostgresTableEntry::GetStatistics(ClientContext &context, column_t column_id) { @@ -54,7 +54,8 @@ TableFunction PostgresTableEntry::GetScanFunction(ClientContext &context, unique result->names = postgres_names; result->postgres_types = postgres_types; result->read_only = transaction.IsReadOnly(); - PostgresScanFunction::PrepareBind(pg_catalog.GetPostgresVersion(), context, *result, approx_num_pages); + PostgresScanFunction::PrepareBind(pg_catalog.GetPostgresVersion(), context, *result, + approx_num_pages.load(std::memory_order_acquire)); bind_data = std::move(result); auto function = PostgresScanFunction();