From 7f3be7ef8736005d8170d4542c5e26a4eb12a67a Mon Sep 17 00:00:00 2001 From: Gavin Brand Date: Wed, 29 Jul 2026 08:30:06 -0700 Subject: [PATCH] feat: maintain inlined-data column stats in the direct-insert writer The direct-insert path (CreateSnapshotForDirectInsert) did not maintain ducklake_table_column_stats min/max or contains_null for inlined rows, so stale zone-map bounds could mis-prune scans, a stale contains_null could drop rows outright, and the persisted stats are wrong for any DuckLake reader of the same catalog that has no reader-side guard. This was masked in our own reads by patch 006; the guard is now retired and the bug fixed in the writer where it originates. #217 fixed the same function on v1.0 by degrading the stats to NULL and named this precise version as a refactor targeting main. - Widen (never shrink, never seed) per-column min/max from each inserted batch via a vectorized PG-Datum accumulator (inline_col_stats), wired into the UNNEST/VALUES and COPY inline writers. Never-seed is per bound, not per row: neither a stats row for a column that has none (e.g. an ALTER-added column whose back-fill has not flushed), nor a missing min or max on a row that already carries the other one. A bound derived from one batch is not a bound over the table. - Maintain contains_null the same way, through the same MergeStats call (as null_count), in all three inline writers. Left stale at false it is worse than a bad bound: the optimizer folds IS NULL to false and the row vanishes, while IS NOT NULL counts it as non-NULL. Counting is not gated on the min/max eligibility test -- null-ness needs no comparison proc or canonical encoding, so unlike a bound it costs nothing to match the normal path for every column type. - Decode the persisted row through DuckLakeColumnStats::FromGlobalStats and apply the widen through MergeStats, rather than reimplementing either locally, so the comparison semantics and the any_valid rule cannot drift from the engine's. - Fold the per-column widen into a single SELECT plus one UPDATE ... FROM (VALUES ...) per snapshot, so insert cost no longer scales with column count, and skip the UPDATE entirely when no bound moved -- the steady state, where a batch falls inside the persisted bounds, now costs one round trip and writes no dead tuples. - Advance next_file_id on the inline commit, forward-porting #217's fix to main. It rotates the stats-cache key so a scan taken between two inline inserts cannot serve stale bounds, and it retires patch 006 with no replacement vendor patch. It also fixes data loss that main has today: the stale cached stats carry next_row_id, so a normal-path insert following direct inserts in one backend reuses row ids, and deleting one of those rows destroys an unrelated one. - Create the accumulator's memory context under CurTransactionContext rather than TopMemoryContext, so a PG elog(ERROR) that longjmps past the destructor does not leak it once per failed insert. - Log a DEBUG1 when a persisted bound cannot be cast to the column's current type -- reachable after a plain ALTER COLUMN TYPE -- rather than skipping that column's widen silently. The accumulator folds native PG Datums instead of building a duckdb::Value per cell. That divergence from the reuse-the-engine rule is deliberate and measured: a Value-per-cell implementation costs ~6x more (+161ms vs +25ms per 100k-row COPY, 7 columns) for byte-identical bounds. Net write-path cost is +7% at batch=1000 and +20% at batch=100, flat in column count. Retires third_party/ducklake-006-inlined-scan-stats-guard.patch, reworks inlined_scan_stats_guard to assert maintained bounds, and forward-ports #217's direct_insert_metadata test, which main never received. That test's third section asserted that the fast path degrades stats to NULL because it cannot maintain them; it now asserts the widened bounds and contains_null. Three further sections pin one column per inline writer, the min/max-ineligible types, and the inline-only table where contains_null correctly stays unknown. --- .../include/pgducklake/inline_col_stats.hpp | 54 +++ .../pgducklake_metadata_manager.hpp | 22 +- pg_ducklake/src/copy_from.cpp | 31 +- pg_ducklake/src/direct_insert.cpp | 57 ++- pg_ducklake/src/inline_col_stats.cpp | 425 +++++++++++++++++ .../src/pgducklake_metadata_manager.cpp | 279 ++++++++++- .../expected/direct_insert_metadata.out | 357 ++++++++++++++ .../expected/inlined_scan_stats_guard.out | 437 ++++++++++++++++++ pg_ducklake/test/regression/schedule | 2 + .../regression/sql/direct_insert_metadata.sql | 194 ++++++++ .../sql/inlined_scan_stats_guard.sql | 277 +++++++++++ ...ucklake-006-inlined-scan-stats-guard.patch | 20 - 12 files changed, 2125 insertions(+), 30 deletions(-) create mode 100644 pg_ducklake/include/pgducklake/inline_col_stats.hpp create mode 100644 pg_ducklake/src/inline_col_stats.cpp create mode 100644 pg_ducklake/test/regression/expected/direct_insert_metadata.out create mode 100644 pg_ducklake/test/regression/expected/inlined_scan_stats_guard.out create mode 100644 pg_ducklake/test/regression/sql/direct_insert_metadata.sql create mode 100644 pg_ducklake/test/regression/sql/inlined_scan_stats_guard.sql delete mode 100644 pg_ducklake/third_party/ducklake-006-inlined-scan-stats-guard.patch diff --git a/pg_ducklake/include/pgducklake/inline_col_stats.hpp b/pg_ducklake/include/pgducklake/inline_col_stats.hpp new file mode 100644 index 00000000..0b8a960f --- /dev/null +++ b/pg_ducklake/include/pgducklake/inline_col_stats.hpp @@ -0,0 +1,54 @@ +#pragma once + +#include "pgddb/pg/declarations.hpp" +#include "pgducklake/pgducklake_metadata_manager.hpp" + +#include +#include + +namespace pgducklake { + +/* Per-column min/max + null accumulator for a single inline-insert batch (direct insert or COPY + * FROM). + * Cells are folded as native PG Datums and compared type-correctly in PG's own value space + * (numeric/temporal/boolean via the type's btree comparison proc; VARCHAR/UUID lexicographically + * over the canonical output text) -- mirroring DuckLakeColumnStats::MergeStats without a + * per-cell text->Value->cast->ToString round-trip. Only the two surviving bounds per column are + * canonicalized (via duckdb::Value::ToString) at Finalize(), so the persisted encoding round-trips + * through the read path exactly as native DuckLake's inline write would produce it. + * CreateSnapshotForDirectInsert widens those bounds into ducklake_table_column_stats. + * The DuckDB-typed and PG-typed state lives behind a pimpl so this header stays free of DuckDB and + * postgres.h includes. */ +class InlineColStats { +public: + /* Fetches column_id + DuckLake type for the first num_cols user columns (ordered by + * column_order -- both the direct-insert and COPY paths write a prefix of the table columns + * in that order). MUST be called inside an already-open SPI connection. On any metadata + * shortfall the accumulator is inactive and no stats are maintained (existing bounds stay + * intact -- never widened into a lie). */ + InlineColStats(uint64_t table_id, int num_cols); + ~InlineColStats(); + + InlineColStats(const InlineColStats &) = delete; + InlineColStats &operator=(const InlineColStats &) = delete; + + bool ColumnEligible(int col) const; + /* Bind an eligible column's PG source type so cells can be compared natively. Call once per + * column before the row loop; a no-op for ineligible columns. If the source type has no usable + * comparison the column is silently demoted to ineligible (no stats, never a lie). Needs no SPI + * connection. */ + void SetupColumn(int col, Oid pg_source_type); + /* Fold one non-null cell, given as a PG Datum of the column's bound source type. */ + void ObserveDatum(int col, Datum value); + /* Not gated on ColumnEligible: null-ness needs no comparison proc or canonical encoding, so it + * costs nothing to match the normal path here. The min/max exclusions are not a precedent -- + * they exist only because a canonical bound for those types is not cheap to produce. */ + void ObserveNull(int col); + void Finalize(std::vector &out); + +private: + struct Impl; + std::unique_ptr impl; +}; + +} // namespace pgducklake diff --git a/pg_ducklake/include/pgducklake/pgducklake_metadata_manager.hpp b/pg_ducklake/include/pgducklake/pgducklake_metadata_manager.hpp index d13e817b..29d82abb 100644 --- a/pg_ducklake/include/pgducklake/pgducklake_metadata_manager.hpp +++ b/pg_ducklake/include/pgducklake/pgducklake_metadata_manager.hpp @@ -2,6 +2,9 @@ #include "pgddb/pg/declarations.hpp" +#include +#include + #include #include #include @@ -82,6 +85,23 @@ bool GetTableInliningInfo(Oid table_oid, uint64_t *table_id_out, uint64_t *schem uint64_t GetNextRowIdForTable(uint64_t table_id, uint64_t schema_version); uint64_t GetNextSnapshotId(); -void CreateSnapshotForDirectInsert(uint64_t snapshot_id, uint64_t table_id, int64_t rows_inserted); + +/* Per-column min/max + null contribution of a single direct-insert batch. min_value/max_value hold + * the DuckLake-canonical text encoding (duckdb::Value::ToString), so they round-trip through the read + * path's Value(text)->cast(column_type). column_type is the DuckLake type string + * (duckdb::DuckLakeTypes::FromString input) used to compare type-correctly when widening. */ +struct DirectInsertColumnStat { + uint64_t column_id = 0; + std::string column_type; + bool has_min = false; + bool has_max = false; + std::string min_value; + std::string max_value; + /* May be set with no bound at all: an all-NULL batch still has to flip contains_null. */ + uint64_t null_count = 0; +}; + +void CreateSnapshotForDirectInsert(uint64_t snapshot_id, uint64_t table_id, int64_t rows_inserted, + const std::vector &column_stats); } // namespace pgducklake diff --git a/pg_ducklake/src/copy_from.cpp b/pg_ducklake/src/copy_from.cpp index 25328d3b..c5ece0d6 100644 --- a/pg_ducklake/src/copy_from.cpp +++ b/pg_ducklake/src/copy_from.cpp @@ -4,8 +4,11 @@ #include "pgducklake/catalog_sync.hpp" #include "pgducklake/copy_from.hpp" +#include "pgducklake/inline_col_stats.hpp" #include "pgducklake/pgducklake_metadata_manager.hpp" +#include + extern "C" { #include "postgres.h" @@ -16,6 +19,7 @@ extern "C" { #include "catalog/namespace.h" #include "commands/copy.h" #include "executor/executor.h" +#include "executor/spi.h" #include "fmgr.h" #include "parser/parse_node.h" #include "utils/builtins.h" @@ -113,6 +117,26 @@ DucklakeCopyFromStdin(CopyStmt *stmt, const char *query_string) { ColumnConvInfo *conv = BuildColumnConvInfo(user_tupdesc, inlined_tupdesc); + /* Per-column min/max accumulator, widened into ducklake_table_column_stats after the batch so a + * later reader never mis-prunes the inlined rows. Constructed under a scoped SPI connection (it + * reads column metadata); its state is SPI-independent afterwards. Eligible columns stored + * natively need their own output function to canonicalize each cell. */ + if (SPI_connect() < 0) { + table_close(inlined_rel, RowExclusiveLock); + table_close(user_rel, RowExclusiveLock); + ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("SPI_connect failed"))); + } + InlineColStats col_stats(table_id, natts); + SPI_finish(); + + /* Bind the accumulator to each column's PG source type: cells fold as native Datums, compared in + * PG value space -- no per-cell text/Value/cast round-trip. */ + for (int i = 0; i < natts; i++) { + if (col_stats.ColumnEligible(i)) { + col_stats.SetupColumn(i, TupleDescAttr(user_tupdesc, i)->atttypid); + } + } + uint64_t begin_snapshot = GetNextSnapshotId(); uint64_t next_row_id = GetNextRowIdForTable(table_id, schema_version); @@ -155,14 +179,17 @@ DucklakeCopyFromStdin(CopyStmt *stmt, const char *query_string) { for (int i = 0; i < natts; i++) { int dst = i + INLINED_SYSTEM_COLS; if (copy_nulls[i]) { + col_stats.ObserveNull(i); slot_values[dst] = (Datum)0; slot_isnull[dst] = true; } else if (conv[i].needs_text_conv) { char *str = OutputFunctionCall(&conv[i].typoutput_finfo, copy_values[i]); + col_stats.ObserveDatum(i, copy_values[i]); slot_values[dst] = CStringGetTextDatum(str); slot_isnull[dst] = false; pfree(str); } else { + col_stats.ObserveDatum(i, copy_values[i]); slot_values[dst] = copy_values[i]; slot_isnull[dst] = false; } @@ -208,8 +235,10 @@ DucklakeCopyFromStdin(CopyStmt *stmt, const char *query_string) { table_close(user_rel, RowExclusiveLock); if (rows_inserted > 0) { + std::vector col_stats_out; + col_stats.Finalize(col_stats_out); SkipSnapshotSyncGuard sync_guard; - CreateSnapshotForDirectInsert(begin_snapshot, table_id, rows_inserted); + CreateSnapshotForDirectInsert(begin_snapshot, table_id, rows_inserted, col_stats_out); CommandCounterIncrement(); } diff --git a/pg_ducklake/src/direct_insert.cpp b/pg_ducklake/src/direct_insert.cpp index 31e0f803..ee73d8be 100644 --- a/pg_ducklake/src/direct_insert.cpp +++ b/pg_ducklake/src/direct_insert.cpp @@ -5,10 +5,13 @@ #include "pgducklake/direct_insert.hpp" #include "pgducklake/duckdb_manager.hpp" #include "pgducklake/guc.hpp" +#include "pgducklake/inline_col_stats.hpp" #include "pgducklake/pgducklake_metadata_manager.hpp" #include +#include #include +#include #include @@ -194,8 +197,10 @@ static bool IsUnnestOfParam(Node *node, int *param_id_out, Oid *param_type_out); static bool ValidateArrayLengths(ParamListInfo bound_params, List *param_ids, int *expected_row_count_out); static PlannedStmt *CreateDirectInsertPlan(Query *parse, DirectInsertContext *context); static PlannedStmt *CreateValuesInsertPlan(Query *parse, ValuesInsertContext *context); -static void DirectInsertIntoInlinedTable(DirectInsertScanState *state); -static void DirectInsertValuesIntoInlinedTable(DirectInsertScanState *state); +static void DirectInsertIntoInlinedTable(DirectInsertScanState *state, + std::vector &col_stats_out); +static void DirectInsertValuesIntoInlinedTable(DirectInsertScanState *state, + std::vector &col_stats_out); /* * Map a DuckDB type string to the PG OID used in the inlined data table; @@ -1217,14 +1222,16 @@ DirectInsert_ExecCustomScan(CustomScanState *node) { state->next_row_id = pgducklake::GetNextRowIdForTable(state->table_id, state->schema_version); state->rows_inserted = 0; + std::vector col_stats; if (state->mode == DIRECT_INSERT_UNNEST) { - DirectInsertIntoInlinedTable(state); + DirectInsertIntoInlinedTable(state, col_stats); } else { - DirectInsertValuesIntoInlinedTable(state); + DirectInsertValuesIntoInlinedTable(state, col_stats); } pgducklake::SkipSnapshotSyncGuard sync_guard; - pgducklake::CreateSnapshotForDirectInsert(state->begin_snapshot, state->table_id, state->rows_inserted); + pgducklake::CreateSnapshotForDirectInsert(state->begin_snapshot, state->table_id, state->rows_inserted, + col_stats); } PG_CATCH(); { @@ -1277,7 +1284,7 @@ DirectInsert_ExplainCustomScan(CustomScanState *node, List *ancestors, ExplainSt } static void -DirectInsertIntoInlinedTable(DirectInsertScanState *state) { +DirectInsertIntoInlinedTable(DirectInsertScanState *state, std::vector &col_stats_out) { int ret; if ((ret = SPI_connect()) < 0) { @@ -1381,6 +1388,16 @@ DirectInsertIntoInlinedTable(DirectInsertScanState *state) { } } + /* Per-column min/max accumulator: widened into the catalog after the batch is written. Cells are + * folded as native PG Datums (element_types[i]) and compared in PG value space -- no per-cell + * text/Value/cast round-trip. */ + InlineColStats col_stats(state->table_id, num_params); + for (int i = 0; i < num_params; i++) { + if (col_stats.ColumnEligible(i)) { + col_stats.SetupColumn(i, element_types[i]); + } + } + uint64_t current_row_id = state->next_row_id; for (int row = 0; row < arr_length; row++) { @@ -1389,17 +1406,20 @@ DirectInsertIntoInlinedTable(DirectInsertScanState *state) { for (int i = 0; i < num_params; i++) { if (elem_nulls[i][row]) { + col_stats.ObserveNull(i); values[i + 2] = (Datum)0; nulls[i + 2] = 'n'; } else if (needs_text_conv[i]) { // Scalar type (DATE, TIMESTAMP, UBIGINT, etc.) -> VARCHAR: // use PG output function to produce a DuckDB-parseable text string. char *str = OidOutputFunctionCall(typoutput[i], elem_values[i][row]); + col_stats.ObserveDatum(i, elem_values[i][row]); values[i + 2] = CStringGetTextDatum(str); nulls[i + 2] = ' '; pfree(str); } else { // Types match (native), BYTEA zero-copy, or unexpected -- pass as-is. + col_stats.ObserveDatum(i, elem_values[i][row]); values[i + 2] = elem_values[i][row]; nulls[i + 2] = ' '; } @@ -1413,6 +1433,8 @@ DirectInsertIntoInlinedTable(DirectInsertScanState *state) { state->rows_inserted++; } + col_stats.Finalize(col_stats_out); + SPI_finish(); ereport(DEBUG1, (errmsg("DuckLake direct insert: successfully inserted %lld rows into %s", @@ -1431,11 +1453,21 @@ struct ValuesColumnConvInfo { }; static void -DirectInsertValuesIntoInlinedTable(DirectInsertScanState *state) { +DirectInsertValuesIntoInlinedTable(DirectInsertScanState *state, std::vector &col_stats_out) { int num_rows = state->values_num_rows; int num_cols = state->values_num_cols; ExprContext *econtext = state->css.ss.ps.ps_ExprContext; + /* Per-column min/max accumulator (widened into the catalog after the batch). The VALUES path + * writes via the table AM (no SPI), so build the accumulator -- which reads column metadata via + * SPI in its constructor -- under a scoped SPI connection; its state is SPI-independent after. */ + int sret; + if ((sret = SPI_connect()) < 0) { + ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("SPI_connect failed: %d", sret))); + } + InlineColStats col_stats(state->table_id, num_cols); + SPI_finish(); + char relname[NAMEDATALEN]; snprintf(relname, sizeof(relname), "ducklake_inlined_data_%llu_%llu", (unsigned long long)state->table_id, (unsigned long long)state->schema_version); @@ -1474,6 +1506,12 @@ DirectInsertValuesIntoInlinedTable(DirectInsertScanState *state) { } else { conv[i].needs_text_conv = false; } + + /* Bind the accumulator to this column's PG source type: cells fold as native Datums, + * compared in PG value space (no per-cell text/Value/cast round-trip). */ + if (col_stats.ColumnEligible(i)) { + col_stats.SetupColumn(i, src_type); + } } /* Ensure DateStyle is ISO for temporal -> VARCHAR text conversion. @@ -1529,14 +1567,17 @@ DirectInsertValuesIntoInlinedTable(DirectInsertScanState *state) { Datum d = ExecEvalExprSwitchContext(state->values_estates[flat], econtext, &isnull); if (isnull) { + col_stats.ObserveNull(col); sv[dst] = (Datum)0; sn[dst] = true; } else if (conv[col].needs_text_conv) { char *str = OutputFunctionCall(&conv[col].typoutput_finfo, d); + col_stats.ObserveDatum(col, d); sv[dst] = CStringGetTextDatum(str); sn[dst] = false; pfree(str); } else { + col_stats.ObserveDatum(col, d); sv[dst] = d; sn[dst] = false; } @@ -1577,6 +1618,8 @@ DirectInsertValuesIntoInlinedTable(DirectInsertScanState *state) { state->rows_inserted = num_rows; + col_stats.Finalize(col_stats_out); + ereport(DEBUG1, (errmsg("DuckLake direct insert (VALUES): inserted %d rows into %s", num_rows, relname))); } diff --git a/pg_ducklake/src/inline_col_stats.cpp b/pg_ducklake/src/inline_col_stats.cpp new file mode 100644 index 00000000..b409a2a7 --- /dev/null +++ b/pg_ducklake/src/inline_col_stats.cpp @@ -0,0 +1,425 @@ +#include "pgducklake/inline_col_stats.hpp" + +#include + +#include + +#include +#include + +extern "C" { +#include "postgres.h" + +#include "executor/spi.h" +#include "fmgr.h" +#include "lib/stringinfo.h" +#include "miscadmin.h" +#include "utils/builtins.h" +#include "utils/datum.h" +#include "utils/lsyscache.h" +#include "utils/memutils.h" +#include "utils/typcache.h" +} + +namespace pgducklake { + +namespace { + +struct ColStatEntry { + uint64_t column_id = 0; + duckdb::LogicalType type = duckdb::LogicalType::SQLNULL; + bool type_known = false; + bool eligible = false; + bool value_cmp = false; /* RequiresValueComparison(type) */ + bool finalize_direct = false; /* PG output text is already the DuckDB-canonical encoding */ + bool ready = false; /* SetupColumn bound a usable PG comparison */ + bool has = false; + + /* PG source-type machinery bound by SetupColumn. */ + Oid src_type = InvalidOid; + FmgrInfo out_finfo; /* output function for src_type (Finalize / lexicographic loop) */ + FmgrInfo *cmp_finfo = nullptr; /* value_cmp: btree comparison proc (owned by the type cache) */ + Oid cmp_collation = InvalidOid; /* value_cmp eligible types are non-collatable */ + int16 typlen = 0; + bool typbyval = false; + + /* value_cmp path: running min/max kept as copied Datums in the accumulator context. */ + Datum min_datum = 0; + Datum max_datum = 0; + /* lexicographic (non-value_cmp) path: running min/max kept as canonical text. */ + std::string min_str; + std::string max_str; + uint64_t null_count = 0; +}; + +/* True for value_cmp types whose PG output text is already byte-for-byte the DuckDB-canonical + * encoding, so Finalize can store the raw OutputFunctionCall text and skip the + * Value(text)->cast(type)->ToString round-trip. + * + * Restricted to the integer family (signed and unsigned, incl. HUGEINT/UHUGEINT). Proof of + * equality: an integer-typed DuckLake column only ever holds integral values, and the PG source + * type is either an int2/int4/int8 (int*out) or -- for the wider types stored as VARCHAR -- + * numeric (numeric_out of an integral value). All of those emit a plain base-10 string: an + * optional leading '-' then digits, no leading zeros, no thousands separators, no decimal point, + * never scientific notation. DuckDB's ToString for every integer type emits exactly the same plain + * base-10 string. So Value(pg_text).Cast(int_type).ToString() == pg_text for all values. + * + * Deliberately excluded (kept on the full round-trip): + * - BOOLEAN: PG emits 't'/'f', DuckDB canonical is 'true'/'false'. + * - DECIMAL: scale/zero/sign formatting equivalence between numeric_out and DuckDB decimal + * ToString is not proven here. + * - DATE/TIME/TIMESTAMP*: PG ISO output and DuckDB ToString agree in the common era but can + * diverge for BC / out-of-range years (e.g. PG '... BC', 5+ digit years). Not proven, so not + * guessed. */ +bool +CanonicalEqualsOutput(const duckdb::LogicalType &type) { + switch (type.id()) { + case duckdb::LogicalTypeId::TINYINT: + case duckdb::LogicalTypeId::SMALLINT: + case duckdb::LogicalTypeId::INTEGER: + case duckdb::LogicalTypeId::BIGINT: + case duckdb::LogicalTypeId::HUGEINT: + case duckdb::LogicalTypeId::UTINYINT: + case duckdb::LogicalTypeId::USMALLINT: + case duckdb::LogicalTypeId::UINTEGER: + case duckdb::LogicalTypeId::UBIGINT: + case duckdb::LogicalTypeId::UHUGEINT: + return true; + default: + return false; + } +} + +/* Types for which native DuckLake persists a table-level min/max we can safely maintain from + * inlined tuples. Mirrors DuckLakeColumnStats::ToStats() minus FLOAT/DOUBLE (NaN handling) and + * GEOMETRY/VARIANT/BLOB (extra_stats / no stats; never reach the inline path anyway). + * + * Gates min/max only -- ObserveNull is deliberately not gated by it. + * + * Excluding FLOAT/DOUBLE means their persisted bounds go stale rather than being widened, which is + * safe only while those bounds are never turned into read-side statistics: ToStats() returns nullptr + * for FLOAT/DOUBLE unless has_contains_nan && !contains_nan, and contains_nan is NULL on every table + * this path can reach. That is an invariant held elsewhere, not here -- see the + * "FIXME: we can gather nan statistics for FLOAT/DOUBLE" in + * third_party/ducklake/src/storage/ducklake_inline_data.cpp. If that FIXME lands, a stale FLOAT + * bound becomes a live mis-pruning bug and these two types must be added here (or excluded on the + * read side) at the same time. */ +bool +StatsEligible(const duckdb::LogicalType &type) { + switch (type.id()) { + case duckdb::LogicalTypeId::BOOLEAN: + case duckdb::LogicalTypeId::TINYINT: + case duckdb::LogicalTypeId::SMALLINT: + case duckdb::LogicalTypeId::INTEGER: + case duckdb::LogicalTypeId::BIGINT: + case duckdb::LogicalTypeId::HUGEINT: + case duckdb::LogicalTypeId::UTINYINT: + case duckdb::LogicalTypeId::USMALLINT: + case duckdb::LogicalTypeId::UINTEGER: + case duckdb::LogicalTypeId::UBIGINT: + case duckdb::LogicalTypeId::UHUGEINT: + case duckdb::LogicalTypeId::DECIMAL: + case duckdb::LogicalTypeId::DATE: + case duckdb::LogicalTypeId::TIME: + case duckdb::LogicalTypeId::TIMESTAMP: + case duckdb::LogicalTypeId::TIMESTAMP_TZ: + case duckdb::LogicalTypeId::TIMESTAMP_SEC: + case duckdb::LogicalTypeId::TIMESTAMP_MS: + case duckdb::LogicalTypeId::TIMESTAMP_NS: + case duckdb::LogicalTypeId::UUID: + case duckdb::LogicalTypeId::VARCHAR: + return true; + default: + return false; + } +} + +/* Forces ISO/YMD temporal output for its scope. Restoring from a destructor keeps a throw out of + * the canonicalization path from leaking the forced setting into the rest of the session. A PG + * elog(ERROR) longjmp still bypasses it, and DateStyle is assigned directly rather than through the + * GUC machinery, so transaction abort would not undo it either. */ +struct IsoDateStyleGuard { + const int saved_style = DateStyle; + const int saved_order = DateOrder; + + IsoDateStyleGuard() { + DateStyle = USE_ISO_DATES; + DateOrder = DATEORDER_YMD; + } + ~IsoDateStyleGuard() { + DateStyle = saved_style; + DateOrder = saved_order; + } +}; + +} // namespace + +struct InlineColStats::Impl { + std::vector entries; + bool active = false; + /* Long-lived home for copied min/max Datums and cached output FmgrInfos: outlives the per-tuple + * / per-batch contexts the callers reset mid-loop. Parented on the transaction rather than + * TopMemoryContext because a PG elog(ERROR) longjmps past ~Impl -- under TopMemoryContext the + * context would then survive to backend exit, one leak per failed insert. This makes abort free + * it. Requires that no caller outlive the (sub)transaction current at construction. */ + MemoryContext ctx = nullptr; + + Impl() { + ctx = AllocSetContextCreate(CurTransactionContext, "InlineColStats", ALLOCSET_SMALL_SIZES); + } + ~Impl() { + if (ctx) { + MemoryContextDelete(ctx); + } + } +}; + +InlineColStats::InlineColStats(uint64_t table_id, int num_cols) : impl(new Impl()) { + StringInfoData q; + initStringInfo(&q); + appendStringInfo(&q, R"( + SELECT column_id, column_type FROM ducklake.ducklake_column + WHERE table_id = %llu AND end_snapshot IS NULL AND parent_column IS NULL + ORDER BY column_order)", + (unsigned long long)table_id); + int ret = SPI_execute(q.data, true, 0); + if (ret != SPI_OK_SELECT || (int)SPI_processed < num_cols) { + return; + } + impl->entries.resize(num_cols); + for (int i = 0; i < num_cols; i++) { + bool isnull; + Datum id_d = SPI_getbinval(SPI_tuptable->vals[i], SPI_tuptable->tupdesc, 1, &isnull); + if (isnull) { + impl->entries.clear(); + return; + } + impl->entries[i].column_id = (uint64_t)DatumGetInt64(id_d); + char *type_str = SPI_getvalue(SPI_tuptable->vals[i], SPI_tuptable->tupdesc, 2); + if (!type_str) { + continue; + } + try { + duckdb::LogicalType t = duckdb::DuckLakeTypes::FromString(type_str); + /* A null-only stat still goes through the typed merge. */ + impl->entries[i].type = t; + impl->entries[i].type_known = true; + if (StatsEligible(t)) { + impl->entries[i].eligible = true; + impl->entries[i].value_cmp = duckdb::RequiresValueComparison(t); + impl->entries[i].finalize_direct = CanonicalEqualsOutput(t); + } + } catch (...) { + /* Unsupported type string -> no stats of any kind for this column. */ + } + pfree(type_str); + } + impl->active = true; +} + +InlineColStats::~InlineColStats() = default; + +bool +InlineColStats::ColumnEligible(int col) const { + if (!impl->active || col < 0 || col >= (int)impl->entries.size()) { + return false; + } + return impl->entries[col].eligible; +} + +void +InlineColStats::SetupColumn(int col, Oid pg_source_type) { + if (!impl->active || col < 0 || col >= (int)impl->entries.size()) { + return; + } + auto &e = impl->entries[col]; + if (!e.eligible) { + return; + } + e.src_type = pg_source_type; + + Oid out_func; + bool out_varlena; + getTypeOutputInfo(pg_source_type, &out_func, &out_varlena); + + MemoryContext old = MemoryContextSwitchTo(impl->ctx); + fmgr_info(out_func, &e.out_finfo); + MemoryContextSwitchTo(old); + + if (e.value_cmp) { + /* Numeric/temporal/boolean: compare in PG value space via the source type's btree + * comparison proc. Its ordering matches DuckDB's value ordering for every eligible + * value_cmp type. The FmgrInfo is owned by the (process-lifetime) type cache. */ + TypeCacheEntry *tce = lookup_type_cache(pg_source_type, TYPECACHE_CMP_PROC_FINFO); + if (!tce || !OidIsValid(tce->cmp_proc_finfo.fn_oid)) { + /* No usable comparison -> maintain nothing rather than persist an unverifiable bound. */ + e.eligible = false; + return; + } + e.cmp_finfo = &tce->cmp_proc_finfo; + e.cmp_collation = InvalidOid; + e.typlen = tce->typlen; + e.typbyval = tce->typbyval; + } + e.ready = true; +} + +void +InlineColStats::ObserveDatum(int col, Datum value) { + if (!impl->active || col < 0 || col >= (int)impl->entries.size()) { + return; + } + auto &e = impl->entries[col]; + if (!e.eligible || !e.ready) { + return; + } + + if (e.value_cmp) { + if (!e.has) { + /* datumCopy flattens an inline-compressed or short varlena but does NOT detoast an external + * pointer -- the copy would then be a dangling TOAST reference. Safe here only because none + * of the three call sites can produce one: COPY input functions, deconstruct_array elements + * of a freshly built array, and ExecEvalExpr over VALUES constants all yield in-memory + * datums. A source that can hand us an external pointer must detoast before calling. */ + MemoryContext old = MemoryContextSwitchTo(impl->ctx); + e.min_datum = datumCopy(value, e.typbyval, e.typlen); + e.max_datum = datumCopy(value, e.typbyval, e.typlen); + MemoryContextSwitchTo(old); + e.has = true; + return; + } + /* Compare in the caller's (per-tuple) context so comparison temporaries are reclaimed by the + * caller's periodic reset; copy a new bound into the accumulator context only when it wins. */ + if (DatumGetInt32(FunctionCall2Coll(e.cmp_finfo, e.cmp_collation, value, e.min_datum)) < 0) { + MemoryContext old = MemoryContextSwitchTo(impl->ctx); + Datum copy = datumCopy(value, e.typbyval, e.typlen); + MemoryContextSwitchTo(old); + if (!e.typbyval) { + pfree(DatumGetPointer(e.min_datum)); + } + e.min_datum = copy; + } + if (DatumGetInt32(FunctionCall2Coll(e.cmp_finfo, e.cmp_collation, value, e.max_datum)) > 0) { + MemoryContext old = MemoryContextSwitchTo(impl->ctx); + Datum copy = datumCopy(value, e.typbyval, e.typlen); + MemoryContextSwitchTo(old); + if (!e.typbyval) { + pfree(DatumGetPointer(e.max_datum)); + } + e.max_datum = copy; + } + return; + } + + /* Lexicographic (VARCHAR/UUID): the PG output text equals DuckDB's canonical VARCHAR encoding + * for these types, so byte-order string compare here matches DuckLakeColumnStats::MergeStats. */ + char *s = OutputFunctionCall(&e.out_finfo, value); + std::string cs(s); + pfree(s); + if (!e.has) { + e.min_str = cs; + e.max_str = cs; + e.has = true; + return; + } + if (cs < e.min_str) { + e.min_str = cs; + } + if (cs > e.max_str) { + e.max_str = cs; + } +} + +void +InlineColStats::ObserveNull(int col) { + if (!impl->active || col < 0 || col >= (int)impl->entries.size()) { + return; + } + impl->entries[col].null_count++; +} + +void +InlineColStats::Finalize(std::vector &out) { + out.clear(); + if (!impl->active) { + return; + } + + /* Temporal output funcs are DateStyle-dependent; force ISO so the text parses back through + * DuckDB's cast during canonicalization (and matches what the inline write stored). */ + IsoDateStyleGuard date_style_guard; + + for (auto &e : impl->entries) { + if (!e.type_known) { + continue; + } + bool have_bounds = e.eligible && e.ready && e.has; + + std::string min_canon; + std::string max_canon; + if (have_bounds && e.value_cmp) { + char *min_txt = OutputFunctionCall(&e.out_finfo, e.min_datum); + char *max_txt = OutputFunctionCall(&e.out_finfo, e.max_datum); + if (e.finalize_direct) { + /* Integer family: PG output text is already byte-identical to DuckDB's canonical + * encoding (see CanonicalEqualsOutput), so store it directly -- no Value/cast/ToString. */ + min_canon = min_txt; + max_canon = max_txt; + pfree(min_txt); + pfree(max_txt); + } else { + /* Boolean / decimal / temporal: canonicalize the two surviving bounds through the same + * Value(text)->cast(type)->ToString path the read side round-trips, so the stored bytes + * are exactly what DuckLake would write (e.g. boolean "t"/"f" -> "true"/"false"). */ + bool ok = true; + try { + duckdb::Value vmin; + duckdb::Value vmax; + std::string err; + if (!duckdb::Value(std::string(min_txt)).DefaultTryCastAs(e.type, vmin, &err) || vmin.IsNull()) { + ok = false; + } else { + min_canon = vmin.ToString(); + } + if (ok && + (!duckdb::Value(std::string(max_txt)).DefaultTryCastAs(e.type, vmax, &err) || vmax.IsNull())) { + ok = false; + } else if (ok) { + max_canon = vmax.ToString(); + } + } catch (...) { + /* Should be unreachable for inlineable data; skip rather than persist an + * unverifiable bound (leaves the existing catalog bound untouched). */ + ok = false; + } + pfree(min_txt); + pfree(max_txt); + if (!ok) { + /* Drop the bounds, not the column: an observed NULL is still worth persisting. */ + have_bounds = false; + } + } + } else if (have_bounds) { + min_canon = e.min_str; + max_canon = e.max_str; + } + + if (!have_bounds && e.null_count == 0) { + continue; + } + + DirectInsertColumnStat s; + s.column_id = e.column_id; + s.column_type = duckdb::DuckLakeTypes::ToString(e.type); + s.has_min = have_bounds; + s.has_max = have_bounds; + if (have_bounds) { + s.min_value = std::move(min_canon); + s.max_value = std::move(max_canon); + } + s.null_count = e.null_count; + out.push_back(std::move(s)); + } +} + +} // namespace pgducklake diff --git a/pg_ducklake/src/pgducklake_metadata_manager.cpp b/pg_ducklake/src/pgducklake_metadata_manager.cpp index 2db257d0..d98569e4 100644 --- a/pg_ducklake/src/pgducklake_metadata_manager.cpp +++ b/pg_ducklake/src/pgducklake_metadata_manager.cpp @@ -11,6 +11,7 @@ #include "pgddb/pgddb_types.hpp" #include "pgddb/pgddb_utils.hpp" +#include #include #include #include @@ -23,7 +24,9 @@ #include #include #include +#include #include +#include #include extern "C" { @@ -747,8 +750,71 @@ GetNextSnapshotId() { return next_snapshot_id; } +/* Widen a column's persisted bounds with this batch's, using DuckLake's own merge so the comparison + * semantics (value-typed for numerics/temporals/boolean, lexicographic otherwise) cannot drift from + * what the engine does everywhere else. The persisted catalog row is decoded with the engine's own + * decoder, DuckLakeColumnStats::FromGlobalStats -- the same call the read side uses -- so `current` + * is bit-for-bit what native DuckLake would have built from that row. + * + * A stats row can legitimately hold one bound and not the other: MergeStats invalidates min and max + * independently when a column is retyped (see its ReconcileStatToType calls). We leave the absent + * side absent and let MergeStats decide; it fills a missing bound only through its !AnyValid() + * branch, i.e. only when the persisted row carries no information at all. + * + * Why we never seed the absent side from this batch: a NULL bound is the safe state -- every query + * still returns correct results, we only give up pruning. A bound that is too narrow is silently + * wrong: it prunes away live rows, and DuckDB's compressed materialization uses a table-level min as + * an exact constant, so a too-high min also corrupts the values a plain unfiltered SELECT returns. + * Filling a missing bound truthfully needs every live row in the table, and one insert batch only + * knows its own. + * + * The price is real and worth stating: on a numeric or temporal column, a half-populated row + * disables zone-map pruning for that column ENTIRELY, not just on the missing side. + * CreateNumericStats builds on NumericStats::CreateEmpty, which pre-seeds min = MaximumValue(type) + * and max = MinimumValue(type) with both has_ flags set, so a one-sided row yields an inverted range; + * HasMinMax (duckdb/src/storage/statistics/numeric_stats.cpp:378-381) requires has_min && has_max && + * Min <= Max, so every consumer -- CheckZonemap (:237) and compressed materialization + * (compressed_materialization.cpp:360) alike -- falls back to NO_PRUNING_POSSIBLE. VARCHAR differs: + * CreateStringStats' one-sided branches build on StringStats::CreateUnknown, whose 0x00..0xFF + * sentinels leave a valid range, so a surviving string bound still prunes on its own side. Either + * way results stay correct, which is the point. Recovering the missing bound needs ground truth (a + * rescan, or a fix at ALTER time), not a guess from whatever happened to be inserted next. */ +static void +WidenColumnStats(const duckdb::LogicalType &type, const duckdb::DuckLakeGlobalColumnStatsInfo &persisted, + const DirectInsertColumnStat &cs, std::string &out_min, bool &out_has_min, std::string &out_max, + bool &out_has_max, bool &out_has_contains_null, bool &out_contains_null) { + auto current = duckdb::DuckLakeColumnStats::FromGlobalStats(type, persisted); + + duckdb::DuckLakeColumnStats incoming(type); + incoming.min = cs.min_value; + incoming.has_min = cs.has_min; + incoming.max = cs.max_value; + incoming.has_max = cs.has_max; + /* Leaving this false would degrade the persisted contains_null to unknown on every direct + * insert. Native's inline writer sets it unconditionally too. */ + incoming.has_null_count = true; + incoming.null_count = cs.null_count; + /* Derived, not hardcoded true: stats that claim validity while carrying no bound drive MergeStats + * into its !has_min / !has_max branches, which CLEAR the persisted bounds. */ + incoming.any_valid = cs.has_min || cs.has_max; + + /* MergeStats' types_differ is always false here -- both sides are built from cs.column_type -- so + * its ReconcileStatToType path never runs on this call. */ + current.MergeStats(incoming); + + out_min = current.min; + out_has_min = current.has_min; + out_max = current.max; + out_has_max = current.has_max; + /* MergeStats folds null_count before its !AnyValid() early return, so an all-NULL batch still + * updates null-ness. */ + out_has_contains_null = current.has_null_count; + out_contains_null = current.null_count > 0; +} + void -CreateSnapshotForDirectInsert(uint64_t snapshot_id, uint64_t table_id, int64_t rows_inserted) { +CreateSnapshotForDirectInsert(uint64_t snapshot_id, uint64_t table_id, int64_t rows_inserted, + const std::vector &column_stats) { int ret; elog(DEBUG1, "CreateSnapshotForDirectInsert: creating snapshot %llu", (unsigned long long)snapshot_id); @@ -791,6 +857,26 @@ CreateSnapshotForDirectInsert(uint64_t snapshot_id, uint64_t table_id, int64_t r } } + /* Force-advance next_file_id by one even though this insert writes no parquet file, mirroring + * native DuckLake: DuckLakeTransactionState::GetNewDataFiles increments + * commit_state.commit_snapshot.next_file_id under `if (table_changes.new_data_files.empty())`, + * commented there as "force an increment of file_id to signal a data change if we have only + * inlined data changes" (ducklake_transaction_state.cpp:801-804). Native's guard is implicit + * here: direct insert writes no parquet by construction, so this path is always the + * inlined-only case. This makes DuckLakeCatalog's table-stats cache key change on every inline + * insert, so a scan taken between two inline inserts in the same session cannot serve stale + * pre-widening bounds and mis-prune the newly inlined rows. next_file_id is the one component we + * can move: the key is (next_file_id, table_id) in stock ducklake and (next_file_id, + * schema_version, table_id) with patch 007 applied, and an inline insert changes schema_version + * no more than it writes a file. + * + * Collision safety: the latest snapshot's next_file_id is the source of truth for the id the next + * real parquet file receives (a flush reads it, assigns that id, and persists +1). We persist the + * bumped value into THIS snapshot's ducklake_snapshot.next_file_id below, so the id we skip is a + * permanent gap -- never reused by a later file. Native produces the same monotonic gaps; gaps are + * fine, reuse is not. */ + next_file_id++; + StringInfoData snapshot_insert; initStringInfo(&snapshot_insert); appendStringInfo(&snapshot_insert, @@ -871,6 +957,197 @@ CreateSnapshotForDirectInsert(uint64_t snapshot_id, uint64_t table_id, int64_t r (unsigned long long)table_id); } + /* Widen the persisted per-column min/max and contains_null to cover this batch's inserted tuples. + * Both only ever extend -- a NULL (unknown) bound is replaced, a known bound is pushed outward, + * and contains_null only goes false->true -- so a zone map that already describes flushed parquet + * is never narrowed into a lie. min/max are compared and stored type-correctly (see + * WidenStatBound); contains_nan/extra_stats are left untouched. + * + * We only ever UPDATE an existing stats row; we never CREATE one here. A missing row means the + * column has no table-level stats yet -- which for a column added by ALTER TABLE ADD COLUMN means + * its pre-existing rows were back-filled from initial_default, values this batch never observed. + * Seeding a row from the batch alone would exclude those back-filled values and lie (and the + * inlined scan then mis-reconstructs them). The genuine first-insert case already created NULL + * rows for every column above, so this UPDATE-only rule still maintains stats there. */ + /* Batched form of the per-column widen: one SELECT of every candidate column's current stats row, + * then a single UPDATE ... FROM (VALUES ...) that widens them all at once -- collapsing the + * former 2-SPI-round-trips-per-column into at most 2 total, and into 1 when no column's bounds + * actually move (the steady state). Semantics are byte-for-byte those of the per-column path: + * - never-seed: only columns that ALREADY have a stats row (i.e. are returned by the SELECT) + * are widened; a column with no row is never added here (see the block comment above). The + * UPDATE ... FROM join can only touch existing rows, and we only emit VALUES for returned + * columns, so the guard holds two ways. + * - type-correct widening via DuckLakeColumnStats::MergeStats -- computed here in C++; SQL only + * carries the already-decided literals. + * - contains_null is maintained the same way, through the same MergeStats call; + * contains_nan / extra_stats are never referenced, so stay untouched. */ + struct WidenCandidate { + uint64_t column_id; + const DirectInsertColumnStat *cs; + duckdb::LogicalType type; + }; + std::vector candidates; + for (const auto &cs : column_stats) { + if (!cs.has_min && !cs.has_max && cs.null_count == 0) { + continue; + } + duckdb::LogicalType type; + try { + type = duckdb::DuckLakeTypes::FromString(cs.column_type); + } catch (...) { + /* Belt and braces: every in-tree producer sets column_type from DuckLakeTypes::ToString of + * a type DuckLakeTypes::FromString itself returned, so this cannot throw today. Kept because + * CreateSnapshotForDirectInsert takes column_stats from its callers, and skipping a column + * is the correct response for any producer that ever gets that wrong. */ + continue; + } + candidates.push_back({cs.column_id, &cs, type}); + } + + if (!candidates.empty()) { + StringInfoData sel; + initStringInfo(&sel); + /* contains_nan / extra_stats are read but never written: they complete the + * DuckLakeGlobalColumnStatsInfo that FromGlobalStats decodes, whose any_valid is + * has_min || has_max || has_extra_stats. Same row, same round trip. */ + appendStringInfo(&sel, + "SELECT column_id, min_value, max_value, contains_null, contains_nan, extra_stats " + "FROM ducklake.ducklake_table_column_stats " + "WHERE table_id = %llu AND column_id IN (", + (unsigned long long)table_id); + for (size_t i = 0; i < candidates.size(); i++) { + appendStringInfo(&sel, "%s%llu", i ? "," : "", (unsigned long long)candidates[i].column_id); + } + appendStringInfoChar(&sel, ')'); + /* read_only = false: take a fresh snapshot so the NULL column_stats rows inserted earlier in + * this same SPI connection (first-insert branch) are visible to the widen. */ + ret = SPI_execute(sel.data, false, 0); + if (ret != SPI_OK_SELECT) { + elog(ERROR, "CreateSnapshotForDirectInsert: failed to read column stats: %d", ret); + } + + /* No further SPI runs before the UPDATE, so SPI_tuptable stays valid across this read loop. */ + uint64_t nrows = SPI_processed; + StringInfoData vals; + initStringInfo(&vals); + int rows_to_update = 0; + for (uint64_t r = 0; r < nrows; r++) { + bool isnull; + Datum cid_d = SPI_getbinval(SPI_tuptable->vals[r], SPI_tuptable->tupdesc, 1, &isnull); + if (isnull) { + continue; + } + uint64_t cid = (uint64_t)DatumGetInt64(cid_d); + + const WidenCandidate *cand = nullptr; + for (const auto &c : candidates) { + if (c.column_id == cid) { + cand = &c; + break; + } + } + if (!cand) { + continue; /* defensive: a row for a column we did not ask about */ + } + const DirectInsertColumnStat &cs = *cand->cs; + + /* Decode the catalog row into the engine's own carrier, mirroring native's + * TransformGlobalStatsRow: a SQL NULL means "no such stat", never a value. */ + duckdb::DuckLakeGlobalColumnStatsInfo persisted; + persisted.column_id = duckdb::FieldIndex(cid); + char *m = SPI_getvalue(SPI_tuptable->vals[r], SPI_tuptable->tupdesc, 2); + if (m) { + persisted.has_min = true; + persisted.min_val = m; + pfree(m); + } + char *x = SPI_getvalue(SPI_tuptable->vals[r], SPI_tuptable->tupdesc, 3); + if (x) { + persisted.has_max = true; + persisted.max_val = x; + pfree(x); + } + Datum cnull_d = SPI_getbinval(SPI_tuptable->vals[r], SPI_tuptable->tupdesc, 4, &isnull); + if (!isnull) { + persisted.has_contains_null = true; + persisted.contains_null = DatumGetBool(cnull_d); + } + Datum cnan_d = SPI_getbinval(SPI_tuptable->vals[r], SPI_tuptable->tupdesc, 5, &isnull); + if (!isnull) { + persisted.has_contains_nan = true; + persisted.contains_nan = DatumGetBool(cnan_d); + } + char *es = SPI_getvalue(SPI_tuptable->vals[r], SPI_tuptable->tupdesc, 6); + if (es) { + persisted.has_extra_stats = true; + persisted.extra_stats = es; + pfree(es); + } + + std::string new_min, new_max; + bool new_has_min = false, new_has_max = false; + bool new_has_contains_null = false, new_contains_null = false; + try { + WidenColumnStats(cand->type, persisted, cs, new_min, new_has_min, new_max, new_has_max, + new_has_contains_null, new_contains_null); + } catch (const std::exception &ex) { + /* Reachable, not defensive: MergeStats compares through the non-Try DefaultCastAs, which + * throws when a persisted bound is no longer parseable as the column's current type -- e.g. + * after a plain out-of-transaction ALTER COLUMN TYPE. Leaving the persisted bounds alone is + * right (never narrow them), but do not do it silently. */ + elog(DEBUG1, "CreateSnapshotForDirectInsert: skipping stats widen for table %llu column %llu: %s", + (unsigned long long)table_id, (unsigned long long)cid, ex.what()); + continue; + } catch (...) { + elog(DEBUG1, "CreateSnapshotForDirectInsert: skipping stats widen for table %llu column %llu", + (unsigned long long)table_id, (unsigned long long)cid); + continue; + } + + /* Emit nothing when the merge moved nothing. In steady state -- a batch entirely inside the + * persisted bounds -- no column changes, rows_to_update stays 0 and the UPDATE is skipped + * altogether, so the widen costs one SELECT, writes no dead tuples and takes no row locks + * that concurrent inserters would have to wait on. */ + bool changed = (new_has_min != persisted.has_min) || (new_has_min && new_min != persisted.min_val) || + (new_has_max != persisted.has_max) || (new_has_max && new_max != persisted.max_val) || + (new_has_contains_null != persisted.has_contains_null) || + (new_has_contains_null && new_contains_null != persisted.contains_null); + if (!changed) { + continue; + } + + char *min_lit = new_has_min ? quote_literal_cstr(new_min.c_str()) : pstrdup("NULL"); + char *max_lit = new_has_max ? quote_literal_cstr(new_max.c_str()) : pstrdup("NULL"); + const char *cnull_lit = new_has_contains_null ? (new_contains_null ? "true" : "false") : "NULL"; + /* Cast the first VALUES row so PG resolves v(column_id, new_min, new_max, new_contains_null) + * column types even when later rows carry SQL NULL bounds. */ + if (rows_to_update == 0) { + appendStringInfo(&vals, "(%llu::bigint, %s::text, %s::text, %s::boolean)", (unsigned long long)cid, + min_lit, max_lit, cnull_lit); + } else { + appendStringInfo(&vals, ", (%llu, %s, %s, %s)", (unsigned long long)cid, min_lit, max_lit, cnull_lit); + } + pfree(min_lit); + pfree(max_lit); + rows_to_update++; + } + + if (rows_to_update > 0) { + StringInfoData wr; + initStringInfo(&wr); + appendStringInfo(&wr, R"( + UPDATE ducklake.ducklake_table_column_stats s + SET min_value = v.new_min, max_value = v.new_max, contains_null = v.new_contains_null + FROM (VALUES %s) AS v(column_id, new_min, new_max, new_contains_null) + WHERE s.table_id = %llu AND s.column_id = v.column_id)", + vals.data, (unsigned long long)table_id); + ret = SPI_execute(wr.data, false, 0); + if (ret != SPI_OK_UPDATE) { + elog(ERROR, "CreateSnapshotForDirectInsert: failed to widen column stats: %d", ret); + } + } + } + SPI_finish(); elog(DEBUG1, "CreateSnapshotForDirectInsert: successfully created snapshot %llu", (unsigned long long)snapshot_id); } diff --git a/pg_ducklake/test/regression/expected/direct_insert_metadata.out b/pg_ducklake/test/regression/expected/direct_insert_metadata.out new file mode 100644 index 00000000..1d333403 --- /dev/null +++ b/pg_ducklake/test/regression/expected/direct_insert_metadata.out @@ -0,0 +1,357 @@ +-- Direct-insert (fast path) metadata commit protocol. +-- The fast path hand-writes the DuckLake commit; these tests pin the parts of +-- the protocol other DuckLake machinery relies on. +CALL ducklake.set_option('data_inlining_row_limit', 100); +-- ============================================================ +-- 1. next_file_id must advance on a fast-path commit +-- ============================================================ +-- DuckLake keys its table-stats cache on (next_file_id, schema_version, +-- table_id) and bumps next_file_id on inlined-only commits to signal a data +-- change. A fast-path commit that carries it forward leaves stale cached +-- stats (next_row_id, record_count, min/max) in every open DuckDB instance. +CREATE TABLE dim_t (id int, val text) USING ducklake; +INSERT INTO dim_t VALUES (0, 'seed'); -- normal path: creates the inlined data table +SELECT next_file_id AS file_id_before +FROM ducklake.ducklake_snapshot ORDER BY snapshot_id DESC LIMIT 1 \gset +SELECT ducklake.reset_direct_insert_stats(); + reset_direct_insert_stats +--------------------------- + +(1 row) + +INSERT INTO dim_t VALUES (1, 'fast'); +-- fast path handled the insert above +SELECT pattern, reason, count FROM ducklake.direct_insert_stats() WHERE count > 0; + pattern | reason | count +----------------+--------+------- + matched_values | ok | 1 +(1 row) + +SELECT next_file_id > :file_id_before AS file_id_advanced +FROM ducklake.ducklake_snapshot ORDER BY snapshot_id DESC LIMIT 1; + file_id_advanced +------------------ + t +(1 row) + +-- ============================================================ +-- 2. row ids must stay unique when fast-path and normal-path +-- inserts interleave +-- ============================================================ +-- Load this backend's DuckDB table-stats cache, then advance next_row_id +-- through the fast path (PG metadata only). The normal-path insert below +-- must not seed its row ids from the stale cache entry. +SELECT count(*) FROM dim_t WHERE id >= 0; + count +------- + 2 +(1 row) + +INSERT INTO dim_t VALUES (2, 'fast2'); +BEGIN; -- transaction block: fast path disengages, normal DuckLake path +INSERT INTO dim_t VALUES (3, 'slow'); +COMMIT; +SELECT it.table_name AS dim_inl +FROM ducklake.ducklake_inlined_data_tables it +JOIN ducklake.ducklake_table t USING (table_id) +WHERE t.table_name = 'dim_t' AND t.end_snapshot IS NULL +ORDER BY it.schema_version DESC LIMIT 1 \gset +SELECT row_id, count(*) AS dup_count +FROM ducklake.:dim_inl GROUP BY row_id HAVING count(*) > 1; + row_id | dup_count +--------+----------- +(0 rows) + +SELECT count(*) AS inlined_rows, count(DISTINCT row_id) AS distinct_row_ids +FROM ducklake.:dim_inl; + inlined_rows | distinct_row_ids +--------------+------------------ + 4 | 4 +(1 row) + +-- deleting one row must not take an unrelated row with it +DELETE FROM dim_t WHERE id = 3; +SELECT * FROM dim_t ORDER BY id; + id | val +----+------- + 0 | seed + 1 | fast + 2 | fast2 +(3 rows) + +-- ============================================================ +-- 3. global column stats must not claim ranges that exclude +-- fast-path rows +-- ============================================================ +-- The fast path maintains min/max by widening them to cover the rows it +-- writes, so the recorded range never excludes a committed row. Stale claims +-- are trusted by DuckLake readers of this catalog (global stats feed DuckDB +-- optimizer statistics, which fold provably-false filters) and are +-- perpetuated by later stats merges. +CREATE TABLE stats_t (id int) USING ducklake; +INSERT INTO stats_t SELECT i FROM generate_series(1, 200) i; -- normal path, real stats +-- cache the (accurate, soon stale) stats in this backend +SELECT count(*) FROM stats_t WHERE id = 150; + count +------- + 1 +(1 row) + +SELECT ducklake.reset_direct_insert_stats(); + reset_direct_insert_stats +--------------------------- + +(1 row) + +INSERT INTO stats_t VALUES (1000); -- outside the recorded min/max +INSERT INTO stats_t VALUES (NULL); -- would violate contains_null = false +SELECT pattern, reason, count FROM ducklake.direct_insert_stats() WHERE count > 0; + pattern | reason | count +----------------+--------+------- + matched_values | ok | 2 +(1 row) + +-- A stale contains_null = false is not a lost optimization: the optimizer +-- folds IS NULL to false and the row disappears from the result. +SELECT s.min_value, s.max_value, s.contains_null +FROM ducklake.ducklake_table_column_stats s +JOIN ducklake.ducklake_table t ON t.table_id = s.table_id AND t.end_snapshot IS NULL +WHERE t.table_name = 'stats_t'; + min_value | max_value | contains_null +-----------+-----------+--------------- + 1 | 1000 | t +(1 row) + +SELECT count(*) FROM stats_t WHERE id = 1000; + count +------- + 1 +(1 row) + +SELECT count(*) FROM stats_t WHERE id IS NULL; + count +------- + 1 +(1 row) + +SELECT count(*) FROM stats_t; + count +------- + 202 +(1 row) + +-- ============================================================ +-- 4. contains_null is maintained for every column, not just +-- the ones whose type has a maintainable min/max +-- ============================================================ +-- Seeded from parquet so contains_null starts known-false. An inline-only +-- table would leave it unknown instead, which section 6 covers. +CALL ducklake.set_option('data_inlining_row_limit', 0); +CREATE TABLE nullstats (id int, v int, f double precision, w int, bs bytea) USING ducklake; +INSERT INTO nullstats SELECT i, i, i::float8, i, '\x01'::bytea FROM generate_series(1, 200) i; +CALL ducklake.set_option('data_inlining_row_limit', 100); +INSERT INTO nullstats VALUES (901, 7, 1.5, 7, '\x02'); -- normal path: creates the inlined data table +-- f (DOUBLE) and bs (BLOB) are excluded from min/max by StatsEligible, so they +-- pin that null-ness is not gated by that exclusion. BLOB additionally is the +-- only fast-path-reachable type whose persisted row can carry extra_stats. +SELECT ducklake.reset_direct_insert_stats(); + reset_direct_insert_stats +--------------------------- + +(1 row) + +INSERT INTO nullstats VALUES (902, NULL, NULL, 8, NULL), (903, NULL, NULL, 9, NULL); +SELECT pattern, reason, count FROM ducklake.direct_insert_stats() WHERE count > 0; + pattern | reason | count +----------------+--------+------- + matched_values | ok | 1 +(1 row) + +-- v's bounds must stay at [1,200]: an all-NULL batch moves no bound. +SELECT s.column_id, s.contains_null, s.min_value, s.max_value +FROM ducklake.ducklake_table_column_stats s +JOIN ducklake.ducklake_table t ON t.table_id = s.table_id AND t.end_snapshot IS NULL +WHERE t.table_name = 'nullstats' ORDER BY s.column_id; + column_id | contains_null | min_value | max_value +-----------+---------------+-----------+----------- + 1 | f | 1 | 903 + 2 | t | 1 | 200 + 3 | t | 1.0 | 200.0 + 4 | f | 1 | 200 + 5 | t | 01 | \x02 +(5 rows) + +SELECT count(*) FROM nullstats WHERE v IS NULL; + count +------- + 2 +(1 row) + +SELECT count(*) FROM nullstats WHERE f IS NULL; + count +------- + 2 +(1 row) + +-- A batch with no NULLs must not reset it: the flip is one-way. +INSERT INTO nullstats VALUES (904, 8, 2.5, 10, '\x03'); +SELECT s.column_id, s.contains_null +FROM ducklake.ducklake_table_column_stats s +JOIN ducklake.ducklake_table t ON t.table_id = s.table_id AND t.end_snapshot IS NULL +WHERE t.table_name = 'nullstats' ORDER BY s.column_id; + column_id | contains_null +-----------+--------------- + 1 | f + 2 | t + 3 | t + 4 | f + 5 | t +(5 rows) + +SELECT count(*) FROM nullstats WHERE v IS NULL; + count +------- + 2 +(1 row) + +-- w's first NULL, so column 4 must flip here and nowhere earlier -- otherwise +-- this asserts nothing about the COPY writer. +COPY nullstats (id, v, f, w, bs) FROM STDIN WITH (FORMAT csv, NULL ''); +SELECT s.column_id, s.contains_null +FROM ducklake.ducklake_table_column_stats s +JOIN ducklake.ducklake_table t ON t.table_id = s.table_id AND t.end_snapshot IS NULL +WHERE t.table_name = 'nullstats' ORDER BY s.column_id; + column_id | contains_null +-----------+--------------- + 1 | f + 2 | t + 3 | t + 4 | t + 5 | t +(5 rows) + +SELECT count(*) FROM nullstats WHERE v IS NULL; + count +------- + 3 +(1 row) + +SELECT count(*) FROM nullstats WHERE w IS NULL; + count +------- + 1 +(1 row) + +SELECT count(*) FROM nullstats; + count +------- + 205 +(1 row) + +-- ============================================================ +-- 5. the UNNEST writer maintains it too +-- ============================================================ +-- Its own null branch, so a fix applied to the VALUES and COPY writers can miss +-- it. Needs a dedicated table: an UNNEST naming a column subset does not reach +-- the fast path at all. +CALL ducklake.set_option('data_inlining_row_limit', 0); +CREATE TABLE nullstats_un (id int, v int) USING ducklake; +INSERT INTO nullstats_un SELECT i, i FROM generate_series(1, 200) i; +CALL ducklake.set_option('data_inlining_row_limit', 100); +INSERT INTO nullstats_un VALUES (901, 7); +PREPARE nullstats_un_ins (int[], int[]) AS + INSERT INTO nullstats_un (id, v) SELECT UNNEST($1), UNNEST($2); +SELECT ducklake.reset_direct_insert_stats(); + reset_direct_insert_stats +--------------------------- + +(1 row) + +EXECUTE nullstats_un_ins(ARRAY[902, 903], ARRAY[NULL, NULL]::int[]); +SELECT pattern, reason, count FROM ducklake.direct_insert_stats() WHERE count > 0; + pattern | reason | count +----------------+--------+------- + matched_unnest | ok | 1 +(1 row) + +SELECT s.column_id, s.contains_null, s.min_value, s.max_value +FROM ducklake.ducklake_table_column_stats s +JOIN ducklake.ducklake_table t ON t.table_id = s.table_id AND t.end_snapshot IS NULL +WHERE t.table_name = 'nullstats_un' ORDER BY s.column_id; + column_id | contains_null | min_value | max_value +-----------+---------------+-----------+----------- + 1 | f | 1 | 903 + 2 | t | 1 | 200 +(2 rows) + +SELECT count(*) FROM nullstats_un WHERE v IS NULL; + count +------- + 2 +(1 row) + +SELECT count(*) FROM nullstats_un; + count +------- + 203 +(1 row) + +DEALLOCATE nullstats_un_ins; +-- ============================================================ +-- 6. a table that never flushed keeps contains_null unknown +-- ============================================================ +-- The commonest fast-path shape, and the one where correctness rests on the +-- value staying unknown rather than being maintained: MergeStats' has_null_count +-- degrade is sticky, so no widen can ever write it. Seeding false here instead +-- would fold IS NULL to false and lose rows. +CALL ducklake.set_option('data_inlining_row_limit', 1000); +CREATE TABLE nullstats_inl (id int, v int) USING ducklake; +INSERT INTO nullstats_inl VALUES (1, 1); -- normal path: creates the inlined data table +SELECT ducklake.reset_direct_insert_stats(); + reset_direct_insert_stats +--------------------------- + +(1 row) + +INSERT INTO nullstats_inl VALUES (2, NULL), (3, 3); +SELECT pattern, reason, count FROM ducklake.direct_insert_stats() WHERE count > 0; + pattern | reason | count +----------------+--------+------- + matched_values | ok | 1 +(1 row) + +SELECT s.column_id, s.contains_null IS NULL AS unknown +FROM ducklake.ducklake_table_column_stats s +JOIN ducklake.ducklake_table t ON t.table_id = s.table_id AND t.end_snapshot IS NULL +WHERE t.table_name = 'nullstats_inl' ORDER BY s.column_id; + column_id | unknown +-----------+--------- + 1 | t + 2 | t +(2 rows) + +SELECT count(*) FROM nullstats_inl WHERE v IS NULL; + count +------- + 1 +(1 row) + +SELECT * FROM ducklake.flush_inlined_data('nullstats_inl'::regclass); + schema_name | table_name | rows_flushed +-------------+---------------+-------------- + public | nullstats_inl | 3 +(1 row) + +SELECT count(*) FROM nullstats_inl WHERE v IS NULL; + count +------- + 1 +(1 row) + +-- Cleanup +DROP TABLE dim_t; +DROP TABLE stats_t; +DROP TABLE nullstats; +DROP TABLE nullstats_un; +DROP TABLE nullstats_inl; +CALL ducklake.set_option('data_inlining_row_limit', 0); diff --git a/pg_ducklake/test/regression/expected/inlined_scan_stats_guard.out b/pg_ducklake/test/regression/expected/inlined_scan_stats_guard.out new file mode 100644 index 00000000..d97b6069 --- /dev/null +++ b/pg_ducklake/test/regression/expected/inlined_scan_stats_guard.out @@ -0,0 +1,437 @@ +-- Regression: the inline (direct-insert) write path must maintain the persisted per-column +-- min/max in ducklake_table_column_stats so a later zone-map scan never mis-prunes its own rows. +-- +-- Background: DuckLake keeps per-table min/max in ducklake_table_column_stats and exposes them to +-- the scan as zone-map statistics (DuckLakeStatistics -> table.GetStatistics). pg_ducklake's +-- direct-insert path appends inlined rows; it must WIDEN those bounds to cover the new tuples. +-- A table seeded with real stats (min=1,max=5) that then receives direct-inserted rows outside +-- that range (100..104) must end up with a zone map of [1,104] -- extended, never stale, never +-- blanked. (Blanking min/max was spiked and rejected: it corrupts already-flushed parquet zone +-- maps. The bounds only ever widen.) Widening is also the only thing the writer does: a bound the +-- catalog does not have is never invented from a single batch, which the two half-populated cases +-- below assert from both directions. +-- +-- Previously a scan-side guard (ducklake-006-inlined-scan-stats-guard.patch) masked stale bounds by +-- suppressing stats for any table with inlined data. That guard is retired: the writer now keeps the +-- bounds truthful, so DuckLakeStatistics reverts to upstream behavior and reads stay correct while +-- retaining zone-map pruning. +\set ON_ERROR_STOP on +\pset pager off +-- Inlining on, limit high enough that the whole table stays inline. +CALL ducklake.set_option('data_inlining_row_limit', 1000); +-- ============================================================= +-- Case 1: fully inlined -- widened bounds, correct filtered scans +-- ============================================================= +CREATE TABLE issg (id INT) USING ducklake; +-- Seed via the DuckDB inline write path (not direct insert): persists min=1, max=5. +SET ducklake.enable_direct_insert = false; +INSERT INTO issg VALUES (1), (2), (3), (4), (5); +SET ducklake.enable_direct_insert = true; +-- The persisted zone map starts at [1, 5]. +SELECT min_value, max_value +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg'); + min_value | max_value +-----------+----------- + 1 | 5 +(1 row) + +-- Direct-insert rows ABOVE that max. The direct-insert path must widen the bounds. +INSERT INTO issg VALUES (100), (101), (102), (103), (104); +-- The persisted zone map is now the widened [1, 104] -- type-correct, not lexical, not NULL. +SELECT min_value, max_value +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg'); + min_value | max_value +-----------+----------- + 1 | 104 +(1 row) + +-- Data is still fully inlined (no parquet files). +SELECT count(*) AS data_files +FROM ducklake.ducklake_data_file +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg'); + data_files +------------ + 0 +(1 row) + +-- Zone-map-engaging scans on the affected column all see the out-of-range rows. +SELECT id FROM issg WHERE id > 50 ORDER BY id; + id +----- + 100 + 101 + 102 + 103 + 104 +(5 rows) + +SELECT id FROM issg WHERE id = 102; + id +----- + 102 +(1 row) + +SELECT max(id) AS true_max FROM issg; + true_max +---------- + 104 +(1 row) + +SELECT count(*) AS above_five FROM issg WHERE id > 5; + above_five +------------ + 5 +(1 row) + +SELECT id FROM issg ORDER BY id; + id +----- + 1 + 2 + 3 + 4 + 5 + 100 + 101 + 102 + 103 + 104 +(10 rows) + +DROP TABLE issg; +-- ============================================================= +-- Case 2: multi-file -- seed -> flush -> direct-insert -> flush. +-- The seed rows live in one parquet file, the direct-inserted rows in another. The table-level +-- zone map must cover the full [1,104] range so neither file's rows are mis-pruned. This is the +-- case the rejected "blank the bounds" shortcut corrupts. +-- ============================================================= +CREATE TABLE issg_mf (id INT) USING ducklake; +-- Seed inline, then flush to a parquet file. +SET ducklake.enable_direct_insert = false; +INSERT INTO issg_mf VALUES (1), (2), (3), (4), (5); +SET ducklake.enable_direct_insert = true; +SELECT count(*) > 0 AS flushed_seed FROM ducklake.flush_inlined_data('issg_mf'::regclass); + flushed_seed +-------------- + t +(1 row) + +-- Table-level bounds after the first flush. +SELECT min_value, max_value +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_mf'); + min_value | max_value +-----------+----------- + 1 | 5 +(1 row) + +-- Direct-insert out-of-range rows (inline), then flush them to a second parquet file. +INSERT INTO issg_mf VALUES (100), (101), (102), (103), (104); +SELECT count(*) > 0 AS flushed_direct FROM ducklake.flush_inlined_data('issg_mf'::regclass); + flushed_direct +---------------- + t +(1 row) + +-- Two parquet files now hold the data. +SELECT count(*) AS data_files +FROM ducklake.ducklake_data_file +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_mf'); + data_files +------------ + 2 +(1 row) + +-- Widened table-level zone map covers both files. +SELECT min_value, max_value +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_mf'); + min_value | max_value +-----------+----------- + 1 | 104 +(1 row) + +-- Zone-map-engaging scans across both files return the correct rows. +SELECT id FROM issg_mf WHERE id > 50 ORDER BY id; + id +----- + 100 + 101 + 102 + 103 + 104 +(5 rows) + +SELECT id FROM issg_mf WHERE id = 102; + id +----- + 102 +(1 row) + +SELECT max(id) AS true_max FROM issg_mf; + true_max +---------- + 104 +(1 row) + +SELECT count(*) AS above_five FROM issg_mf WHERE id > 5; + above_five +------------ + 5 +(1 row) + +SELECT id FROM issg_mf ORDER BY id; + id +----- + 1 + 2 + 3 + 4 + 5 + 100 + 101 + 102 + 103 + 104 +(10 rows) + +DROP TABLE issg_mf; +-- ------------------------------------------------------------------ +-- A stats row holding one bound but not the other must KEEP the NULL. +-- +-- MergeStats casts min and max independently when a column is retyped, so +-- a half-populated row is reachable through ordinary SQL. The widen must +-- leave the absent side absent: a NULL bound only costs pruning, while a +-- bound seeded from one insert batch is a claim about the whole table that +-- the batch cannot support -- and every already-written row outside it is +-- then pruned away on the next filtered scan. +-- +-- Reached below with no catalog edit: DATE -> TIMESTAMP where the persisted +-- max ('5874897-12-31') is outside TIMESTAMP range, so ReconcileStatToType +-- keeps min and drops max. +-- ------------------------------------------------------------------ +CREATE TABLE issg_half (d DATE) USING ducklake; +SET ducklake.enable_direct_insert = false; +INSERT INTO issg_half VALUES ('2024-01-01'), ('5874897-12-31'); +SELECT count(*) > 0 AS flushed_first FROM ducklake.flush_inlined_data('issg_half'::regclass); + flushed_first +--------------- + t +(1 row) + +INSERT INTO issg_half VALUES ('3000-01-01'); +SELECT count(*) > 0 AS flushed_second FROM ducklake.flush_inlined_data('issg_half'::regclass); + flushed_second +---------------- + t +(1 row) + +-- Delete the extreme rows. The persisted max still describes a deleted row -- +-- table-level stats are not recomputed on DELETE. +DELETE FROM issg_half WHERE d <= '2024-01-01' OR d > '4000-01-01'; +-- Retype and insert in one transaction: min casts to TIMESTAMP, max does not. +BEGIN; +ALTER TABLE issg_half ALTER COLUMN d TYPE TIMESTAMP; +INSERT INTO issg_half VALUES ('2025-01-01 00:00:00'); +COMMIT; +-- Half populated: min set, max NULL. +SELECT min_value, max_value AS half +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_half'); + min_value | half +---------------------+------ + 2024-01-01 00:00:00 | +(1 row) + +-- A direct insert whose own max is 2026-06-01. Seeding max from it would +-- claim the table has nothing after 2026, hiding the surviving 3000-01-01 row. +SET ducklake.enable_direct_insert = true; +INSERT INTO issg_half VALUES ('2026-06-01 00:00:00'); +-- max must still be NULL. min is untouched: the batch is above it. +SELECT min_value, max_value AS still_null +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_half'); + min_value | still_null +---------------------+------------ + 2024-01-01 00:00:00 | +(1 row) + +-- The row above the batch's max is still found, and all three rows come back. +SELECT count(*) AS above_2900 FROM issg_half WHERE d > '2900-01-01'; + above_2900 +------------ + 1 +(1 row) + +SELECT d FROM issg_half ORDER BY d; + d +-------------------------- + Wed Jan 01 00:00:00 2025 + Mon Jun 01 00:00:00 2026 + Wed Jan 01 00:00:00 3000 +(3 rows) + +DROP TABLE issg_half; +-- ------------------------------------------------------------------ +-- A too-narrow seeded min does not merely mis-prune: it corrupts the +-- values a plain unfiltered SELECT returns. +-- +-- DuckDB's compressed materialization uses the table-level min as an exact +-- constant to re-base the column, so a min above the true minimum shifts +-- every smaller value upward. With min seeded to 25 on a table holding +-- 10, 20, 30, the 10 and the 20 come back as 266 and 276 (+256) while +-- sum(id) still reports 125 -- easy to miss unless asserted. +-- +-- The NULL-min half of this state is not reachable from PG DDL today (the +-- retype direction above invalidates max, not min), so it is set directly. +-- ------------------------------------------------------------------ +CREATE TABLE issg_narrow (id INTEGER) USING ducklake; +INSERT INTO issg_narrow VALUES (10), (20), (30); +SELECT min_value, max_value AS seeded +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_narrow'); + min_value | seeded +-----------+-------- + 10 | 30 +(1 row) + +UPDATE ducklake.ducklake_table_column_stats SET min_value = NULL +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_narrow'); +-- The batch min (25) is ABOVE the table's true minimum (10). Seeding it here +-- is what corrupts the projection. +INSERT INTO issg_narrow VALUES (25), (40); +-- min stays NULL; max widens 30 -> 40. +SELECT min_value, max_value AS widened +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_narrow'); + min_value | widened +-----------+--------- + | 40 +(1 row) + +-- 10, 20, 25, 30, 40 -- not 25, 30, 40, 266, 276. +SELECT id FROM issg_narrow ORDER BY id; + id +---- + 10 + 20 + 25 + 30 + 40 +(5 rows) + +SELECT sum(id) AS total FROM issg_narrow; + total +------- + 125 +(1 row) + +DROP TABLE issg_narrow; +-- ------------------------------------------------------------------ +-- Persisted temporal bounds must not depend on the writing session. +-- +-- The accumulator renders temporal values through PG output functions, +-- which are DateStyle-dependent, so it forces ISO for the duration of +-- Finalize. Without that, a non-ISO session would persist bounds the +-- read side cannot parse back. Asserting it here keeps the guard from +-- being removed later as apparently-dead code. +-- ------------------------------------------------------------------ +SET DateStyle = 'German, DMY'; +CREATE TABLE issg_ds (d DATE, ts TIMESTAMP) USING ducklake; +INSERT INTO issg_ds VALUES ('2024-06-01', '2024-06-01 08:30:00'), + ('2024-07-15', '2024-07-15 22:15:00'); +-- ISO, despite the session rendering dates as 01.06.2024. +SELECT column_id, min_value, max_value +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_ds') +ORDER BY column_id; + column_id | min_value | max_value +-----------+---------------------+--------------------- + 1 | 2024-06-01 | 2024-07-15 + 2 | 2024-06-01 08:30:00 | 2024-07-15 22:15:00 +(2 rows) + +-- The session's own setting is left as the user set it. +SELECT current_setting('DateStyle') AS datestyle_after_insert; + datestyle_after_insert +------------------------ + German, DMY +(1 row) + +DROP TABLE issg_ds; +RESET DateStyle; +-- ------------------------------------------------------------------ +-- A scan taken BETWEEN two inline inserts must not serve stale bounds. +-- +-- DuckDB caches a table's zone-map statistics under the key +-- (next_file_id, schema_version, table_id). An inline insert writes no +-- parquet file and changes no schema, so neither component used to move: +-- within one session, a filtered scan run after the first insert cached +-- max=5, and the identical scan after the second insert reused that +-- entry and pruned away the freshly inlined 100..104. Truthful persisted +-- bounds are not enough on their own -- the reader never re-read them. +-- +-- The direct-insert commit therefore force-advances next_file_id, which +-- rotates the cache key on every inline write. The priming step has to +-- be a real scan of the table: selecting from ducklake_table_column_stats +-- reads metadata and never populates the cache, so a test that primes +-- that way passes even when the bug is present. +-- ------------------------------------------------------------------ +CREATE TABLE issg_cache (id INT) USING ducklake; +SET ducklake.enable_direct_insert = true; +INSERT INTO issg_cache VALUES (1), (2), (3), (4), (5); +-- Prime the stats cache: planning this scan caches max=5 for the table. +SELECT count(*) AS above_fifty_before FROM issg_cache WHERE id > 50; + above_fifty_before +-------------------- + 0 +(1 row) + +-- Remember where file ids stood before the second inline insert. +SELECT max(next_file_id) AS nfi_before FROM ducklake.ducklake_snapshot +\gset +INSERT INTO issg_cache VALUES (100), (101), (102), (103), (104); +-- The inline commit advanced next_file_id even though it wrote no file: +-- that is what rotates the cache key. (Boolean, not the id -- ids depend +-- on how many snapshots earlier tests took.) +SELECT max(next_file_id) > :nfi_before AS advanced FROM ducklake.ducklake_snapshot; + advanced +---------- + t +(1 row) + +-- ...and it really wrote no file: the whole table is still inlined. +SELECT count(*) AS data_files +FROM ducklake.ducklake_data_file +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_cache'); + data_files +------------ + 0 +(1 row) + +-- Same filtered scan as before, same session: it now sees the new rows. +-- Pre-fix this returned nothing. +SELECT id FROM issg_cache WHERE id > 50 ORDER BY id; + id +----- + 100 + 101 + 102 + 103 + 104 +(5 rows) + +-- Unfiltered, so no zone map is consulted: the rows were never missing, +-- only mis-pruned. Ten rows either way. +SELECT count(*) AS total FROM issg_cache; + total +------- + 10 +(1 row) + +DROP TABLE issg_cache; +-- Reset the session-global inlining limit so this test does not leak inlining +-- state into later tests in the schedule (matches the sibling inlining tests). +CALL ducklake.set_option('data_inlining_row_limit', 0); diff --git a/pg_ducklake/test/regression/schedule b/pg_ducklake/test/regression/schedule index 37c04c5f..ba8ca328 100644 --- a/pg_ducklake/test/regression/schedule +++ b/pg_ducklake/test/regression/schedule @@ -21,9 +21,11 @@ test: options test: data_inlining_row_limit test: create_with_options test: direct_insert_stats +test: direct_insert_metadata test: inlined_data_schema_change test: alter_partition_inlining test: inlined_flush_lost_update +test: inlined_scan_stats_guard test: types test: unresolved_type_ops test: decimal_precision diff --git a/pg_ducklake/test/regression/sql/direct_insert_metadata.sql b/pg_ducklake/test/regression/sql/direct_insert_metadata.sql new file mode 100644 index 00000000..4af2e7b4 --- /dev/null +++ b/pg_ducklake/test/regression/sql/direct_insert_metadata.sql @@ -0,0 +1,194 @@ +-- Direct-insert (fast path) metadata commit protocol. +-- The fast path hand-writes the DuckLake commit; these tests pin the parts of +-- the protocol other DuckLake machinery relies on. + +CALL ducklake.set_option('data_inlining_row_limit', 100); + +-- ============================================================ +-- 1. next_file_id must advance on a fast-path commit +-- ============================================================ +-- DuckLake keys its table-stats cache on (next_file_id, schema_version, +-- table_id) and bumps next_file_id on inlined-only commits to signal a data +-- change. A fast-path commit that carries it forward leaves stale cached +-- stats (next_row_id, record_count, min/max) in every open DuckDB instance. + +CREATE TABLE dim_t (id int, val text) USING ducklake; +INSERT INTO dim_t VALUES (0, 'seed'); -- normal path: creates the inlined data table + +SELECT next_file_id AS file_id_before +FROM ducklake.ducklake_snapshot ORDER BY snapshot_id DESC LIMIT 1 \gset + +SELECT ducklake.reset_direct_insert_stats(); +INSERT INTO dim_t VALUES (1, 'fast'); +-- fast path handled the insert above +SELECT pattern, reason, count FROM ducklake.direct_insert_stats() WHERE count > 0; + +SELECT next_file_id > :file_id_before AS file_id_advanced +FROM ducklake.ducklake_snapshot ORDER BY snapshot_id DESC LIMIT 1; + +-- ============================================================ +-- 2. row ids must stay unique when fast-path and normal-path +-- inserts interleave +-- ============================================================ +-- Load this backend's DuckDB table-stats cache, then advance next_row_id +-- through the fast path (PG metadata only). The normal-path insert below +-- must not seed its row ids from the stale cache entry. +SELECT count(*) FROM dim_t WHERE id >= 0; +INSERT INTO dim_t VALUES (2, 'fast2'); +BEGIN; -- transaction block: fast path disengages, normal DuckLake path +INSERT INTO dim_t VALUES (3, 'slow'); +COMMIT; + +SELECT it.table_name AS dim_inl +FROM ducklake.ducklake_inlined_data_tables it +JOIN ducklake.ducklake_table t USING (table_id) +WHERE t.table_name = 'dim_t' AND t.end_snapshot IS NULL +ORDER BY it.schema_version DESC LIMIT 1 \gset + +SELECT row_id, count(*) AS dup_count +FROM ducklake.:dim_inl GROUP BY row_id HAVING count(*) > 1; + +SELECT count(*) AS inlined_rows, count(DISTINCT row_id) AS distinct_row_ids +FROM ducklake.:dim_inl; + +-- deleting one row must not take an unrelated row with it +DELETE FROM dim_t WHERE id = 3; +SELECT * FROM dim_t ORDER BY id; + +-- ============================================================ +-- 3. global column stats must not claim ranges that exclude +-- fast-path rows +-- ============================================================ +-- The fast path maintains min/max by widening them to cover the rows it +-- writes, so the recorded range never excludes a committed row. Stale claims +-- are trusted by DuckLake readers of this catalog (global stats feed DuckDB +-- optimizer statistics, which fold provably-false filters) and are +-- perpetuated by later stats merges. + +CREATE TABLE stats_t (id int) USING ducklake; +INSERT INTO stats_t SELECT i FROM generate_series(1, 200) i; -- normal path, real stats + +-- cache the (accurate, soon stale) stats in this backend +SELECT count(*) FROM stats_t WHERE id = 150; + +SELECT ducklake.reset_direct_insert_stats(); +INSERT INTO stats_t VALUES (1000); -- outside the recorded min/max +INSERT INTO stats_t VALUES (NULL); -- would violate contains_null = false +SELECT pattern, reason, count FROM ducklake.direct_insert_stats() WHERE count > 0; + +-- A stale contains_null = false is not a lost optimization: the optimizer +-- folds IS NULL to false and the row disappears from the result. +SELECT s.min_value, s.max_value, s.contains_null +FROM ducklake.ducklake_table_column_stats s +JOIN ducklake.ducklake_table t ON t.table_id = s.table_id AND t.end_snapshot IS NULL +WHERE t.table_name = 'stats_t'; + +SELECT count(*) FROM stats_t WHERE id = 1000; +SELECT count(*) FROM stats_t WHERE id IS NULL; +SELECT count(*) FROM stats_t; + +-- ============================================================ +-- 4. contains_null is maintained for every column, not just +-- the ones whose type has a maintainable min/max +-- ============================================================ +-- Seeded from parquet so contains_null starts known-false. An inline-only +-- table would leave it unknown instead, which section 6 covers. +CALL ducklake.set_option('data_inlining_row_limit', 0); +CREATE TABLE nullstats (id int, v int, f double precision, w int, bs bytea) USING ducklake; +INSERT INTO nullstats SELECT i, i, i::float8, i, '\x01'::bytea FROM generate_series(1, 200) i; +CALL ducklake.set_option('data_inlining_row_limit', 100); +INSERT INTO nullstats VALUES (901, 7, 1.5, 7, '\x02'); -- normal path: creates the inlined data table + +-- f (DOUBLE) and bs (BLOB) are excluded from min/max by StatsEligible, so they +-- pin that null-ness is not gated by that exclusion. BLOB additionally is the +-- only fast-path-reachable type whose persisted row can carry extra_stats. +SELECT ducklake.reset_direct_insert_stats(); +INSERT INTO nullstats VALUES (902, NULL, NULL, 8, NULL), (903, NULL, NULL, 9, NULL); +SELECT pattern, reason, count FROM ducklake.direct_insert_stats() WHERE count > 0; + +-- v's bounds must stay at [1,200]: an all-NULL batch moves no bound. +SELECT s.column_id, s.contains_null, s.min_value, s.max_value +FROM ducklake.ducklake_table_column_stats s +JOIN ducklake.ducklake_table t ON t.table_id = s.table_id AND t.end_snapshot IS NULL +WHERE t.table_name = 'nullstats' ORDER BY s.column_id; + +SELECT count(*) FROM nullstats WHERE v IS NULL; +SELECT count(*) FROM nullstats WHERE f IS NULL; + +-- A batch with no NULLs must not reset it: the flip is one-way. +INSERT INTO nullstats VALUES (904, 8, 2.5, 10, '\x03'); +SELECT s.column_id, s.contains_null +FROM ducklake.ducklake_table_column_stats s +JOIN ducklake.ducklake_table t ON t.table_id = s.table_id AND t.end_snapshot IS NULL +WHERE t.table_name = 'nullstats' ORDER BY s.column_id; +SELECT count(*) FROM nullstats WHERE v IS NULL; + +-- w's first NULL, so column 4 must flip here and nowhere earlier -- otherwise +-- this asserts nothing about the COPY writer. +COPY nullstats (id, v, f, w, bs) FROM STDIN WITH (FORMAT csv, NULL ''); +905,,3.5,,\x04 +\. +SELECT s.column_id, s.contains_null +FROM ducklake.ducklake_table_column_stats s +JOIN ducklake.ducklake_table t ON t.table_id = s.table_id AND t.end_snapshot IS NULL +WHERE t.table_name = 'nullstats' ORDER BY s.column_id; +SELECT count(*) FROM nullstats WHERE v IS NULL; +SELECT count(*) FROM nullstats WHERE w IS NULL; +SELECT count(*) FROM nullstats; + +-- ============================================================ +-- 5. the UNNEST writer maintains it too +-- ============================================================ +-- Its own null branch, so a fix applied to the VALUES and COPY writers can miss +-- it. Needs a dedicated table: an UNNEST naming a column subset does not reach +-- the fast path at all. +CALL ducklake.set_option('data_inlining_row_limit', 0); +CREATE TABLE nullstats_un (id int, v int) USING ducklake; +INSERT INTO nullstats_un SELECT i, i FROM generate_series(1, 200) i; +CALL ducklake.set_option('data_inlining_row_limit', 100); +INSERT INTO nullstats_un VALUES (901, 7); + +PREPARE nullstats_un_ins (int[], int[]) AS + INSERT INTO nullstats_un (id, v) SELECT UNNEST($1), UNNEST($2); +SELECT ducklake.reset_direct_insert_stats(); +EXECUTE nullstats_un_ins(ARRAY[902, 903], ARRAY[NULL, NULL]::int[]); +SELECT pattern, reason, count FROM ducklake.direct_insert_stats() WHERE count > 0; + +SELECT s.column_id, s.contains_null, s.min_value, s.max_value +FROM ducklake.ducklake_table_column_stats s +JOIN ducklake.ducklake_table t ON t.table_id = s.table_id AND t.end_snapshot IS NULL +WHERE t.table_name = 'nullstats_un' ORDER BY s.column_id; +SELECT count(*) FROM nullstats_un WHERE v IS NULL; +SELECT count(*) FROM nullstats_un; +DEALLOCATE nullstats_un_ins; + +-- ============================================================ +-- 6. a table that never flushed keeps contains_null unknown +-- ============================================================ +-- The commonest fast-path shape, and the one where correctness rests on the +-- value staying unknown rather than being maintained: MergeStats' has_null_count +-- degrade is sticky, so no widen can ever write it. Seeding false here instead +-- would fold IS NULL to false and lose rows. +CALL ducklake.set_option('data_inlining_row_limit', 1000); +CREATE TABLE nullstats_inl (id int, v int) USING ducklake; +INSERT INTO nullstats_inl VALUES (1, 1); -- normal path: creates the inlined data table +SELECT ducklake.reset_direct_insert_stats(); +INSERT INTO nullstats_inl VALUES (2, NULL), (3, 3); +SELECT pattern, reason, count FROM ducklake.direct_insert_stats() WHERE count > 0; + +SELECT s.column_id, s.contains_null IS NULL AS unknown +FROM ducklake.ducklake_table_column_stats s +JOIN ducklake.ducklake_table t ON t.table_id = s.table_id AND t.end_snapshot IS NULL +WHERE t.table_name = 'nullstats_inl' ORDER BY s.column_id; +SELECT count(*) FROM nullstats_inl WHERE v IS NULL; + +SELECT * FROM ducklake.flush_inlined_data('nullstats_inl'::regclass); +SELECT count(*) FROM nullstats_inl WHERE v IS NULL; + +-- Cleanup +DROP TABLE dim_t; +DROP TABLE stats_t; +DROP TABLE nullstats; +DROP TABLE nullstats_un; +DROP TABLE nullstats_inl; +CALL ducklake.set_option('data_inlining_row_limit', 0); diff --git a/pg_ducklake/test/regression/sql/inlined_scan_stats_guard.sql b/pg_ducklake/test/regression/sql/inlined_scan_stats_guard.sql new file mode 100644 index 00000000..ce961365 --- /dev/null +++ b/pg_ducklake/test/regression/sql/inlined_scan_stats_guard.sql @@ -0,0 +1,277 @@ +-- Regression: the inline (direct-insert) write path must maintain the persisted per-column +-- min/max in ducklake_table_column_stats so a later zone-map scan never mis-prunes its own rows. +-- +-- Background: DuckLake keeps per-table min/max in ducklake_table_column_stats and exposes them to +-- the scan as zone-map statistics (DuckLakeStatistics -> table.GetStatistics). pg_ducklake's +-- direct-insert path appends inlined rows; it must WIDEN those bounds to cover the new tuples. +-- A table seeded with real stats (min=1,max=5) that then receives direct-inserted rows outside +-- that range (100..104) must end up with a zone map of [1,104] -- extended, never stale, never +-- blanked. (Blanking min/max was spiked and rejected: it corrupts already-flushed parquet zone +-- maps. The bounds only ever widen.) Widening is also the only thing the writer does: a bound the +-- catalog does not have is never invented from a single batch, which the two half-populated cases +-- below assert from both directions. +-- +-- Previously a scan-side guard (ducklake-006-inlined-scan-stats-guard.patch) masked stale bounds by +-- suppressing stats for any table with inlined data. That guard is retired: the writer now keeps the +-- bounds truthful, so DuckLakeStatistics reverts to upstream behavior and reads stay correct while +-- retaining zone-map pruning. + +\set ON_ERROR_STOP on +\pset pager off + +-- Inlining on, limit high enough that the whole table stays inline. +CALL ducklake.set_option('data_inlining_row_limit', 1000); + +-- ============================================================= +-- Case 1: fully inlined -- widened bounds, correct filtered scans +-- ============================================================= + +CREATE TABLE issg (id INT) USING ducklake; + +-- Seed via the DuckDB inline write path (not direct insert): persists min=1, max=5. +SET ducklake.enable_direct_insert = false; +INSERT INTO issg VALUES (1), (2), (3), (4), (5); +SET ducklake.enable_direct_insert = true; + +-- The persisted zone map starts at [1, 5]. +SELECT min_value, max_value +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg'); + +-- Direct-insert rows ABOVE that max. The direct-insert path must widen the bounds. +INSERT INTO issg VALUES (100), (101), (102), (103), (104); + +-- The persisted zone map is now the widened [1, 104] -- type-correct, not lexical, not NULL. +SELECT min_value, max_value +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg'); + +-- Data is still fully inlined (no parquet files). +SELECT count(*) AS data_files +FROM ducklake.ducklake_data_file +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg'); + +-- Zone-map-engaging scans on the affected column all see the out-of-range rows. +SELECT id FROM issg WHERE id > 50 ORDER BY id; +SELECT id FROM issg WHERE id = 102; +SELECT max(id) AS true_max FROM issg; +SELECT count(*) AS above_five FROM issg WHERE id > 5; +SELECT id FROM issg ORDER BY id; + +DROP TABLE issg; + +-- ============================================================= +-- Case 2: multi-file -- seed -> flush -> direct-insert -> flush. +-- The seed rows live in one parquet file, the direct-inserted rows in another. The table-level +-- zone map must cover the full [1,104] range so neither file's rows are mis-pruned. This is the +-- case the rejected "blank the bounds" shortcut corrupts. +-- ============================================================= + +CREATE TABLE issg_mf (id INT) USING ducklake; + +-- Seed inline, then flush to a parquet file. +SET ducklake.enable_direct_insert = false; +INSERT INTO issg_mf VALUES (1), (2), (3), (4), (5); +SET ducklake.enable_direct_insert = true; +SELECT count(*) > 0 AS flushed_seed FROM ducklake.flush_inlined_data('issg_mf'::regclass); + +-- Table-level bounds after the first flush. +SELECT min_value, max_value +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_mf'); + +-- Direct-insert out-of-range rows (inline), then flush them to a second parquet file. +INSERT INTO issg_mf VALUES (100), (101), (102), (103), (104); +SELECT count(*) > 0 AS flushed_direct FROM ducklake.flush_inlined_data('issg_mf'::regclass); + +-- Two parquet files now hold the data. +SELECT count(*) AS data_files +FROM ducklake.ducklake_data_file +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_mf'); + +-- Widened table-level zone map covers both files. +SELECT min_value, max_value +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_mf'); + +-- Zone-map-engaging scans across both files return the correct rows. +SELECT id FROM issg_mf WHERE id > 50 ORDER BY id; +SELECT id FROM issg_mf WHERE id = 102; +SELECT max(id) AS true_max FROM issg_mf; +SELECT count(*) AS above_five FROM issg_mf WHERE id > 5; +SELECT id FROM issg_mf ORDER BY id; + +DROP TABLE issg_mf; + +-- ------------------------------------------------------------------ +-- A stats row holding one bound but not the other must KEEP the NULL. +-- +-- MergeStats casts min and max independently when a column is retyped, so +-- a half-populated row is reachable through ordinary SQL. The widen must +-- leave the absent side absent: a NULL bound only costs pruning, while a +-- bound seeded from one insert batch is a claim about the whole table that +-- the batch cannot support -- and every already-written row outside it is +-- then pruned away on the next filtered scan. +-- +-- Reached below with no catalog edit: DATE -> TIMESTAMP where the persisted +-- max ('5874897-12-31') is outside TIMESTAMP range, so ReconcileStatToType +-- keeps min and drops max. +-- ------------------------------------------------------------------ +CREATE TABLE issg_half (d DATE) USING ducklake; + +SET ducklake.enable_direct_insert = false; +INSERT INTO issg_half VALUES ('2024-01-01'), ('5874897-12-31'); +SELECT count(*) > 0 AS flushed_first FROM ducklake.flush_inlined_data('issg_half'::regclass); +INSERT INTO issg_half VALUES ('3000-01-01'); +SELECT count(*) > 0 AS flushed_second FROM ducklake.flush_inlined_data('issg_half'::regclass); + +-- Delete the extreme rows. The persisted max still describes a deleted row -- +-- table-level stats are not recomputed on DELETE. +DELETE FROM issg_half WHERE d <= '2024-01-01' OR d > '4000-01-01'; + +-- Retype and insert in one transaction: min casts to TIMESTAMP, max does not. +BEGIN; +ALTER TABLE issg_half ALTER COLUMN d TYPE TIMESTAMP; +INSERT INTO issg_half VALUES ('2025-01-01 00:00:00'); +COMMIT; + +-- Half populated: min set, max NULL. +SELECT min_value, max_value AS half +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_half'); + +-- A direct insert whose own max is 2026-06-01. Seeding max from it would +-- claim the table has nothing after 2026, hiding the surviving 3000-01-01 row. +SET ducklake.enable_direct_insert = true; +INSERT INTO issg_half VALUES ('2026-06-01 00:00:00'); + +-- max must still be NULL. min is untouched: the batch is above it. +SELECT min_value, max_value AS still_null +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_half'); + +-- The row above the batch's max is still found, and all three rows come back. +SELECT count(*) AS above_2900 FROM issg_half WHERE d > '2900-01-01'; +SELECT d FROM issg_half ORDER BY d; + +DROP TABLE issg_half; + +-- ------------------------------------------------------------------ +-- A too-narrow seeded min does not merely mis-prune: it corrupts the +-- values a plain unfiltered SELECT returns. +-- +-- DuckDB's compressed materialization uses the table-level min as an exact +-- constant to re-base the column, so a min above the true minimum shifts +-- every smaller value upward. With min seeded to 25 on a table holding +-- 10, 20, 30, the 10 and the 20 come back as 266 and 276 (+256) while +-- sum(id) still reports 125 -- easy to miss unless asserted. +-- +-- The NULL-min half of this state is not reachable from PG DDL today (the +-- retype direction above invalidates max, not min), so it is set directly. +-- ------------------------------------------------------------------ +CREATE TABLE issg_narrow (id INTEGER) USING ducklake; +INSERT INTO issg_narrow VALUES (10), (20), (30); + +SELECT min_value, max_value AS seeded +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_narrow'); + +UPDATE ducklake.ducklake_table_column_stats SET min_value = NULL +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_narrow'); + +-- The batch min (25) is ABOVE the table's true minimum (10). Seeding it here +-- is what corrupts the projection. +INSERT INTO issg_narrow VALUES (25), (40); + +-- min stays NULL; max widens 30 -> 40. +SELECT min_value, max_value AS widened +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_narrow'); + +-- 10, 20, 25, 30, 40 -- not 25, 30, 40, 266, 276. +SELECT id FROM issg_narrow ORDER BY id; +SELECT sum(id) AS total FROM issg_narrow; + +DROP TABLE issg_narrow; + +-- ------------------------------------------------------------------ +-- Persisted temporal bounds must not depend on the writing session. +-- +-- The accumulator renders temporal values through PG output functions, +-- which are DateStyle-dependent, so it forces ISO for the duration of +-- Finalize. Without that, a non-ISO session would persist bounds the +-- read side cannot parse back. Asserting it here keeps the guard from +-- being removed later as apparently-dead code. +-- ------------------------------------------------------------------ +SET DateStyle = 'German, DMY'; + +CREATE TABLE issg_ds (d DATE, ts TIMESTAMP) USING ducklake; +INSERT INTO issg_ds VALUES ('2024-06-01', '2024-06-01 08:30:00'), + ('2024-07-15', '2024-07-15 22:15:00'); + +-- ISO, despite the session rendering dates as 01.06.2024. +SELECT column_id, min_value, max_value +FROM ducklake.ducklake_table_column_stats +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_ds') +ORDER BY column_id; + +-- The session's own setting is left as the user set it. +SELECT current_setting('DateStyle') AS datestyle_after_insert; + +DROP TABLE issg_ds; +RESET DateStyle; + +-- ------------------------------------------------------------------ +-- A scan taken BETWEEN two inline inserts must not serve stale bounds. +-- +-- DuckDB caches a table's zone-map statistics under the key +-- (next_file_id, schema_version, table_id). An inline insert writes no +-- parquet file and changes no schema, so neither component used to move: +-- within one session, a filtered scan run after the first insert cached +-- max=5, and the identical scan after the second insert reused that +-- entry and pruned away the freshly inlined 100..104. Truthful persisted +-- bounds are not enough on their own -- the reader never re-read them. +-- +-- The direct-insert commit therefore force-advances next_file_id, which +-- rotates the cache key on every inline write. The priming step has to +-- be a real scan of the table: selecting from ducklake_table_column_stats +-- reads metadata and never populates the cache, so a test that primes +-- that way passes even when the bug is present. +-- ------------------------------------------------------------------ +CREATE TABLE issg_cache (id INT) USING ducklake; + +SET ducklake.enable_direct_insert = true; +INSERT INTO issg_cache VALUES (1), (2), (3), (4), (5); + +-- Prime the stats cache: planning this scan caches max=5 for the table. +SELECT count(*) AS above_fifty_before FROM issg_cache WHERE id > 50; + +-- Remember where file ids stood before the second inline insert. +SELECT max(next_file_id) AS nfi_before FROM ducklake.ducklake_snapshot +\gset + +INSERT INTO issg_cache VALUES (100), (101), (102), (103), (104); + +-- The inline commit advanced next_file_id even though it wrote no file: +-- that is what rotates the cache key. (Boolean, not the id -- ids depend +-- on how many snapshots earlier tests took.) +SELECT max(next_file_id) > :nfi_before AS advanced FROM ducklake.ducklake_snapshot; + +-- ...and it really wrote no file: the whole table is still inlined. +SELECT count(*) AS data_files +FROM ducklake.ducklake_data_file +WHERE table_id = (SELECT table_id FROM ducklake.ducklake_table WHERE table_name = 'issg_cache'); + +-- Same filtered scan as before, same session: it now sees the new rows. +-- Pre-fix this returned nothing. +SELECT id FROM issg_cache WHERE id > 50 ORDER BY id; + +-- Unfiltered, so no zone map is consulted: the rows were never missing, +-- only mis-pruned. Ten rows either way. +SELECT count(*) AS total FROM issg_cache; + +DROP TABLE issg_cache; + +-- Reset the session-global inlining limit so this test does not leak inlining +-- state into later tests in the schedule (matches the sibling inlining tests). +CALL ducklake.set_option('data_inlining_row_limit', 0); diff --git a/pg_ducklake/third_party/ducklake-006-inlined-scan-stats-guard.patch b/pg_ducklake/third_party/ducklake-006-inlined-scan-stats-guard.patch deleted file mode 100644 index 3be36697..00000000 --- a/pg_ducklake/third_party/ducklake-006-inlined-scan-stats-guard.patch +++ /dev/null @@ -1,20 +0,0 @@ -diff --git a/src/storage/ducklake_scan.cpp b/src/storage/ducklake_scan.cpp -index c0e4cf4..17fb9b5 100644 ---- a/src/storage/ducklake_scan.cpp -+++ b/src/storage/ducklake_scan.cpp -@@ -104,6 +104,15 @@ unique_ptr DuckLakeStatistics(ClientContext &context, const Func - return nullptr; - } - auto &table = file_list.GetTable(); -+ if (!table.GetInlinedDataTables().empty()) { -+ // Tables with inlined data are scanned row-by-row out of the metadata catalog. pg_ducklake's -+ // direct-insert path does not maintain ducklake_table_column_stats min/max for those inlined -+ // rows, so the persisted column stats can be inconsistent with the inlined heap and drive a -+ // zone-map mis-decode on read. Skip stats for such tables -- they are small (under the -+ // data_inlining_row_limit), so the lost pruning is negligible. -+ // FIXME: maintain inlined-data column stats so this can use the global stats instead. -+ return nullptr; -+ } - return table.GetStatistics(context, column_index); - } -