From 6c28047111d767e5fa8b8f60b1a673ad07723593 Mon Sep 17 00:00:00 2001 From: ChronicallyJD Date: Mon, 27 Jul 2026 16:13:35 -0600 Subject: [PATCH] Decode a rewrite scan against the descriptor it is handed ALTER TABLE ... ALTER COLUMN TYPE could not change any fixed-width column. Every conversion that moved a column across a width boundary raised "corrupt encoded chunk", and text to a fixed-width type segfaulted the backend. 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. columnar_scan_begin built its read state from the relation instead, so 4-byte values were read as 8-byte ones, a fixed-width chunk was read as varlena, and in the text-to-int direction an integer read out of the value stream was dereferenced as a text pointer. Build the read state on the first getnextslot, from slot->tts_tupleDescriptor, which is the shape the caller is asking for. For every other scan that descriptor is the relation's, so nothing else changes. ColumnarBeginReadWithStorage already took an explicit tuple descriptor, so no signature moved. It is allocated in the context the scan descriptor itself was allocated in, not the current one: getnextslot usually runs in a per-tuple context that is reset before the scan ends, and the first version of this crashed in ColumnarEndRead with the read state already freed -- visible only because an assert build fills freed memory with 0x7f. New suite test/alter_column_type.sh, 21 checks, registered. heap is the oracle for the values, and the resulting column type is asserted separately because every value comparison here comes out equal whether or not the conversion happened. 13 of 21 fail against main; the ones that pass are the varlena-to-varlena conversions, which never went through a width boundary. The crash case is ordered last on purpose, because it takes the cluster down and everything after it in the file then fails for reasons of its own. Closes #178. --- src/columnar_tableam.c | 81 +++++++++++++++++-- test/alter_column_type.sh | 164 ++++++++++++++++++++++++++++++++++++++ test/run_all_versions.sh | 2 +- 3 files changed, 240 insertions(+), 7 deletions(-) create mode 100755 test/alter_column_type.sh diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index df61abb..e88ec33 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); @@ -1325,7 +1388,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 7907b87..694bd3d 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 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 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=(