Decode a rewrite scan against the descriptor it is handed (supersedes #183)

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

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

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=(