diff --git a/src/columnar_write_state.c b/src/columnar_write_state.c index 8d69b3c..27e74ce 100644 --- a/src/columnar_write_state.c +++ b/src/columnar_write_state.c @@ -31,12 +31,41 @@ #include "utils/snapmgr.h" #include "utils/typcache.h" +/* + * Direct comparison kinds for the zone min/max (issue #155). + * + * Tracking a chunk's min and max costs two comparisons per value per column, and + * routing each through fmgr is most of what they cost: removing min/max tracking + * entirely saved 15% of a five-int-column load, where the comparison itself is a + * subtraction. These are the types whose stored Datum can be read as a C value + * and compared without a collation, which is the same condition, and the same + * list, the vectorized filter's fast path uses (COLUMNAR_VECFAST_* in + * columnar_vector.c). Anything else keeps the fmgr path. + * + * float4 and float8 are deliberately NOT here even though the filter fast path + * takes them. btree float ordering puts NaN above every other value, which a C + * comparison gets wrong: every comparison against NaN is false, so NaN would read + * as equal and never become a chunk's maximum. A zone map with a maximum that is + * too low makes the reader skip a row group that does hold matching rows, and the + * query silently returns fewer rows. The filter path can take the same shortcut + * because it compares to answer one predicate, not to build stored bounds that a + * later scan trusts. + */ +typedef enum ColumnarFastCmp +{ + COLUMNAR_FASTCMP_NONE = 0, + COLUMNAR_FASTCMP_I16, + COLUMNAR_FASTCMP_I32, + COLUMNAR_FASTCMP_I64 +} ColumnarFastCmp; + /* per-column, per-write-state facts needed for the min/max skip list */ typedef struct ColumnarColumnDef { bool orderable; /* type has a default btree comparison proc */ FmgrInfo cmpFn; /* the comparison proc, when orderable */ Oid collation; /* collation to compare under */ + ColumnarFastCmp fastCmp; /* direct comparison, when the type allows */ /* * int2/int4 column: its exact sum fits an int64 accumulator, so the zone map @@ -132,6 +161,46 @@ static void flush_ws_projections(ColumnarWriteState *writeState); static ChunkGroupBuffer *columnar_start_chunk_group(ColumnarWriteState *writeState); static void columnar_init_col_defs(ColumnarWriteState *writeState); +/* + * columnar_cmp_value + * Compare two values of a column under its ordering, taking the direct + * route when the type allows one and fmgr otherwise. The integer kinds + * reproduce their btree comparison exactly, so which route is taken can + * never change the answer. + */ +static inline int32 +columnar_cmp_value(ColumnarColumnDef *def, Datum a, Datum b) +{ + switch (def->fastCmp) + { + case COLUMNAR_FASTCMP_I16: + { + int16 x = DatumGetInt16(a); + int16 y = DatumGetInt16(b); + + return (x < y) ? -1 : (x > y) ? 1 : 0; + } + case COLUMNAR_FASTCMP_I32: + { + int32 x = DatumGetInt32(a); + int32 y = DatumGetInt32(b); + + return (x < y) ? -1 : (x > y) ? 1 : 0; + } + case COLUMNAR_FASTCMP_I64: + { + int64 x = DatumGetInt64(a); + int64 y = DatumGetInt64(b); + + return (x < y) ? -1 : (x > y) ? 1 : 0; + } + case COLUMNAR_FASTCMP_NONE: + break; + } + + return DatumGetInt32(FunctionCall2Coll(&def->cmpFn, def->collation, a, b)); +} + /* * columnar_init_col_defs * Allocate and fill writeState->colDefs: for each column, resolve the btree @@ -163,6 +232,31 @@ columnar_init_col_defs(ColumnarWriteState *writeState) fmgr_info_copy(&writeState->colDefs[c].cmpFn, &tce->cmp_proc_finfo, ColumnarWriteContext); writeState->colDefs[c].collation = att->attcollation; + + /* + * Resolve a direct comparison where the type permits one. These + * compare byte-for-byte under any collation, so the fast path + * cannot disagree with the operator it replaces; the zone map it + * feeds is read back through the same ordering. + */ + switch (att->atttypid) + { + case INT2OID: + writeState->colDefs[c].fastCmp = COLUMNAR_FASTCMP_I16; + break; + case INT4OID: + case DATEOID: + writeState->colDefs[c].fastCmp = COLUMNAR_FASTCMP_I32; + break; + case INT8OID: + case TIMESTAMPOID: + case TIMESTAMPTZOID: + writeState->colDefs[c].fastCmp = COLUMNAR_FASTCMP_I64; + break; + default: + writeState->colDefs[c].fastCmp = COLUMNAR_FASTCMP_NONE; + break; + } } /* int2/int4: exact sum fits int64, carried in the zone map (D5) */ @@ -414,25 +508,30 @@ ColumnarWriteRow(ColumnarWriteState *writeState, Relation rel, } else { - int32 cmpMin = DatumGetInt32( - FunctionCall2Coll(&def->cmpFn, def->collation, - values[c], col->minValue)); - int32 cmpMax = DatumGetInt32( - FunctionCall2Coll(&def->cmpFn, def->collation, - values[c], col->maxValue)); - - if (cmpMin < 0) + /* + * A value above the running maximum cannot also be below the + * running minimum, so the second comparison is only needed + * when the first does not settle it. Testing the maximum + * first makes the ascending case -- which is what a bulk load + * of a serial or timestamp column produces -- cost one + * comparison per value instead of two. + */ + int32 cmpMax = columnar_cmp_value(def, values[c], + col->maxValue); + + if (cmpMax > 0) { if (!att->attbyval) - pfree(DatumGetPointer(col->minValue)); - col->minValue = datumCopy(values[c], att->attbyval, + pfree(DatumGetPointer(col->maxValue)); + col->maxValue = datumCopy(values[c], att->attbyval, att->attlen); } - if (cmpMax > 0) + else if (columnar_cmp_value(def, values[c], + col->minValue) < 0) { if (!att->attbyval) - pfree(DatumGetPointer(col->maxValue)); - col->maxValue = datumCopy(values[c], att->attbyval, + pfree(DatumGetPointer(col->minValue)); + col->minValue = datumCopy(values[c], att->attbyval, att->attlen); } } diff --git a/test/write_minmax_fastpath.sh b/test/write_minmax_fastpath.sh new file mode 100755 index 0000000..0cc0d2d --- /dev/null +++ b/test/write_minmax_fastpath.sh @@ -0,0 +1,177 @@ +#!/usr/bin/env bash +# +# pgColumnar zone min/max via the direct comparison path (issue #155). +# +# Tracking a chunk's min and max costs two comparisons per value per column, and +# routing both through fmgr is most of what they cost. Integer-family columns now +# compare directly, and the maximum is tested first so an ascending load -- what a +# bulk load of a serial or timestamp column produces -- needs one comparison per +# value rather than two. +# +# The risk this carries is not a crash. A zone map drives row-group skipping, so a +# minimum that is too high or a maximum that is too low makes the reader skip a +# group that does hold matching rows and the query silently returns fewer. That is +# the failure this file is built to catch, so the checks are differential against +# a heap mirror over predicates that sit exactly on the chunk bounds. +# +# The float case has its own check and is the reason floats are deliberately NOT +# on the fast path. btree float ordering puts NaN above every other value; a C +# comparison gets that wrong, because every comparison against NaN is false, so +# NaN would read as equal and never become a chunk's maximum. A later change that +# adds float4/float8 to the fast list fails here rather than silently losing rows. +# +# Usage: test/write_minmax_fastpath.sh [PG_CONFIG] +# Written fresh for pgColumnar. + +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" + +ROWS=${PGC_MINMAX_ROWS:-200000} + +# small chunk groups so there are many zone maps to get wrong, and so a predicate +# can fall inside one group and outside its neighbours +psql_run "DROP TABLE IF EXISTS mm_c; DROP TABLE IF EXISTS mm_h; + SET pgcolumnar.stripe_row_limit = 20000; + SET pgcolumnar.chunk_group_row_limit = 5000; + CREATE TABLE mm_c ( + i2 smallint, i4 int, i8 bigint, + d date, ts timestamp, tz timestamptz, + asc_i int, desc_i int, rand_i int, neg_i int, + nullable_i int, t text + ) USING pgcolumnar; + INSERT INTO mm_c SELECT + (g % 30000)::smallint, + g, + g::bigint * 1000000, + '2000-01-01'::date + (g % 10000), + '2000-01-01'::timestamp + (g || ' sec')::interval, + '2000-01-01'::timestamptz + (g || ' sec')::interval, + g, + $ROWS - g, + (g * 7919) % 100000, + -g, + CASE WHEN g % 5 = 0 THEN NULL ELSE g END, + 't' || g + FROM generate_series(1, $ROWS) g; + CREATE TABLE mm_h (LIKE mm_c); + INSERT INTO mm_h SELECT * FROM mm_c;" >/dev/null 2>&1 + +echo "-- $ROWS rows, $(q "SELECT count(*) FROM pgcolumnar.row_group r + JOIN pgcolumnar.storage s ON s.storage_id = r.storage_id + WHERE s.relation_oid = 'mm_c'::regclass;") row groups" + +# --- 1. every column agrees with heap under range predicates ------------------- + +# Predicates chosen to land on and around chunk-group boundaries, which is where a +# botched bound shows up: a group is skipped or kept wrongly only at its edges. +diff_count() { # label, predicate + local c h + c="$(q "SELECT count(*) FROM mm_c WHERE $2;")" + h="$(q "SELECT count(*) FROM mm_h WHERE $2;")" + [ "$c" = "$h" ] || echo "$1(columnar=$c heap=$h)" +} + +bad="" +bad="$bad $(diff_count i4_eq "i4 = 5000")" +bad="$bad $(diff_count i4_bound "i4 BETWEEN 4999 AND 5001")" +bad="$bad $(diff_count i4_range "i4 > $((ROWS / 2)) AND i4 < $((ROWS / 2 + 17))")" +bad="$bad $(diff_count i2_eq "i2 = 12345")" +bad="$bad $(diff_count i2_range "i2 BETWEEN 100 AND 200")" +bad="$bad $(diff_count i8_range "i8 > 4999000000 AND i8 < 5001000000")" +bad="$bad $(diff_count date_range "d BETWEEN '2000-06-01' AND '2000-06-03'")" +bad="$bad $(diff_count ts_range "ts > '2000-01-01 01:00:00' AND ts < '2000-01-01 01:00:10'")" +bad="$bad $(diff_count tz_range "tz > '2000-01-01 01:00:00+00' AND tz < '2000-01-01 01:00:10+00'")" +bad="$bad $(diff_count asc_range "asc_i BETWEEN 19999 AND 20001")" +bad="$bad $(diff_count desc_range "desc_i BETWEEN 19999 AND 20001")" +bad="$bad $(diff_count rand_range "rand_i BETWEEN 500 AND 600")" +bad="$bad $(diff_count neg_range "neg_i BETWEEN -20001 AND -19999")" +bad="$bad $(diff_count null_range "nullable_i BETWEEN 4999 AND 5001")" +bad="$bad $(diff_count null_isnull "nullable_i IS NULL")" +bad="$bad $(diff_count text_eq "t = 't5000'")" + +check "every column returns the same rows as heap under range predicates" \ + "$(echo $bad | tr -s ' ')" "" + +# --- 2. the stored bounds are the true bounds ---------------------------------- + +# min/max read straight back out, which is what the zone maps hold. An aggregate +# over a column with no deletes is answered from those zone maps, so this reads +# the stored bounds rather than the data. +bad="" +for col in i2 i4 i8 d ts tz asc_i desc_i rand_i neg_i nullable_i t; do + c="$(q "SELECT min($col)::text || '|' || max($col)::text FROM mm_c;")" + h="$(q "SELECT min($col)::text || '|' || max($col)::text FROM mm_h;")" + [ "$c" = "$h" ] || bad="$bad $col(columnar=$c heap=$h)" +done +check "min and max match heap on every column" "${bad:-same}" "same" + +# --- 3. skipping still happens ------------------------------------------------- + +# A fast path that quietly widened every bound would pass checks 1 and 2 while +# giving up the skipping the zone maps exist for, so assert groups are still +# ruled out. +skipped="$(q "EXPLAIN (ANALYZE, COSTS off, TIMING off, SUMMARY off) + SELECT count(*) FROM mm_c WHERE asc_i BETWEEN 19999 AND 20001;" \ + | grep -oE 'Columnar Chunk Groups Removed by Filter: [0-9]+' \ + | grep -oE '[0-9]+$' | head -1)" +echo "-- chunk groups skipped for a narrow range: ${skipped:-none reported}" + +check "a narrow range still skips row groups" \ + "$( [ -n "${skipped:-}" ] && [ "$skipped" -gt 0 ] && echo yes || echo "no (${skipped:-unreported})")" \ + "yes" + +# --- 4. floats keep their btree ordering, NaN included ------------------------- + +psql_run "DROP TABLE IF EXISTS mm_f; DROP TABLE IF EXISTS mm_fh; + SET pgcolumnar.stripe_row_limit = 20000; + SET pgcolumnar.chunk_group_row_limit = 20000; + CREATE TABLE mm_f (f4 real, f8 double precision) USING pgcolumnar; + INSERT INTO mm_f SELECT + CASE WHEN g = 5001 THEN 'NaN'::real + WHEN g = 5002 THEN 'Infinity'::real + WHEN g = 5003 THEN '-Infinity'::real + ELSE g::real END, + CASE WHEN g = 5001 THEN 'NaN'::float8 + WHEN g = 5002 THEN 'Infinity'::float8 + WHEN g = 5003 THEN '-Infinity'::float8 + ELSE g::float8 END + FROM generate_series(1, 5003) g; + CREATE TABLE mm_fh (LIKE mm_f); + INSERT INTO mm_fh SELECT * FROM mm_f;" >/dev/null 2>&1 + +# The special values sit at the END of a single chunk group, on purpose. NaN +# arriving first would become that chunk's min and max before any comparison ran, +# and every check below would pass whatever the comparison did -- which is what an +# earlier version of this fixture did, and it let a deliberately broken build +# through. +check "the float fixture is one chunk group" \ + "$(q "SELECT count(*) FROM pgcolumnar.row_group r + JOIN pgcolumnar.storage s ON s.storage_id = r.storage_id + WHERE s.relation_oid = 'mm_f'::regclass;")" "1" + +# btree orders NaN above everything, so it is the maximum of both columns. A C +# comparison would never make it one, and then a query for it can be skipped. +check "max of a float column with NaN matches heap" \ + "$(q "SELECT max(f8)::text FROM mm_f;")" "$(q "SELECT max(f8)::text FROM mm_fh;")" + +check "max of a real column with NaN matches heap" \ + "$(q "SELECT max(f4)::text FROM mm_f;")" "$(q "SELECT max(f4)::text FROM mm_fh;")" + +check "a row holding NaN is still found by an equality predicate" \ + "$(q "SELECT count(*) FROM mm_f WHERE f8 = 'NaN';")" \ + "$(q "SELECT count(*) FROM mm_fh WHERE f8 = 'NaN';")" + +check "infinities are found too" \ + "$(q "SELECT count(*) FROM mm_f WHERE f8 = 'Infinity' OR f8 = '-Infinity';")" \ + "$(q "SELECT count(*) FROM mm_fh WHERE f8 = 'Infinity' OR f8 = '-Infinity';")" + +# --- 5. the fast path is restricted to the types that can take it -------------- + +SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/src" + +check "float types are not on the direct comparison list" \ + "$(grep -cE 'COLUMNAR_FASTCMP_F(32|64)' "$SRC/columnar_write_state.c")" "0" + +pgc_summary