diff --git a/src/columnar.h b/src/columnar.h index cfee55e..d101d29 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -528,6 +528,26 @@ extern void ColumnarFreeLivenessCache(ColumnarLivenessCache *cache); extern bool ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, Datum *values, bool *nulls); +/* + * Decode exactly the columns in `needed` (0-based attnums); every other column + * reads as null. An empty or NULL set decodes nothing, which is what it says: + * this deliberately has no "NULL means all" convention, because a Bitmapset + * cannot distinguish empty from NULL, so a caller whose computed set came out + * empty would silently get the opposite of what it asked for. For every column + * call ColumnarReadRowByNumber, which takes no set. + * + * Decoding every column regardless makes a wide table exceed the fetch cache's + * size cap, so the entry is dropped after every fetch and the group is decoded + * again for the next row (issue #157). + */ +extern bool ColumnarReadRowByNumberCols(Relation rel, Snapshot snapshot, + uint64 rowNumber, Datum *values, + bool *nulls, Bitmapset *needed); + +/* Is the row visible? Decodes nothing. */ +extern bool ColumnarRowIsLive(Relation rel, Snapshot snapshot, + uint64 rowNumber); + /* ------------------------------------------------------------------------- * Decoded chunk group (columnar_vector.c aggregate path) * diff --git a/src/columnar_projection.c b/src/columnar_projection.c index 9bd7dcb..f00105f 100644 --- a/src/columnar_projection.c +++ b/src/columnar_projection.c @@ -342,14 +342,11 @@ columnar_read_projection(PG_FUNCTION_ARGS) MemoryContext oldContext; FmgrInfo *outFns; int ncols; - int tnatts; int i; Snapshot snap; ColumnarReadState *readState; Datum *rvals; bool *rnulls; - Datum *basevals; - bool *basenulls; uint64 projRowNum; if (PG_ARGISNULL(0) || PG_ARGISNULL(1)) @@ -393,7 +390,6 @@ columnar_read_projection(PG_FUNCTION_ARGS) projname, get_rel_name(relid)))); ncols = proj->columnsLen; - tnatts = RelationGetDescr(rel)->natts; /* projection storage layout: [rownumber int8, projcol1..projcolK] */ projTupdesc = CreateTemplateTupleDesc(ncols + 1); @@ -427,9 +423,6 @@ columnar_read_projection(PG_FUNCTION_ARGS) snap = GetActiveSnapshot(); rvals = palloc(sizeof(Datum) * (ncols + 1)); rnulls = palloc(sizeof(bool) * (ncols + 1)); - basevals = palloc(sizeof(Datum) * tnatts); - basenulls = palloc(sizeof(bool) * tnatts); - readState = ColumnarBeginReadWithStorage(rel, snap, proj->projStorageId, projTupdesc, NULL, NULL, 0, NULL); @@ -440,8 +433,13 @@ columnar_read_projection(PG_FUNCTION_ARGS) Datum result; bool resnull = false; - /* deletes/visibility come from the base */ - if (!ColumnarReadRowByNumber(rel, snap, baseRow, basevals, basenulls)) + /* + * Only the base row's visibility is wanted here -- every value this loop + * emits comes from the projection's own storage. Reconstructing the whole + * base row to answer that decoded every column and threw all of it away + * (issue #157). + */ + if (!ColumnarRowIsLive(rel, snap, baseRow)) continue; initStringInfo(&buf); @@ -505,6 +503,7 @@ columnar_reconstruct_via_projection(PG_FUNCTION_ARGS) bool *rnulls; Datum *basevals; bool *basenulls; + Bitmapset *uncovered = NULL; uint64 projRowNum; if (PG_ARGISNULL(0) || PG_ARGISNULL(1)) @@ -601,6 +600,18 @@ columnar_reconstruct_via_projection(PG_FUNCTION_ARGS) basevals = palloc(sizeof(Datum) * tnatts); basenulls = palloc(sizeof(bool) * tnatts); + /* + * The base row is only read for the columns the projection does not carry, + * so those are the only ones worth decoding (issue #157). + */ + for (i = 0; i < tnatts; i++) + { + if (TupleDescAttr(tableDesc, i)->attisdropped) + continue; + if (covered[i] < 0) + uncovered = bms_add_member(uncovered, i); + } + readState = ColumnarBeginReadWithStorage(rel, snap, proj->projStorageId, projTupdesc, NULL, NULL, 0, NULL); @@ -612,8 +623,9 @@ columnar_reconstruct_via_projection(PG_FUNCTION_ARGS) bool resnull = false; bool first = true; - /* fetch the base row (liveness + any non-covered columns) */ - if (!ColumnarReadRowByNumber(rel, snap, baseRow, basevals, basenulls)) + /* fetch the base row: liveness, and only the columns not covered */ + if (!ColumnarReadRowByNumberCols(rel, snap, baseRow, basevals, + basenulls, uncovered)) continue; initStringInfo(&buf); diff --git a/src/columnar_reader.c b/src/columnar_reader.c index 8e15aea..4cd3886 100644 --- a/src/columnar_reader.c +++ b/src/columnar_reader.c @@ -1633,9 +1633,27 @@ columnar_fetch_group_slot(uint64 storageId, uint64 groupNumber, bool *hit) return victim; } -bool -ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, - Datum *values, bool *nulls) +/* + * columnar_fetch_row + * Shared worker behind the three fetch entry points below. + * + * Which columns to decode is said two ways, and deliberately not one. + * allColumns is an explicit flag; needed is a set of 0-based attribute + * numbers consulted only when it is false. + * + * The obvious single-argument form -- a set where NULL means "all" -- cannot + * be made safe, because a Bitmapset does not distinguish empty from NULL: an + * empty one *is* NULL. A caller that computes its set and finds nothing in it + * would then silently ask for every column, which is the exact opposite, and + * no assertion can catch it because the two cases are the same value. + * A column outside it is not read, not decoded and not indexed, and comes + * back null. wantValues == false stops as soon as liveness is settled, + * without touching the group's bytes at all. + */ +static bool +columnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber, + Datum *values, bool *nulls, bool allColumns, + Bitmapset *needed, bool wantValues) { uint64 storageId = ColumnarStorageId(rel); TupleDesc tupdesc = RelationGetDescr(rel); @@ -1711,6 +1729,18 @@ ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, } } + /* + * Liveness is fully settled here: it depends only on the row group covering + * the row and on the delete vector. A caller that asks nothing else is done, + * without the group's bytes being read or a single column decoded (#157). + */ + if (!wantValues) + { + MemoryContextSwitchTo(oldContext); + MemoryContextDelete(tmp); + return true; + } + /* * The group's bytes and its decoded columns are the expensive part and the * part that repeats across fetches of the same group, so they come from the @@ -1813,6 +1843,19 @@ ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, char *cursor; uint64 present; + /* + * A column the caller did not ask for is neither decoded nor indexed, + * and reads as null rather than being left untouched: a caller that + * projects and then reads outside its projection gets a null instead of + * whatever the array happened to hold. + */ + if (!allColumns && !bms_is_member(c, needed)) + { + values[c] = (Datum) 0; + nulls[c] = true; + continue; + } + if (cc == NULL) { values[c] = getmissingattr(tupdesc, c + 1, &nulls[c]); @@ -1903,6 +1946,55 @@ ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, return true; } +/* + * ColumnarReadRowByNumber + * Reconstruct every column of the row addressed by a row number. False when + * the row is not visible. + */ +bool +ColumnarReadRowByNumber(Relation rel, Snapshot snapshot, uint64 rowNumber, + Datum *values, bool *nulls) +{ + return columnar_fetch_row(rel, snapshot, rowNumber, values, nulls, + true, NULL, true); +} + +/* + * ColumnarReadRowByNumberCols + * Decode exactly the columns in `needed`; every other column reads as null. + * An empty or NULL set therefore decodes nothing, which is what it says + * rather than a silent "everything" -- for every column, call + * ColumnarReadRowByNumber, which takes no set and cannot be misread. + * + * Decoding every column whatever the caller wanted is not merely wasted + * work on a wide table. The decoded bytes are measured against the fetch + * cache's size cap, so the entry is dropped after every fetch and the group + * is decoded again for the next row -- the behaviour the cache exists to + * remove (issue #157). + */ +bool +ColumnarReadRowByNumberCols(Relation rel, Snapshot snapshot, uint64 rowNumber, + Datum *values, bool *nulls, Bitmapset *needed) +{ + return columnar_fetch_row(rel, snapshot, rowNumber, values, nulls, + false, needed, true); +} + +/* + * ColumnarRowIsLive + * Is the row visible? Decodes nothing. + * + * columnar_index_delete_tuples asks exactly this, once per candidate index + * tuple on a path nbtree drives during deletion, and answered it by + * reconstructing every column and freeing the result unread. + */ +bool +ColumnarRowIsLive(Relation rel, Snapshot snapshot, uint64 rowNumber) +{ + return columnar_fetch_row(rel, snapshot, rowNumber, NULL, NULL, + false, NULL, false); +} + void ColumnarRescanRead(ColumnarReadState *readState) { diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index c347b18..0cafb34 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -31,7 +31,8 @@ #include "optimizer/plancat.h" #include "port/atomics.h" #include "storage/bufmgr.h" -#if PG_VERSION_NUM >= 180000 +#if PG_VERSION_NUM >= 170000 +/* the read-stream ANALYZE rework landed in PG17, and so did this header */ #include "storage/read_stream.h" #endif #include "storage/lmgr.h" @@ -581,7 +582,14 @@ columnar_analyze_set_slice(ColumnarAnalyzeState *st, BlockNumber blockno) } } -#if PG_VERSION_NUM >= 180000 +/* + * The block comes from a read stream from PG17 and as a plain BlockNumber + * before that. columnar_compat.h supplies the parameter list and splits at the + * same major; these two must agree, and when they did not, PG17 took the + * pre-17 branch and failed to compile on a `blockno` its signature does not + * have. + */ +#if PG_VERSION_NUM >= 170000 static bool columnar_scan_analyze_next_block(COLUMNAR_ANALYZE_NEXT_BLOCK_ARGS) { @@ -848,23 +856,23 @@ columnar_index_delete_tuples(Relation rel, TM_IndexDeleteOp *delstate) { Snapshot snapshot = ActiveSnapshotSet() ? GetActiveSnapshot() : GetTransactionSnapshot(); - TupleDesc tupdesc = RelationGetDescr(rel); - Datum *values = (Datum *) palloc(sizeof(Datum) * tupdesc->natts); - bool *nulls = (bool *) palloc(sizeof(bool) * tupdesc->natts); int i; for (i = 0; i < delstate->ndeltids; i++) { uint64 rowNumber = ColumnarItemPointerToRowNumber(&delstate->deltids[i].tid); - bool live = ColumnarReadRowByNumber(rel, snapshot, rowNumber, - values, nulls); - delstate->status[delstate->deltids[i].id].knowndeletable = !live; + /* + * Only liveness matters here, and ColumnarRowIsLive decodes nothing to + * answer it. This used to reconstruct every column of the row and then + * free the result unread, once per candidate index tuple, on a path + * nbtree drives during deletion (issue #157). + */ + delstate->status[delstate->deltids[i].id].knowndeletable = + !ColumnarRowIsLive(rel, snapshot, rowNumber); } - pfree(values); - pfree(nulls); return InvalidTransactionId; } #endif diff --git a/test/native_fetch_projection.sh b/test/native_fetch_projection.sh new file mode 100755 index 0000000..6f4bbf4 --- /dev/null +++ b/test/native_fetch_projection.sh @@ -0,0 +1,159 @@ +#!/usr/bin/env bash +# +# pgColumnar fetch-by-row-number with a column projection (issue #157). +# +# Fetching a row by number decoded every column of its row group whatever the +# caller wanted. Two entry points now exist beside it: one that decodes only a +# given set of columns, and one that answers visibility and decodes nothing. +# +# Neither has a SQL surface of its own, so this exercises them through the two +# callers that use them: pgcolumnar.read_projection, which needs the base row +# only for visibility, and pgcolumnar.reconstruct_via_projection, which needs +# exactly the columns the projection does not carry. +# +# Four things are asserted. +# +# 1. The output is unchanged. This is the whole risk of the change: a projection +# that decodes too few columns returns a null where a value belongs, and +# nothing raises. Checked against the same query with the projection dropped, +# which reads the base table directly, so the oracle does not share the code +# under test. +# +# 2. Visibility still comes from the base row. The liveness entry point stops +# before decoding anything, so a delete that it failed to see would show up as +# a row that should have disappeared and did not. +# +# 3. The empty-set case is right. When a projection covers every column the +# computed set comes out empty, and a Bitmapset cannot tell empty from NULL. +# The first version of this API read NULL as "every column", so that case +# asked for the opposite of what it meant -- invisibly, because decoding +# everything still returns the right answer. The set now says what it means, +# which changes behaviour on that path, so the path is checked. +# +# 4. The call sites are the new ones. A wide fixture throughout, because on a +# narrow table decoding one column against forty is not observable. +# +# Usage: test/native_fetch_projection.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_FETCHPROJ_ROWS:-20000} +NCOLS=${PGC_FETCHPROJ_COLS:-40} + +cols=""; sel="" +for i in $(seq 1 "$NCOLS"); do + cols="$cols, c$i bigint" + sel="$sel, g * $i" +done + +psql_run "DROP TABLE IF EXISTS fp_w; + CREATE TABLE fp_w (id int$cols) USING pgcolumnar; + INSERT INTO fp_w SELECT g$sel FROM generate_series(1, $ROWS) g;" >/dev/null + +check "the wide fixture has $((NCOLS + 1)) columns" \ + "$(q "SELECT count(*) FROM information_schema.columns + WHERE table_name = 'fp_w';")" "$((NCOLS + 1))" + +psql_run "SELECT pgcolumnar.add_projection('fp_w', 'fp_p', + ARRAY['id','c1'], ARRAY['id']);" >/dev/null + +# --- 1. reconstruct returns the same rows as reading the table ----------------- + +# reconstruct_via_projection reads the covered columns from the projection and +# the rest from the base row, which is the projected-fetch path. The oracle is +# the base table itself, which does not go through that path at all. +mismatch="$(q "WITH viaproj AS ( + SELECT pgcolumnar.reconstruct_via_projection('fp_w','fp_p') AS r + ), direct AS ( + SELECT id::text || '|' || c1::text || '|' || c2::text || '|' || + c${NCOLS}::text AS d + FROM fp_w + ) + SELECT count(*) FROM viaproj + WHERE split_part(r, '|', 1) || '|' || split_part(r, '|', 2) || '|' || + split_part(r, '|', 3) || '|' || split_part(r, '|', $((NCOLS + 1))) + NOT IN (SELECT d FROM direct);")" + +check "every reconstructed row matches the base table" "$mismatch" "0" + +# --- 2. visibility still comes from the base row ------------------------------ + +before="$(q "SELECT count(*) FROM pgcolumnar.read_projection('fp_w','fp_p');")" +check "the projection reads every row to start with" "$before" "$ROWS" + +psql_run "DELETE FROM fp_w WHERE id <= 100;" >/dev/null + +after="$(q "SELECT count(*) FROM pgcolumnar.read_projection('fp_w','fp_p');")" +check "a deleted row stops being read through the projection" \ + "$after" "$((ROWS - 100))" + +# the same must hold for the reconstruct path, which uses the projected fetch +rafter="$(q "SELECT count(*) FROM pgcolumnar.reconstruct_via_projection('fp_w','fp_p');")" +check "and stops being reconstructed too" "$rafter" "$((ROWS - 100))" + +# a value from an uncovered column is still right after the delete, so the +# projection set was not narrowed by one column too many +check "an uncovered column still reads correctly after a delete" \ + "$(q "SELECT split_part(r, '|', $((NCOLS + 1))) + FROM pgcolumnar.reconstruct_via_projection('fp_w','fp_p') AS r + WHERE split_part(r, '|', 1) = '101';")" \ + "$(q "SELECT c${NCOLS}::text FROM fp_w WHERE id = 101;")" + +# --- 3. a projection that covers every column --------------------------------- + +# The set of columns to read from the base row is computed, and here it comes out +# EMPTY, because the projection carries all of them. That is the case the old +# "NULL means every column" convention got backwards: an empty computed set is +# indistinguishable from NULL, so it asked for every column instead of none. It +# still returned the right answer, which is why it was invisible -- it just did +# the whole decode this change exists to avoid. +# +# The fix makes the set say what it means, so this path now decodes nothing. That +# is a real behaviour change on a live path, so it is checked rather than assumed: +# every value here has to come from the projection and still be right. +psql_run "DROP TABLE IF EXISTS fp_all; + CREATE TABLE fp_all (id int$cols) USING pgcolumnar; + INSERT INTO fp_all SELECT g$sel FROM generate_series(1, 2000) g;" >/dev/null +psql_run "SELECT pgcolumnar.add_projection('fp_all', 'fp_ap', + ARRAY['id', 'c1', 'c2', 'c3', 'c4', 'c5', 'c6', 'c7', 'c8', 'c9', 'c10', 'c11', 'c12', 'c13', 'c14', 'c15', 'c16', 'c17', 'c18', 'c19', 'c20', 'c21', 'c22', 'c23', 'c24', 'c25', 'c26', 'c27', 'c28', 'c29', 'c30', 'c31', 'c32', 'c33', 'c34', 'c35', 'c36', 'c37', 'c38', 'c39', 'c40'], ARRAY['id']);" >/dev/null + +check "the all-covering projection reconstructs every row" \ + "$(q "SELECT count(*) FROM pgcolumnar.reconstruct_via_projection('fp_all','fp_ap');")" \ + "2000" + +check "and its values are right with nothing decoded from the base" \ + "$(q "SELECT split_part(r, '|', 2) || '/' || split_part(r, '|', $((NCOLS + 1))) + FROM pgcolumnar.reconstruct_via_projection('fp_all','fp_ap') AS r + WHERE split_part(r, '|', 1) = '77';")" \ + "$(q "SELECT c1::text || '/' || c${NCOLS}::text FROM fp_all WHERE id = 77;")" + +# --- 4. the entry points are the ones being used ------------------------------ + +# The timing difference is real but modest, because the decoded-group cache +# already amortises the decode across a group, so it is not asserted here. These +# pin the call sites instead: a revert to the full-decode entry point would pass +# every check above while giving back what the change was for. +SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/src" + +check "the visibility-only caller decodes nothing" \ + "$(grep -c 'ColumnarRowIsLive(rel, snap, baseRow)' "$SRC/columnar_projection.c")" "1" + +check "the reconstruct caller asks only for uncovered columns" \ + "$(grep -c 'ColumnarReadRowByNumberCols(rel, snap, baseRow' "$SRC/columnar_projection.c")" "1" + +check "index deletion asks only whether the row is live" \ + "$(grep -c 'ColumnarRowIsLive(rel, snapshot, rowNumber)' "$SRC/columnar_tableam.c")" "1" + +# and the convention that made an empty set mean its opposite stays gone: the +# worker takes an explicit flag, so "every column" cannot be spelled as a set +check "asking for every column is a flag, not an absent set" \ + "$(grep -c 'bool allColumns' "$SRC/columnar_reader.c")" "1" + +check "the column test consults that flag rather than a null set" \ + "$(grep -c '!allColumns && !bms_is_member(c, needed)' "$SRC/columnar_reader.c")" "1" + +pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index ac74e5b..f339ecf 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -27,7 +27,7 @@ SRCDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" SUITES=(harness_selftest smoke phase2 phase3 phase4 phase5 phase6 audit concurrency unique_conc \ differential recovery fuzz hardening concurrent_diff parallel sorted_projection \ arrow_export parquet_export read_stream corruption \ - generated_columns temporal arrow_import index_only projections arrow_nested parquet_import parquet_nested arrow_nested_import parquet_nested_import native_writer native_roundtrip native_encoding native_zonemap write_minmax_fastpath native_skip native_agg native_agg_deletes native_bloom native_vecskip native_index native_fetch_position native_dml native_ios native_projection native_cluster native_compact native_recluster native_reclaim native_ownership native_reclaim_cycles native_reclaim_frag native_reclaim_reconcile native_gap native_truncate native_rewrite native_rewrite_conc native_parquet_schema native_read_parquet native_parquet_fdw native_parquet_pushdown native_parquet_hardening native_parquet_units native_parquet_flba native_parquet_codecs native_parquet_projection native_parquet_multifile native_parquet_streaming native_parquet_partition native_cancel wal_envelope decode_interrupts native_fetch_cache analyze_stats isolation) + generated_columns temporal arrow_import index_only projections arrow_nested parquet_import parquet_nested arrow_nested_import parquet_nested_import native_writer native_roundtrip native_encoding native_zonemap write_minmax_fastpath native_skip native_agg native_agg_deletes native_bloom native_vecskip native_index native_fetch_position native_dml native_ios native_projection native_cluster native_compact native_recluster native_reclaim native_ownership native_reclaim_cycles native_reclaim_frag native_reclaim_reconcile native_gap native_truncate native_rewrite native_rewrite_conc native_parquet_schema native_read_parquet native_parquet_fdw native_parquet_pushdown native_parquet_hardening native_parquet_units native_parquet_flba native_parquet_codecs native_parquet_projection native_parquet_multifile native_parquet_streaming native_parquet_partition native_cancel wal_envelope decode_interrupts native_fetch_cache analyze_stats native_fetch_projection isolation) # Default matrix: one assert-enabled pg_config per major, 15 through 19. DEFAULT_CONFIGS=(