diff --git a/CHANGELOG.md b/CHANGELOG.md index b252cc6..bb4188a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,14 @@ which was true until that script existed. ### Changed +- The unsupported-rewrite error names `REPACK` on PostgreSQL 19 (#399). `REPACK` + replaces `CLUSTER` and `VACUUM FULL` in 19 and dispatches through the same + copy-for-cluster path, which pgColumnar does not implement, so a 19 user who + typed `REPACK` was told that `CLUSTER / VACUUM FULL` was unsupported: two + commands they had not typed, and on 19 the superseded ones. The message now + names the command and hints at `pgcolumnar.vacuum()`, which does the work. This + covers `REPACK (CONCURRENTLY)` too: given a table with an identity index, where + PostgreSQL will run it, heap succeeds and a columnar table is refused. - `CREATE TABLE ... USING pgcolumnar AS SELECT` no longer fails when the source plan is parallel (#387). The storage-row creation path re-checked for an existing row against `GetLatestSnapshot()`, which raises "cannot update diff --git a/design/PG18_19_OPPORTUNITIES.md b/design/PG18_19_OPPORTUNITIES.md index 4ba3aca..92856da 100644 --- a/design/PG18_19_OPPORTUNITIES.md +++ b/design/PG18_19_OPPORTUNITIES.md @@ -97,6 +97,20 @@ extension)" and "Allow ReadStream to be consumed as raw block numbers". something the AM opts into. Concurrent REPACK on a columnar table is therefore unverified and likely needs additional work; treat it as future work. + **CORRECTED 2026-08-05 (#399). It does not work, and this entry is why inspection + was not enough.** `pgcolumnar_relation_copy_for_cluster` is *registered* but is a + stub that unconditionally raises `COLUMNAR_UNSUPPORTED`, so "pgColumnar already + implements that callback" was true of the symbol and false of the behaviour. + Measured on 19beta2: `REPACK`, `REPACK ... USING INDEX`, `REPACK (VERBOSE)`, + `CLUSTER` and `VACUUM FULL` all raise, while `REPACK` succeeds on a heap table on + the same build. `REPACK CONCURRENTLY` is not the syntax; it is + `REPACK (CONCURRENTLY)`. That form **is** ours: on a fixture with no identity index + both access methods are refused by PostgreSQL before the AM is reached, which is + what an early reading of this mistook for "not columnar-specific". With a primary + key and `wal_level=logical`, heap succeeds and columnar raises our error. The + columnar table is undamaged afterwards. + Pinned by `test/native_repack.sh`. The supported route is `pgcolumnar.vacuum()`. + ## 6. Optimizer statistics injection (PostgreSQL 18) - `pg_restore_relation_stats()`, `pg_restore_attribute_stats()`, diff --git a/docs/limitations.md b/docs/limitations.md index 0d73153..e587400 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -268,8 +268,10 @@ returning no rows. ## Vacuum and compaction -- `VACUUM FULL` and `CLUSTER` are not supported on a columnar table; the - copy-for-cluster path raises an error. Use `pgcolumnar.vacuum` or +- `REPACK`, `VACUUM FULL` and `CLUSTER` are not supported on a columnar table; the + copy-for-cluster path raises an error. `REPACK` arrives in PostgreSQL 19 and + replaces the other two, and it dispatches through the same path, so it is + refused for the same reason. Use `pgcolumnar.vacuum` or `pgcolumnar.vacuum_full` instead. - `pgcolumnar.vacuum` always rewrites the whole relation into full row groups. It accepts a `stripe_count` argument for compatibility with the interface, but it diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 259d30a..b497d69 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -1383,7 +1383,29 @@ pgcolumnar_relation_copy_data(Relation rel, const RelFileLocator *newrlocator) static void pgcolumnar_relation_copy_for_cluster(COLUMNAR_COPY_FOR_CLUSTER_ARGS) { - COLUMNAR_UNSUPPORTED("CLUSTER / VACUUM FULL"); + /* + * Name every command that reaches here, and name them per major (#399). + * + * PostgreSQL 19 adds REPACK, which replaces CLUSTER and VACUUM FULL and + * dispatches through this same callback. A 19 user who types REPACK was + * previously told "CLUSTER / VACUUM FULL is not supported yet", naming two + * commands they did not type and, on 19, the superseded ones. The error + * should describe what the user asked for. + * + * The hint matters more than the message: the operation is available, under + * a different name. pgcolumnar.vacuum() reclaims space and the ordering + * functions cluster, so this is a spelling difference rather than a missing + * capability, and the error is where someone will look for that. + */ + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), +#if PG_VERSION_NUM >= 190000 + errmsg("columnar: REPACK, CLUSTER and VACUUM FULL are not supported yet"), +#else + errmsg("columnar: CLUSTER and VACUUM FULL are not supported yet"), +#endif + errhint("Use pgcolumnar.vacuum() to reclaim space, or " + "pgcolumnar.vacuum_sorted() to rewrite in sorted order."))); } /* diff --git a/test/native_repack.sh b/test/native_repack.sh new file mode 100755 index 0000000..3c2ad27 --- /dev/null +++ b/test/native_repack.sh @@ -0,0 +1,155 @@ +#!/usr/bin/env bash +# +# REPACK on a columnar table (issue #399). PostgreSQL 19 and later only. +# +# REPACK replaces CLUSTER and VACUUM FULL in 19. It is not a new table-AM +# callback: it reuses the CLUSTER machinery, which dispatches a rewrite through +# relation_copy_for_cluster. design/PG18_19_OPPORTUNITIES.md concluded from that +# dispatch that REPACK "should work" on a columnar table, because pgColumnar +# registers that callback. +# +# It does not work. The callback is registered and is a stub that raises, so the +# reasoning was true of the symbol and false of the behaviour. That is the gap a +# suite closes and a grep does not, and it is why this file exists. +# +# What is asserted here is therefore the ACTUAL behaviour, not the hoped-for one: +# every spelling of the command errors, it errors naming the command the user +# typed, the table is unharmed afterwards, and the supported alternative works. +# If someone later implements relation_copy_for_cluster, these checks fail and +# should be rewritten to assert success. That is the intended signal. +# +# The heap control is not decoration. Without it, "REPACK fails here" cannot be +# told apart from "REPACK does not work on this build at all". +# +# REPACK (CONCURRENTLY) needs two fixtures for the same reason, and the first +# version of this suite got it wrong. Without an identity index both access +# methods are refused before the AM is reached, so asserting that failure alone +# passes for a reason that has nothing to do with us, and would keep passing if +# our behaviour changed. With a primary key and wal_level=logical, heap succeeds +# and columnar raises our error, so the concurrent form IS a columnar behaviour. +# +# Usage: test/native_repack.sh [PG_CONFIG] +# Written fresh for pgColumnar. + +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +# REPACK (CONCURRENTLY) needs logical decoding of the relation's changes, so the +# cluster must be able to provide it. Without this the concurrent form fails for a +# reason that has nothing to do with the access method. +PGC_EXTRA_CONF="wal_level=logical +max_locks_per_transaction=1024 +max_wal_senders=10 +max_replication_slots=10" +pgc_setup "${1:-/usr/local/pg19/bin/pg_config}" + +# Version gate, and it must be VISIBLE. A suite that silently passes on 15 to 18 +# is the failure mode this project keeps finding. +srv="$(q 'SHOW server_version_num')" +if [ "${srv:-0}" -lt 190000 ]; then + echo "SKIP REPACK requires PostgreSQL 19 (server_version_num=$srv)" + echo "native_repack.sh: SKIPPED" + exit 0 +fi + +ROWS=${PGC_REPACK_ROWS:-20000} + +psql_run "CREATE TABLE rp (id int, v text) USING pgcolumnar; + SELECT pgcolumnar.set_options('rp', stripe_row_limit => 5000, compression => 'zstd'); + INSERT INTO rp SELECT g, 'x'||g FROM generate_series(1,$ROWS) g; + DELETE FROM rp WHERE id % 3 = 0; + CREATE INDEX rp_id ON rp (id); + CREATE TABLE rp_heap (id int, v text); + INSERT INTO rp_heap SELECT g, 'x'||g FROM generate_series(1,$ROWS) g; + CREATE INDEX rp_heap_id ON rp_heap (id);" + +live=$(q "SELECT count(*) FROM rp") +hash_before=$(q "SELECT md5(string_agg(id::text||':'||v, ',' ORDER BY id)) FROM rp") +opts_before=$(q "SELECT stripe_row_limit || '/' || compression FROM pgcolumnar.options o + JOIN pg_class c ON c.oid = o.regclass WHERE c.relname = 'rp'") + +err_of() { psql_run "$1" 2>&1 | grep -oE 'ERROR:.*' | head -1; } + +# ---- every spelling errors, and the message names the command ---------------- +e="$(err_of 'REPACK rp;')" +check "REPACK on a columnar table errors (#399)" \ + "$(case "$e" in *ERROR*) echo yes ;; *) echo "no (succeeded)" ;; esac)" "yes" +check "and the error names REPACK, which is what the user typed (#399)" \ + "$(case "$e" in *REPACK*) echo yes ;; *) echo "no ($e)" ;; esac)" "yes" + +check "REPACK ... USING INDEX errors the same way" \ + "$(case "$(err_of 'REPACK rp USING INDEX rp_id;')" in *"not supported"*) echo yes ;; *) echo no ;; esac)" "yes" +check "REPACK (VERBOSE) errors the same way" \ + "$(case "$(err_of 'REPACK (VERBOSE) rp;')" in *"not supported"*) echo yes ;; *) echo no ;; esac)" "yes" +check "CLUSTER errors the same way" \ + "$(case "$(err_of 'CLUSTER rp USING rp_id;')" in *"not supported"*) echo yes ;; *) echo no ;; esac)" "yes" +check "VACUUM FULL errors the same way" \ + "$(case "$(err_of 'VACUUM FULL rp;')" in *"not supported"*) echo yes ;; *) echo no ;; esac)" "yes" + +# ---- REPACK (CONCURRENTLY), which needs two fixtures to mean anything --------- +# "REPACK CONCURRENTLY" without parentheses is not syntax; it is an option. +# +# The concurrent form has a precondition: the relation needs an identity index. On +# a table without one BOTH access methods fail, and the failure says so. Asserting +# only that is a trap, because the assertion passes for the wrong reason and would +# keep passing if the columnar behaviour changed. It is kept below, labelled as +# what it is, and paired with a fixture that MEETS the precondition. +# The distinction is the assertion: without an identity index the error is +# PostgreSQL's own precondition ("cannot execute REPACK (CONCURRENTLY) on +# relation"), raised before the access method is consulted. With one, below, the +# error is ours. Two different failures that a single "it errors" check would +# conflate. +nopk="$(err_of 'REPACK (CONCURRENTLY) rp;')" +check "without an identity index it is refused by PostgreSQL, not by us" \ + "$(case "$nopk" in + *"not supported"*) echo "no (ours: $nopk)" ;; + *"cannot execute REPACK"*) echo yes ;; + *) echo "no ($nopk)" ;; + esac)" \ + "yes" + +psql_run "CREATE TABLE rp_pk (id int PRIMARY KEY, v text) USING pgcolumnar; + INSERT INTO rp_pk SELECT g, 'x'||g FROM generate_series(1,$ROWS) g; + CREATE TABLE rp_pk_heap (id int PRIMARY KEY, v text); + INSERT INTO rp_pk_heap SELECT g, 'x'||g FROM generate_series(1,$ROWS) g;" +pk_rows="$(q 'SELECT count(*) FROM rp_pk')" +pk_hash="$(q "SELECT md5(string_agg(id::text||':'||v, ',' ORDER BY id)) FROM rp_pk")" + +# With the precondition met, heap succeeds. So the concurrent form IS reachable on +# this build, and a columnar failure is ours rather than a missing primary key. +check "control: with an identity index, REPACK (CONCURRENTLY) succeeds on heap" \ + "$(psql_run 'REPACK (CONCURRENTLY) rp_pk_heap;' 2>&1 | grep -c 'ERROR' || true)" "0" + +c_pk="$(err_of 'REPACK (CONCURRENTLY) rp_pk;')" +check "and on a columnar table it raises OUR error, so it is a columnar behaviour" \ + "$(case "$c_pk" in *"not supported"*) echo yes ;; *) echo "no ($c_pk)" ;; esac)" "yes" + +# A half-finished concurrent rewrite would be the bad outcome. It does not happen. +check "the columnar table is undamaged after the refused concurrent repack: rows" \ + "$(q 'SELECT count(*) FROM rp_pk')" "$pk_rows" +check "the columnar table is undamaged after the refused concurrent repack: content" \ + "$(q "SELECT md5(string_agg(id::text||':'||v, ',' ORDER BY id)) FROM rp_pk")" "$pk_hash" + +# ---- the control: the same command works on heap ----------------------------- +check "control: REPACK works on a heap table on this build" \ + "$(psql_run 'REPACK rp_heap;' 2>&1 | grep -c ERROR || true)" "0" + +# ---- a failed rewrite must leave the table untouched ------------------------- +check "the columnar table is unharmed: row count" "$(q 'SELECT count(*) FROM rp')" "$live" +check "the columnar table is unharmed: content hash" \ + "$(q "SELECT md5(string_agg(id::text||':'||v, ',' ORDER BY id)) FROM rp")" "$hash_before" +check "the columnar table is unharmed: storage options" \ + "$(q "SELECT stripe_row_limit || '/' || compression FROM pgcolumnar.options o + JOIN pg_class c ON c.oid = o.regclass WHERE c.relname = 'rp'")" "$opts_before" +check "the columnar table is unharmed: access method" \ + "$(q "SELECT amname FROM pg_am a JOIN pg_class c ON c.relam = a.oid WHERE c.relname = 'rp'")" \ + "pgcolumnar" + +# ---- and the supported route does work --------------------------------------- +check "pgcolumnar.vacuum is the supported alternative and succeeds" \ + "$(psql_run "SELECT pgcolumnar.vacuum('rp');" 2>&1 | grep -c ERROR || true)" "0" +check "and it kept every live row" "$(q 'SELECT count(*) FROM rp')" "$live" +check "and the content is still identical" \ + "$(q "SELECT md5(string_agg(id::text||':'||v, ',' ORDER BY id)) FROM rp")" "$hash_before" + +pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 8725ba6..2290c8b 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_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_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=(