diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index c84b9cf..c5c6bd7 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -142,6 +142,14 @@ typedef struct ColumnarScanDescData { TableScanDescData rs_base; ColumnarReadState *readState; + + /* + * The context the scan descriptor itself was allocated in. The read state + * is built on the first getnextslot, where the current context is usually a + * per-tuple one that is reset before the scan ends, so it is allocated here + * instead and outlives the row that triggered it. + */ + MemoryContext scanContext; ColumnarAnalyzeState *analyzeState; } ColumnarScanDescData; typedef struct ColumnarScanDescData *ColumnarScanDesc; @@ -441,22 +449,75 @@ columnar_scan_begin(Relation rel, Snapshot snapshot, int nkeys, scan->rs_base.rs_parallel = pscan; /* + * The read state is built on the first getnextslot, not here, because the + * shape to decode against arrives with the slot and is not always the + * relation's current one. + * + * ALTER TABLE ... ALTER COLUMN TYPE is where they differ (#178). Phase 2 of + * ATRewriteTable has already updated pg_attribute when phase 3 scans the old + * relation, so RelationGetDescr(rel) describes the new types while the bytes + * on disk are still the old ones. Core hands the scan a slot built from + * tab->oldDesc for exactly that reason; decoding against the relation + * instead read 4-byte values as 8-byte ones and worse. + * + * For every other scan the slot's descriptor is the relation's, so this + * costs a branch and changes nothing. + * * Phase 2 projects all columns for a plain sequential scan (there is no * per-scan projection channel in the table AM without the custom scan of * a later phase), so we pass a NULL projection set. Any ScanKeys the * executor supplies are forwarded for chunk-group skipping. */ - scan->readState = ColumnarBeginRead(rel, snapshot, pscan, NULL, nkeys, key); + scan->rs_base.rs_key = key; + scan->readState = NULL; + scan->scanContext = CurrentMemoryContext; return (TableScanDesc) scan; } +/* + * columnar_scan_read_state + * The scan's reader, built on first use against the descriptor the caller + * is asking for. + * + * Deferring it is what lets a rewrite decode against tab->oldDesc (#178). It + * also means no caller may assume scan->readState is already set: the parallel + * index build reads it straight off the scan without ever going through + * getnextslot, and dereferenced NULL the first time this was written that way. + * Everything that wants the reader comes through here. + * + * The read state is allocated in the context the scan descriptor itself lives + * in. The current context on first use is usually a per-tuple one that is reset + * before the scan ends, and ColumnarEndRead then frees an already-freed pointer. + */ +static ColumnarReadState * +columnar_scan_read_state(ColumnarScanDesc scan, TupleDesc tupdesc) +{ + if (scan->readState == NULL) + { + MemoryContext oldContext = MemoryContextSwitchTo(scan->scanContext); + + scan->readState = + ColumnarBeginReadWithStorage(scan->rs_base.rs_rd, + scan->rs_base.rs_snapshot, + ColumnarStorageId(scan->rs_base.rs_rd), + tupdesc, + scan->rs_base.rs_parallel, NULL, + scan->rs_base.rs_nkeys, + scan->rs_base.rs_key); + MemoryContextSwitchTo(oldContext); + } + + return scan->readState; +} + static void columnar_scan_end(TableScanDesc sscan) { ColumnarScanDesc scan = (ColumnarScanDesc) sscan; - ColumnarEndRead(scan->readState); + if (scan->readState != NULL) + ColumnarEndRead(scan->readState); if (scan->analyzeState != NULL) { @@ -479,7 +540,8 @@ columnar_scan_rescan(TableScanDesc sscan, ScanKey key, bool set_params, { ColumnarScanDesc scan = (ColumnarScanDesc) sscan; - ColumnarRescanRead(scan->readState); + if (scan->readState != NULL) + ColumnarRescanRead(scan->readState); } static bool @@ -491,8 +553,9 @@ columnar_scan_getnextslot(TableScanDesc sscan, ScanDirection direction, ExecClearTuple(slot); - if (!ColumnarReadNextRow(scan->readState, slot->tts_values, - slot->tts_isnull, &rowNumber)) + if (!ColumnarReadNextRow(columnar_scan_read_state(scan, + slot->tts_tupleDescriptor), + slot->tts_values, slot->tts_isnull, &rowNumber)) return false; ExecStoreVirtualTuple(slot); @@ -1348,7 +1411,13 @@ columnar_index_build_range_scan(Relation table_rel, Relation index_rel, */ if (scan != NULL) { - readState = ((ColumnarScanDesc) scan)->readState; + /* + * An index build always wants the relation's current shape: it is + * indexing what the table is now, not what an in-flight rewrite is + * converting away from. + */ + readState = columnar_scan_read_state((ColumnarScanDesc) scan, + RelationGetDescr(table_rel)); ownReadState = false; } else diff --git a/test/alter_column_type.sh b/test/alter_column_type.sh new file mode 100755 index 0000000..ac95fe0 --- /dev/null +++ b/test/alter_column_type.sh @@ -0,0 +1,164 @@ +#!/usr/bin/env bash +# +# pgColumnar ALTER TABLE ... ALTER COLUMN TYPE (issue #178). +# +# Phase 2 of ATRewriteTable updates pg_attribute before phase 3 scans the old +# relation, so RelationGetDescr() describes the new types while the bytes on disk +# are still the old ones. Core builds the scan's slot from tab->oldDesc for +# exactly that reason. The scan decoded against the relation instead, so every +# conversion that moved a column across a width boundary read the stored bytes as +# the wrong shape: +# +# int -> bigint ERROR: corrupt encoded chunk (raw length does not match value count) +# int -> text ERROR: corrupt encoded chunk (fixed-width encoding on a non-fixed-width column) +# bool -> int ERROR: corrupt encoded chunk (bit width out of range) +# text -> int SIGSEGV -- an integer read out of the stream and then +# dereferenced as a text pointer, in the USING expression +# +# Varlena-to-varlena conversions were unaffected, which is why this looked like a +# type-specific problem rather than a descriptor problem. +# +# heap is the oracle for the values. Comparing against a heap table converted the +# same way is stronger than comparing to constants written here: it is the +# property that has to hold, whatever the data. +# +# The crash case is worth keeping even though it is now just another conversion: +# it is the one where nothing between the misread and the dereference could +# notice, so it is the one that regresses silently. +# +# Usage: test/alter_column_type.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_ALTERTYPE_ROWS:-20000} + +# Convert one column in a columnar table and in a heap table of the same shape, +# then compare every row. The two tables are built and altered through separate +# psql_run calls: sharing one would let a failure on the columnar side stop the +# script before the heap side ran, and the comparison would then find nothing +# against nothing and pass. +conv() { # label, column type, value expression, alter clause + local c="at_c" h="at_h" + + psql_run "DROP TABLE IF EXISTS $c; + CREATE TABLE $c (id int, v $2) USING pgcolumnar; + INSERT INTO $c SELECT g, $3 FROM generate_series(1, $ROWS) g;" >/dev/null 2>&1 + psql_run "DROP TABLE IF EXISTS $h; + CREATE TABLE $h (id int, v $2); + INSERT INTO $h SELECT g, $3 FROM generate_series(1, $ROWS) g;" >/dev/null 2>&1 + + local err + err="$(psql_run "ALTER TABLE $c ALTER COLUMN v TYPE $4;" 2>&1 || true)" + psql_run "ALTER TABLE $h ALTER COLUMN v TYPE $4;" >/dev/null 2>&1 + + if echo "$err" | grep -qiE "corrupt encoded chunk|server closed|terminated"; then + check "$1" "failed: $(echo "$err" | grep -oiE 'corrupt encoded chunk[^\"]*|server closed' | head -1)" "ok" + return + fi + + # every row must match heap, and the row count must be right: a conversion + # that silently dropped rows would otherwise pass a value comparison over + # whatever survived + local mismatch count + mismatch="$(q "SELECT count(*) FROM $c a JOIN $h b USING (id) + WHERE a.v IS DISTINCT FROM b.v;" | tail -1)" + count="$(q "SELECT count(*) FROM $c;" | tail -1)" + check "$1" "${mismatch:-x}/${count:-x}" "0/$ROWS" +} + +# --- fixed width to fixed width, the width-boundary cases -------------------- + +conv "int to bigint" int "g" "bigint" +conv "bigint to int" bigint "g" "int" +conv "smallint to int" smallint "(g % 1000)::smallint" "int" +conv "int to smallint" int "(g % 1000)" "smallint" +conv "float4 to float8" float4 "(g * 1.5)::float4" "float8" +conv "float8 to float4" float8 "(g % 512)::float8" "float4" +conv "date to timestamp" date "date '2024-01-01' + g" "timestamp" +conv "int to numeric" int "g" "numeric" + +# --- across the fixed/varlena boundary, both directions ---------------------- + +conv "int to text" int "g" "text" +conv "bool to int" bool "(g % 2 = 0)" "int USING v::int" + +conv "numeric to text" numeric "g" "text" + +# --- varlena to varlena, which always worked and must keep working ----------- + +conv "text to varchar(64)" text "'s' || g" "varchar(64)" +conv "text to bytea" text "'s' || g" "bytea USING v::bytea" +conv "text rewritten by USING" text "'s' || g" "text USING upper(v)" + +# --- the shape must survive too --------------------------------------------- + +# A conversion is a rewrite, so the row groups are rebuilt. Check the table is +# still readable through an index and still answers an aggregate, not just that +# a sequential comparison matched. +# The ALTER is its own call. psql runs a multi-statement -c in one implicit +# transaction, so putting it with the CREATE means a failed ALTER rolls the table +# away too, and the checks below then fail because nothing exists rather than +# because the rewrite was wrong. +psql_run "DROP TABLE IF EXISTS at_ix; + CREATE TABLE at_ix (id int, v int) USING pgcolumnar; + INSERT INTO at_ix SELECT g, g * 2 FROM generate_series(1, $ROWS) g; + CREATE INDEX at_ix_i ON at_ix (id);" >/dev/null 2>&1 +psql_run "ALTER TABLE at_ix ALTER COLUMN v TYPE bigint;" >/dev/null 2>&1 + +# Assert the conversion actually happened. Every value check below compares +# numbers that are equal whether or not the column changed type, so without this +# they are all satisfied by an ALTER that failed and rolled back. +check "the column really is bigint now" \ + "$(q "SELECT atttypid::regtype::text FROM pg_attribute + WHERE attrelid = 'at_ix'::regclass AND attname = 'v';" | tail -1)" \ + "bigint" + +check "an index scan still finds a row after the rewrite" \ + "$(q "SET enable_seqscan = off; SET pgcolumnar.enable_custom_scan = off; + SELECT v FROM at_ix WHERE id = $((ROWS / 2));" | tail -1)" \ + "$((ROWS))" + +check "the aggregate over the converted column is right" \ + "$(q "SELECT sum(v) FROM at_ix;" | tail -1)" \ + "$(q "SELECT sum(g * 2)::bigint FROM generate_series(1, $ROWS) g;" | tail -1)" + +check "every row survived the rewrite" \ + "$(q "SELECT count(*) FROM at_ix;" | tail -1)" "$ROWS" + +# A column with nulls: the validity bitmap is rebuilt by the rewrite, and an +# off-by-one there moves values onto the wrong rows rather than losing them. +psql_run "DROP TABLE IF EXISTS at_n; DROP TABLE IF EXISTS at_nh; + CREATE TABLE at_n (id int, v int) USING pgcolumnar; + INSERT INTO at_n SELECT g, CASE WHEN g % 3 = 0 THEN NULL ELSE g END + FROM generate_series(1, $ROWS) g;" >/dev/null 2>&1 +psql_run "CREATE TABLE at_nh (id int, v int); + INSERT INTO at_nh SELECT g, CASE WHEN g % 3 = 0 THEN NULL ELSE g END + FROM generate_series(1, $ROWS) g;" >/dev/null 2>&1 +psql_run "ALTER TABLE at_n ALTER COLUMN v TYPE bigint;" >/dev/null 2>&1 +psql_run "ALTER TABLE at_nh ALTER COLUMN v TYPE bigint;" >/dev/null 2>&1 + +check "the nullable column really is bigint now" \ + "$(q "SELECT atttypid::regtype::text FROM pg_attribute + WHERE attrelid = 'at_n'::regclass AND attname = 'v';" | tail -1)" \ + "bigint" + +check "nulls stay on the same rows through a conversion" \ + "$(q "SELECT count(*) FROM at_n a JOIN at_nh b USING (id) + WHERE a.v IS DISTINCT FROM b.v;" | tail -1)" "0" + +# --- the crash, deliberately last --------------------------------------------- +# +# The stored bytes are varlena, the new type is fixed width, and against the +# unfixed build the USING expression dereferences an integer read out of the +# value stream as a text pointer. It runs last because it takes the whole +# cluster down with it: every check after it in the file reports empty results +# and fails for a reason that has nothing to do with what it tests, which makes +# a run against a broken build unreadable. Ordering it here keeps the controls +# above meaningful. +conv "text to int (the crash)" text "g::text" "int USING v::int" + +pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 86f6100..ebd60a6 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 write_fsst_compressed native_skip native_agg native_agg_deletes native_agg_addcolumn 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 import_exclusion import_deferred row_triggers native_lazy_slot native_fetch_cache analyze_stats native_fetch_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_zonemap write_minmax_fastpath write_fsst_compressed native_skip native_agg native_agg_deletes native_agg_addcolumn native_bloom native_vecskip native_index native_fetch_position native_dml alter_column_type 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 import_exclusion import_deferred row_triggers native_lazy_slot native_fetch_cache analyze_stats native_fetch_projection isolation) # Default matrix: one assert-enabled pg_config per major, 15 through 19. DEFAULT_CONFIGS=(