diff --git a/src/columnar.h b/src/columnar.h index 6741e6c..ad62bbd 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -451,6 +451,10 @@ extern void PgColumnarCheckFreeSpaceNoOverlap(uint64 storageId); * ------------------------------------------------------------------------- */ extern uint64 PgColumnarNextStorageId(void); extern void PgColumnarInsertNativeStorageRow(const NativeStorageMetadata *s); + +/* projection: needed attnos (pull_varattnos form) -> the reader's 0-based set */ +extern Bitmapset *PgColumnarProjectionFromAttnos(Bitmapset *needed, int natts, + int *nProjected); extern void PgColumnarSetSortedExtent(uint64 storageId, int64 firstGroup, int64 lastGroup); extern void PgColumnarCheckNativeFormatVersion(uint64 storageId, const char *relName); diff --git a/src/columnar_customscan.c b/src/columnar_customscan.c index 6572757..5c57ef1 100644 --- a/src/columnar_customscan.c +++ b/src/columnar_customscan.c @@ -169,17 +169,23 @@ static const CustomExecMethods pgcolumnar_exec_methods = { * carry a ctid system Var). This is the projection pushed into the reader * (spec 9). */ -static Bitmapset * -pgcolumnar_projected_columns(CustomScan *cscan, int natts, int *nProjected) +/* + * PgColumnarProjectionFromAttnos + * Turn a set of needed attribute numbers, in pull_varattnos' offset form, + * into the reader's 0-based projection. Returns NULL for "read every + * column", which is what a system column, a whole-row Var, or an empty set + * all mean. + * + * Shared because index_build_range_scan needs the same computation over a + * different source (#413): its columns come from IndexInfo rather than from + * a plan. The escapes are the interesting part and are worth having once. + */ +Bitmapset * +PgColumnarProjectionFromAttnos(Bitmapset *needed, int natts, int *nProjected) { - Bitmapset *needed = NULL; Bitmapset *projected = NULL; - Index scanrelid = cscan->scan.scanrelid; int attno; - pull_varattnos((Node *) cscan->scan.plan.targetlist, scanrelid, &needed); - pull_varattnos((Node *) cscan->scan.plan.qual, scanrelid, &needed); - /* a system column or whole-row Var forces reading every column */ for (attno = FirstLowInvalidHeapAttributeNumber + 1; attno <= 0; attno++) { @@ -210,6 +216,19 @@ pgcolumnar_projected_columns(CustomScan *cscan, int natts, int *nProjected) return projected; } +static Bitmapset * +pgcolumnar_projected_columns(CustomScan *cscan, int natts, int *nProjected) +{ + Bitmapset *needed = NULL; + Index scanrelid = cscan->scan.scanrelid; + + pull_varattnos((Node *) cscan->scan.plan.targetlist, scanrelid, &needed); + pull_varattnos((Node *) cscan->scan.plan.qual, scanrelid, &needed); + + return PgColumnarProjectionFromAttnos(needed, natts, nProjected); +} + + /* * pgcolumnar_commute_strategy * The btree comparison strategy for "value op column" given the strategy diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index b497d69..72fbde7 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -29,6 +29,7 @@ #include "executor/tuptable.h" #include "miscadmin.h" #include "nodes/pathnodes.h" +#include "optimizer/optimizer.h" #include "optimizer/pathnode.h" #include "optimizer/plancat.h" #include "port/atomics.h" @@ -1490,7 +1491,60 @@ pgcolumnar_index_build_range_scan(Relation table_rel, Relation index_rel, else snapshot = GetTransactionSnapshot(); - readState = PgColumnarBeginRead(table_rel, snapshot, NULL, NULL, 0, NULL); + /* + * Project. The columns this build needs are all in our own arguments and + * we were throwing them away, so a one-column index on a wide table + * decoded every column (#413). + * + * Three sources, and missing any of them reads unset slot values: + * ii_IndexAttrNumbers the key columns + * ii_Expressions an expression index references more + * ii_Predicate a partial index evaluates against more + * + * PgColumnarProjectionFromAttnos returns NULL for "every column", which + * covers a whole-row or system-column reference, and is what the custom + * scan already does with the same escapes. + */ + { + Bitmapset *needed = NULL; + Bitmapset *projected; + int nProjected = 0; + int i; + + for (i = 0; i < index_info->ii_NumIndexAttrs; i++) + { + AttrNumber attno = index_info->ii_IndexAttrNumbers[i]; + + /* 0 marks an expression column; Vars come from ii_Expressions */ + if (attno != 0) + needed = bms_add_member(needed, + attno - FirstLowInvalidHeapAttributeNumber); + } + pull_varattnos((Node *) index_info->ii_Expressions, 1, &needed); + pull_varattnos((Node *) index_info->ii_Predicate, 1, &needed); + + projected = + PgColumnarProjectionFromAttnos(needed, + RelationGetDescr(table_rel)->natts, + &nProjected); + + /* + * Say what was projected, so a test can assert the projection + * NARROWED rather than infer it from a stopwatch. A fix here that + * silently did nothing would pass every correctness check and a + * wall-clock check on a quiet machine, which is the failure mode + * this projection is being added to avoid. + * + * DEBUG1, so it costs nothing at the default log level. + */ + elog(DEBUG1, + "columnar: index build on \"%s\" projecting %d of %d columns", + RelationGetRelationName(index_rel), nProjected, + RelationGetDescr(table_rel)->natts); + + readState = PgColumnarBeginRead(table_rel, snapshot, NULL, + projected, 0, NULL); + } ownReadState = true; } diff --git a/test/native_index_projection.sh b/test/native_index_projection.sh new file mode 100755 index 0000000..0ff885c --- /dev/null +++ b/test/native_index_projection.sh @@ -0,0 +1,117 @@ +#!/usr/bin/env bash +# +# CREATE INDEX projects only the columns the index needs (issue #413). +# +# pgcolumnar_index_build_range_scan opened its reader with no projection, so building a +# one-column index on a wide table decoded every column. It never had to: the callback +# receives IndexInfo, which carries ii_IndexAttrNumbers and the expression and predicate +# trees. The information was in its own arguments and was thrown away. +# +# Measured on 300,000 rows of 20 columns, index on the key alone, non-assert PG18: +# +# before columnar 1,403 ms heap 149 ms 9.4x slower than heap +# after columnar 87 ms heap 144 ms 1.65x faster +# +# What is asserted, in the order the risk sits: +# +# 1. the projection NARROWED, read from the build's own DEBUG1 line. A fix that +# silently did nothing passes every correctness check below, and a wall-clock check +# on a quiet machine, so this is the assertion with teeth. +# 2. expression and partial indexes project their EXTRA columns. Getting those wrong +# evaluates against unset slot values, which is a wrong answer and not a slow one. +# 3. a forced index scan and a forced seq scan agree over the FULL ordered result. +# Point lookups can be satisfied by an index that is wrong for keys nobody asked for. +# +# amcheck runs where the build has contrib and skips VISIBLY where it does not. Source +# builds have no contrib, and a silent skip is the defect this file exists to prevent. +# +# Usage: test/native_index_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_IDXPROJ_ROWS:-300000} + +# lib.sh's q() emits psql's output for every statement, so "SET ...; SELECT ..." in one +# call returns "SET" as well as the value. Keep the session but read only the result. +qset() { # $1 = SET clause(s), $2 = query + env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -At -q -c "$1" -c "$2" 2>/dev/null | tail -1 +} + + +# 20 columns: an int key, an int secondary, 18 wide text. +cols="k int, k2 int" +vals="g, g*2" +for i in $(seq 1 18); do cols="$cols, c$i text"; vals="$vals, repeat('x',80)||g"; done +psql_run "CREATE TABLE w ($cols) USING pgcolumnar;" +psql_run "INSERT INTO w SELECT $vals FROM generate_series(1,$ROWS) g;" +check "fixture rows" "$(q 'SELECT count(*) FROM w')" "$ROWS" +check "fixture is wide" \ + "$(q "SELECT count(*) FROM pg_attribute WHERE attrelid='w'::regclass AND attnum>0")" "20" + +# The projection each build chose, from its own DEBUG1 line. This is the check a +# do-nothing fix would fail and the correctness checks below would not. +proj() { # $1 index name, $2 CREATE INDEX statement -> "n of m" + env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" \ + -c "SET client_min_messages=debug1;" -c "$2" 2>&1 \ + | grep -oE "index build on \"$1\" projecting [0-9]+ of [0-9]+" \ + | grep -oE '[0-9]+ of [0-9]+' | head -1 +} + +check "plain index projects one column of twenty" \ + "$(proj w_k 'CREATE INDEX w_k ON w (k)')" "1 of 20" +check "two-column index projects two" \ + "$(proj w_k12 'CREATE INDEX w_k12 ON w (k, k2)')" "2 of 20" +check "an index on a late column projects one, not everything before it" \ + "$(proj w_c18 'CREATE INDEX w_c18 ON w (c18)')" "1 of 20" +check "an expression index projects its expression's columns" \ + "$(proj w_expr 'CREATE INDEX w_expr ON w ((k + k2))')" "2 of 20" +check "a partial index projects the predicate's columns too" \ + "$(proj w_part 'CREATE INDEX w_part ON w (k) WHERE k2 > 100')" "2 of 20" +check "an expression over a text column projects that column" \ + "$(proj w_len 'CREATE INDEX w_len ON w ((length(c1)))')" "1 of 20" + +# Correctness. An index built from under-projected data indexes unset slot values, so +# each of these must find what a sequential scan finds. +check "plain index finds the row" \ + "$(qset 'SET enable_seqscan=off' 'SELECT k2 FROM w WHERE k = 12345')" "24690" +check "expression index finds the row" \ + "$(qset 'SET enable_seqscan=off' 'SELECT count(*) FROM w WHERE (k + k2) = 30000')" "1" +check "partial index finds the row inside its predicate" \ + "$(qset 'SET enable_seqscan=off' 'SELECT count(*) FROM w WHERE k = 50000 AND k2 > 100')" "1" + +# The oracle, depending on nothing but PostgreSQL. Compare the FULL ordered result of a +# forced index scan against a forced seq scan. +agree() { # $1 label, $2 predicate, $3 selected expression + local viaix viaseq + viaix=$(qset "SET enable_seqscan=off; SET enable_bitmapscan=off" \ + "SELECT md5(string_agg(t::text, ',' ORDER BY t)) + FROM (SELECT $3 AS t FROM w WHERE $2) s") + viaseq=$(qset "SET enable_indexscan=off; SET enable_bitmapscan=off; SET enable_indexonlyscan=off" \ + "SELECT md5(string_agg(t::text, ',' ORDER BY t)) + FROM (SELECT $3 AS t FROM w WHERE $2) s") + check "$1" "$viaix" "$viaseq" +} +agree "plain index agrees with a sequential scan" "k BETWEEN 1000 AND 9999" "k" +agree "late-column index agrees" "c18 > repeat('x',80)||'99000'" "k" +agree "expression index agrees" "(k + k2) BETWEEN 300 AND 30000" "k" +agree "partial index agrees on its subset" "k2 > 100 AND k < 5000" "k" +agree "text expression index agrees" "length(c1) = 83" "k" + +# amcheck where available. The skip must be visible: a check that reports nothing is +# indistinguishable from a check that passes, which is what this file is about. +if psql_run "CREATE EXTENSION IF NOT EXISTS amcheck;" >/dev/null 2>&1 && + [ "$(q "SELECT count(*) FROM pg_proc WHERE proname='bt_index_check'")" != "0" ]; then + for ix in w_k w_k12 w_c18 w_expr w_part w_len; do + out=$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -c "SELECT bt_index_check('$ix'::regclass)" 2>&1) + check "bt_index_check($ix)" \ + "$(grep -qE 'ERROR' <<<"$out" && echo bad || echo ok)" "ok" + done +else + echo "SKIP amcheck is not installed on this build; the seq-scan oracle above still ran" +fi + +pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 2290c8b..10cdc0f 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -208,7 +208,7 @@ SRCDIR="${PGC_RUN_SRCDIR:-$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)}" SUITES=(harness_selftest docs_style smoke phase2 phase3 phase4 phase5 phase6 audit concurrency unique_conc \ differential recovery replication native_backend_crash fuzz fuzz_parquet fuzz_arrow 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_fastdecode native_zonemap write_minmax_fastpath write_fsst_compressed fsst_margin encode_invariants encode_effort native_skip pushdown_report native_agg native_agg_deletes native_agg_addcolumn native_groupagg ungrouped_vector_agg parallel_vector_agg native_bloom bloom_setting bloom_lazy native_vecskip native_index native_fetch_position native_dml alter_column_type native_ios native_projection native_cluster native_repack native_compact native_recluster recluster_extent native_vacuum_race native_sort_by sort_status native_reclaim native_ownership drop_cleanup pg_dump_roundtrip native_reclaim_cycles native_reclaim_frag native_reclaim_reconcile native_gap native_format native_truncate native_rewrite native_rewrite_conc rewrite_group_scan native_parquet_schema native_read_parquet native_parquet_fdw native_parquet_pushdown native_parquet_hardening server_file_privilege native_parquet_stack native_parquet_units native_parquet_flba native_parquet_codecs native_parquet_projection native_parquet_multifile native_parquet_streaming native_parquet_partition native_cancel cancel_decode wal_envelope decode_interrupts import_exclusion import_deferred parallel_copy parallel_export_parquet fk_referencing row_triggers native_lazy_slot native_ctas native_fetch_cache native_fetch_interrupt analyze_stats analyze_reltuples native_fetch_projection column_projection 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_fastdecode native_zonemap write_minmax_fastpath write_fsst_compressed fsst_margin encode_invariants encode_effort native_skip pushdown_report native_agg native_agg_deletes native_agg_addcolumn native_groupagg ungrouped_vector_agg parallel_vector_agg native_bloom bloom_setting bloom_lazy native_vecskip native_index native_index_projection native_fetch_position native_dml alter_column_type native_ios native_projection native_cluster native_repack native_compact native_recluster recluster_extent native_vacuum_race native_sort_by sort_status native_reclaim native_ownership drop_cleanup pg_dump_roundtrip native_reclaim_cycles native_reclaim_frag native_reclaim_reconcile native_gap native_format native_truncate native_rewrite native_rewrite_conc rewrite_group_scan native_parquet_schema native_read_parquet native_parquet_fdw native_parquet_pushdown native_parquet_hardening server_file_privilege native_parquet_stack native_parquet_units native_parquet_flba native_parquet_codecs native_parquet_projection native_parquet_multifile native_parquet_streaming native_parquet_partition native_cancel cancel_decode wal_envelope decode_interrupts import_exclusion import_deferred parallel_copy parallel_export_parquet fk_referencing row_triggers native_lazy_slot native_ctas native_fetch_cache native_fetch_interrupt analyze_stats analyze_reltuples native_fetch_projection column_projection isolation) # Default matrix: one assert-enabled pg_config per major, 15 through 19. DEFAULT_CONFIGS=(