diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bb9e311..6c1095d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,36 @@ true until the next version shipped. ### Fixed +- `DROP` after `ALTER TABLE ... SET ACCESS METHOD heap` now takes the + relid-keyed catalog rows with it, and the hook that does it stays out of the + way in databases that have no extension. + + **Two catalogs are keyed by relid rather than by storage id.** + `pgcolumnar.options` and `pgcolumnar.projection_declaration` survive the + rewrite onto heap storage, which drops only the storage-id catalogs. The drop + hook then looked at the access method, saw heap, and returned before touching + them. `config_dump` emits a bare oid for a relid that no longer resolves, and + `rebuild_projections()` aborts on an orphan declaration, taking every other + table in the database with it. That is the blast radius #304 closed for a + plain `DROP` of a still-columnar table, reached instead through access-method + conversion. + + **The hook runs in every database of the cluster, because the library is + preloaded rather than loaded by `CREATE EXTENSION`.** So it must reach the + columnar catalogs only where they exist. It now gates on + `pgcolumnar.options` resolving, not on the schema: `DROP EXTENSION` leaves the + schema behind and takes its tables, so a schema test passes in exactly the + case where the catalogs have gone. + + It also skips the extension's own member relations rather than everything in + the extension's schema. A user table that merely lives in `pgcolumnar` is not + a member, and it gets cleaned up like any other. + + This matters beyond `DROP TABLE`. Every table rewrite drops a transient + relation through the same hook, so `VACUUM FULL`, `CLUSTER`, + `ALTER TABLE ... ALTER COLUMN TYPE` and `CREATE MATERIALIZED VIEW` take the + path too, and `vacuumdb --full --all` visits every database in the cluster. + - `pgcolumnar.expire()` no longer drops live rows, and every path that renumbers live rows now clears the visibility map (#403). diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 2224270e..145c55db 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -31,6 +31,7 @@ #include "catalog/pg_class.h" #include "catalog/storage.h" #include "commands/defrem.h" +#include "commands/extension.h" #include "commands/vacuum.h" #include "executor/executor.h" #include "executor/tuptable.h" @@ -2612,15 +2613,61 @@ pgcolumnar_object_access(ObjectAccessType access, Oid classId, Oid objectId, * rewrites into fresh storage. */ pgcolumnar_delete_storage_tree(storageId); - PgColumnarDeleteOptions(objectId); + } + + /* + * Options and projection declarations are keyed by relid, not storage + * id. SET ACCESS METHOD heap rewrites the table onto heap storage and + * already drops the storage-id catalogs, but it leaves these rows. + * DROP then no longer sees a columnar AM, so a hook gated on the AM + * skipped them: config_dump emitted a bare oid, and + * rebuild_projections() aborted on the orphan declaration (#304's + * shape, reached through AM conversion rather than a plain DROP). + * + * Skip the extension's own catalog tables. DROP EXTENSION drops those + * as ordinary relations, and opening options or projection_declaration + * while they are themselves being dropped would fail. + */ + { + Oid columnarNsp = get_namespace_oid(COLUMNAR_SCHEMA_NAME, true); + Oid optionsOid = OidIsValid(columnarNsp) ? + get_relname_relid("options", columnarNsp) : InvalidOid; + Oid extOid = get_extension_oid("pgcolumnar", true); + /* - * And the projection declarations, for the same reason and in the - * same place (#304). A declaration left behind holds a regclass that - * no longer resolves, which config_dump then dumps as a bare OID and - * rebuild_projections() aborts on, taking every other table in the - * database with it. + * This hook is armed in EVERY database of the cluster, because the + * library is preloaded rather than loaded by CREATE EXTENSION. So it + * fires in a database where the extension was never installed and in + * one where it has been dropped. PgColumnarDeleteOptions opens the + * catalog through get_namespace_oid(..., false), which ERRORs there, + * and an ERROR raised in an object-access hook aborts the DROP that + * called it. That is not confined to DROP TABLE: every REWRITE drops + * a transient pg_temp_ through this hook, so VACUUM FULL, + * CLUSTER, ALTER TABLE ... ALTER COLUMN TYPE and CREATE MATERIALIZED + * VIEW take the same path. + * + * Gate on the OPTIONS TABLE resolving, not on the schema. DROP + * EXTENSION leaves the schema behind and takes its tables, so a + * schema test passes in exactly the case where the catalogs have + * gone -- which is why the two failures read differently ("schema + * pgcolumnar does not exist" where it was never installed, + * "columnar metadata table pgcolumnar.options does not exist" + * afterwards). + * + * Skip the extension's OWN MEMBERS rather than everything in its + * schema. DROP EXTENSION drops those as ordinary relations, and + * opening options while options is itself being dropped would fail. + * A user table that merely lives in the pgcolumnar schema is not a + * member, so it is still cleaned up -- which the schema test got + * wrong, leaking a row per such table. */ - PgColumnarDeleteProjectionDeclarationsForRel(objectId); + if (OidIsValid(optionsOid) && + (!OidIsValid(extOid) || + getExtensionOfObject(RelationRelationId, objectId) != extOid)) + { + PgColumnarDeleteOptions(objectId); + PgColumnarDeleteProjectionDeclarationsForRel(objectId); + } } relation_close(rel, NoLock); diff --git a/test/alter_am_cleanup.sh b/test/alter_am_cleanup.sh new file mode 100755 index 00000000..af327beb --- /dev/null +++ b/test/alter_am_cleanup.sh @@ -0,0 +1,256 @@ +#!/usr/bin/env bash +# +# pgColumnar: DROP TABLE after SET ACCESS METHOD heap must still take the +# relid-keyed catalogs with it. +# +# Storage-id catalogs (row_group, zone_map, ...) are already gone after the +# rewrite onto heap storage. pgcolumnar.options and projection_declaration are +# keyed by relid. The drop hook used to look at the access method and return +# before touching them, so converting away from columnar and then dropping left +# those rows behind. config_dump emits a bare oid for a relid that no longer +# resolves, and rebuild_projections() aborts on an orphan declaration -- the +# same blast radius #304 closed for a plain DROP of a still-columnar table. +# +# Usage: test/alter_am_cleanup.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}" + +# catalogs keyed by storage id, then the two keyed by relid +snapshot() { + q "SELECT (SELECT count(*) FROM pgcolumnar.storage) || '/' || + (SELECT count(*) FROM pgcolumnar.projection) || '/' || + (SELECT count(*) FROM pgcolumnar.row_group) || '/' || + (SELECT count(*) FROM pgcolumnar.options) || '/' || + (SELECT count(*) FROM pgcolumnar.projection_declaration);" | tail -1 +} + +base="$(snapshot)" + +psql_run "CREATE TABLE aac_t (id int, a int, b text) USING pgcolumnar;" +psql_run "SELECT pgcolumnar.set_options('aac_t', stripe_row_limit => 5000, + sort_by => ARRAY['id']);" +psql_run "SELECT pgcolumnar.add_projection('aac_t','p1',ARRAY['a','b'],ARRAY['a']);" +psql_run "INSERT INTO aac_t SELECT g, g, 'x' FROM generate_series(1,200) g;" + +grew="$(snapshot)" +check "premise: options and a projection declaration were recorded" \ + "$(awk -v a="$base" -v b="$grew" 'BEGIN { print (a == b) ? "no" : "yes" }')" "yes" + +psql_run "ALTER TABLE aac_t SET ACCESS METHOD heap;" +check "the table is heap after the conversion" \ + "$(q "SELECT am.amname FROM pg_class c JOIN pg_am am ON am.oid = c.relam + WHERE c.relname = 'aac_t'")" "heap" + +# Storage-id catalogs are already gone; the relid-keyed rows are what DROP +# used to miss. Do not require them to vanish at convert time: a round-trip +# back to columnar should still see the declared options. +check "converting to heap does not drop the declared options" \ + "$(q "SELECT stripe_row_limit FROM pgcolumnar.options + WHERE regclass = 'aac_t'::regclass")" "5000" + +psql_run "DROP TABLE aac_t;" +check "DROP after SET ACCESS METHOD heap leaves no options or declarations" \ + "$(snapshot)" "$base" + +# Round-trip: convert away and back, then DROP a still-columnar table. +psql_run "CREATE TABLE aac_rt (id int) USING pgcolumnar;" +psql_run "SELECT pgcolumnar.set_options('aac_rt', stripe_row_limit => 8000);" +psql_run "ALTER TABLE aac_rt SET ACCESS METHOD heap;" +psql_run "ALTER TABLE aac_rt SET ACCESS METHOD pgcolumnar;" +check "options survive a heap round-trip" \ + "$(q "SELECT stripe_row_limit FROM pgcolumnar.options + WHERE regclass = 'aac_rt'::regclass")" "8000" +psql_run "DROP TABLE aac_rt;" +check "a round-tripped table still cleans up on DROP" "$(snapshot)" "$base" + +# Control: a never-columnar heap must not be charged for this either. +psql_run "CREATE TABLE aac_h (id int);" +psql_run "DROP TABLE aac_h;" +# kept alive for the VACUUM FULL control further down +psql_run "CREATE TABLE aac_h_ctl (id int);" +check "dropping a heap table that was never columnar leaves the catalogs alone" \ + "$(snapshot)" "$base" + +# ---- the hook is armed in EVERY database, not only where the extension is ---- +# +# pgcolumnar_object_access reaches pgcolumnar.options to delete the relid-keyed +# rows. The library is preloaded, so the hook runs in a database where CREATE +# EXTENSION never ran and in one where the extension has been dropped. Reaching +# the catalogs there ERRORs, and an ERROR raised inside an object-access hook +# aborts the statement that fired it. +# +# The blast radius is not DROP TABLE. Every table REWRITE builds a transient +# pg_temp_ relation and performDeletion()s it through this same hook, so +# VACUUM FULL, CLUSTER, ALTER COLUMN TYPE and CREATE MATERIALIZED VIEW take the +# path too, and `vacuumdb --full --all` visits every database in the cluster. +# +# Every statement below is its own arm. An unasserted setup statement that fails +# makes the NEXT arm report a failure it did not cause: a CREATE MATERIALIZED +# VIEW left as setup turns the REFRESH arm into "relation does not exist", which +# reddens for the wrong reason and hides which statement the hook actually broke. + +# rc=0 on success, otherwise rc=N plus the first ERROR/FATAL line, so a red says +# which statement failed and how rather than only that something did. +aac_dbrun() { + local db="$1" sql="$2" out rc + out="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$db" -v ON_ERROR_STOP=1 -At -c "$sql" 2>&1)" + rc=$? + [ "$rc" = 0 ] && { printf 'rc=0\n'; return; } + printf 'rc=%s %s\n' "$rc" "$(printf '%s\n' "$out" | grep -m1 -E 'ERROR|FATAL' | head -c 120)" +} +aac_dbq() { + env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$1" -At -c "$2" 2>/dev/null || true +} + +psql_admin "CREATE DATABASE aac_nocx;" >/dev/null 2>&1 + +check "premise: the extension is absent in the second database" \ + "$(aac_dbq aac_nocx "SELECT count(*) FROM pg_extension WHERE extname='pgcolumnar'")" "0" +check "premise: and installed in this suite's own database" \ + "$(aac_dbq "$PGC_DB" "SELECT count(*) FROM pg_extension WHERE extname='pgcolumnar'")" "1" + +check "a plain table is created in a database without the extension" \ + "$(aac_dbrun aac_nocx 'CREATE TABLE nx (i int primary key, t text);')" "rc=0" +check "and dropped there" \ + "$(aac_dbrun aac_nocx 'DROP TABLE nx;')" "rc=0" +check "an explicit DROP of a temp table succeeds there" \ + "$(aac_dbrun aac_nocx 'CREATE TEMP TABLE nxt (i int); DROP TABLE nxt;')" "rc=0" +check "and ON COMMIT DROP commits there" \ + "$(aac_dbrun aac_nocx 'BEGIN; CREATE TEMP TABLE nxo (i int) ON COMMIT DROP; COMMIT;')" "rc=0" + +# The rewrite paths. Each drops a transient relation through the hook. +psql_admin "SELECT 1;" >/dev/null 2>&1 +check "premise: a table to rewrite exists in that database" \ + "$(aac_dbrun aac_nocx 'CREATE TABLE rw (i int primary key, t text); + INSERT INTO rw SELECT g, g::text FROM generate_series(1,100) g;')" "rc=0" +check "VACUUM FULL succeeds without the extension" \ + "$(aac_dbrun aac_nocx 'VACUUM FULL rw;')" "rc=0" +check "CLUSTER succeeds without the extension" \ + "$(aac_dbrun aac_nocx 'CLUSTER rw USING rw_pkey;')" "rc=0" +check "ALTER COLUMN TYPE succeeds without the extension" \ + "$(aac_dbrun aac_nocx 'ALTER TABLE rw ALTER COLUMN t TYPE varchar(64);')" "rc=0" +check "CREATE MATERIALIZED VIEW succeeds without the extension" \ + "$(aac_dbrun aac_nocx 'CREATE MATERIALIZED VIEW rwm AS SELECT * FROM rw;')" "rc=0" +check "and REFRESH MATERIALIZED VIEW does too" \ + "$(aac_dbrun aac_nocx 'REFRESH MATERIALIZED VIEW rwm;')" "rc=0" +check "TRUNCATE succeeds without the extension" \ + "$(aac_dbrun aac_nocx 'TRUNCATE rw;')" "rc=0" + +# Controls. Without these the arms above are satisfied by a hook that never runs +# at all, which would also break the cleanup this suite exists to test. +check "control: VACUUM FULL still succeeds where the extension IS installed" \ + "$(aac_dbrun "$PGC_DB" 'VACUUM FULL aac_h_ctl;')" "rc=0" + +# ---- a table in the extension's schema is ours to clean, not ours to skip ---- +# +# Skipping the whole pgcolumnar schema leaks a row per user table created there. +# The thing that must be skipped is the extension's OWN MEMBERS, which is a +# different set: DROP EXTENSION drops those as ordinary relations, and opening +# options while options is itself being dropped would fail. + +psql_run "CREATE TABLE pgcolumnar.aac_inschema (id int) USING pgcolumnar;" +psql_run "SELECT pgcolumnar.set_options('pgcolumnar.aac_inschema', stripe_row_limit => 7000);" +check "premise: a table in the extension's schema recorded its options" \ + "$(q "SELECT count(*) FROM pgcolumnar.options + WHERE regclass = 'pgcolumnar.aac_inschema'::regclass")" "1" +psql_run "DROP TABLE pgcolumnar.aac_inschema;" +check "dropping a table that lives in the extension's schema still cleans up" \ + "$(snapshot)" "$base" + +# ---- a relation that predates CREATE EXTENSION is not an extension member ---- +# +# getExtensionOfObject returns InvalidOid for it, exactly as it does for any +# other non-member, so it is cleaned up rather than skipped. Round-tripped +# through columnar so it has a row to leak if the member test were inverted. + +psql_admin "CREATE DATABASE aac_pre;" >/dev/null 2>&1 +check "premise: a table exists before the extension does" \ + "$(aac_dbrun aac_pre 'CREATE TABLE early (id int);')" "rc=0" +check "premise: and the extension is installed after it" \ + "$(aac_dbrun aac_pre 'CREATE EXTENSION pgcolumnar;')" "rc=0" +check "premise: the pre-existing table converts to columnar and records options" \ + "$(aac_dbrun aac_pre "ALTER TABLE early SET ACCESS METHOD pgcolumnar; + SELECT pgcolumnar.set_options('early', stripe_row_limit => 6000);")" "rc=0" +check "premise: its options row is really there" \ + "$(aac_dbq aac_pre "SELECT count(*) FROM pgcolumnar.options WHERE regclass = 'early'::regclass")" "1" +check "and dropping it after SET ACCESS METHOD heap leaves no orphan" \ + "$(aac_dbrun aac_pre 'ALTER TABLE early SET ACCESS METHOD heap; DROP TABLE early;')" "rc=0" +check "the pre-extension table's options row is gone" \ + "$(aac_dbq aac_pre "SELECT count(*) FROM pgcolumnar.options")" "0" + +# ---- the gate is on the OPTIONS TABLE, not on the schema -------------------- +# +# The hook gates on pgcolumnar.options resolving rather than on the pgcolumnar +# schema resolving. Measured on this tree rather than reasoned about: after +# DROP EXTENSION pgcolumnar the schema is STILL in pg_namespace and +# pgcolumnar.options is NOT in pg_class. The schema outlives the extension +# because it is not one of its members -- nothing depends on it with deptype +# 'e' -- while every catalog table in it is. +# +# So a gate on the schema opens a catalog that has gone, and an ERROR raised in +# an object_access hook aborts the command that called it. Not only DROP TABLE: +# every rewrite drops a transient pg_temp_ through this hook, so VACUUM +# FULL, CLUSTER, ALTER COLUMN TYPE and CREATE MATERIALIZED VIEW go too, and +# vacuumdb --full --all fails cluster-wide. +# +# aac_nocx above cannot hold this. It has neither the schema nor the tables, so +# a schema gate declines there for the same reason an options gate does and +# every one of its arms stays green. A database where the extension was +# installed and then dropped is the only place the two gates disagree, and this +# is that database. + +psql_admin "DROP DATABASE IF EXISTS aac_dropx;" >/dev/null 2>&1 +psql_admin "CREATE DATABASE aac_dropx;" >/dev/null 2>&1 +check "premise: the extension installs in a third database" \ + "$(aac_dbrun aac_dropx 'CREATE EXTENSION pgcolumnar;')" "rc=0" +check "premise: with a columnar table carrying both relid-keyed catalog rows" \ + "$(aac_dbrun aac_dropx "CREATE TABLE dx (id int, a int, b text) USING pgcolumnar; + SELECT pgcolumnar.set_options('dx', stripe_row_limit => 5000); + SELECT pgcolumnar.add_projection('dx','p1',ARRAY['a','b'],ARRAY['a']);")" "rc=0" +check "premise: those rows are really there before the extension goes" \ + "$(aac_dbq aac_dropx "SELECT (SELECT count(*) FROM pgcolumnar.options) || '/' || + (SELECT count(*) FROM pgcolumnar.projection_declaration)")" "1/1" + +# DROP EXTENSION drops options and projection_declaration as ordinary +# relations, each of them through this same hook, and drops the columnar table +# through it too. The member skip is what stops the hook opening a sibling +# catalog that DROP EXTENSION has already removed -- which would abort the +# DROP EXTENSION itself, leaving no way to remove the extension at all. +check "DROP EXTENSION succeeds with those catalogs populated" \ + "$(aac_dbrun aac_dropx 'DROP EXTENSION pgcolumnar CASCADE;')" "rc=0" + +check "premise: DROP EXTENSION left the schema behind" \ + "$(aac_dbq aac_dropx "SELECT count(*) FROM pg_namespace WHERE nspname='pgcolumnar'")" "1" +check "premise: and took pgcolumnar.options with it" \ + "$(aac_dbq aac_dropx "SELECT count(*) FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'pgcolumnar' AND c.relname = 'options'")" "0" + +check "a plain table is created after DROP EXTENSION" \ + "$(aac_dbrun aac_dropx 'CREATE TABLE dxh (i int primary key, t text);')" "rc=0" +check "and dropped there" \ + "$(aac_dbrun aac_dropx 'DROP TABLE dxh;')" "rc=0" +check "an explicit DROP of a temp table succeeds after DROP EXTENSION" \ + "$(aac_dbrun aac_dropx 'CREATE TEMP TABLE dxt (i int); DROP TABLE dxt;')" "rc=0" + +check "premise: a table to rewrite exists after DROP EXTENSION" \ + "$(aac_dbrun aac_dropx 'CREATE TABLE dxr (i int primary key, t text); + INSERT INTO dxr SELECT g, g::text FROM generate_series(1,100) g;')" "rc=0" +check "VACUUM FULL succeeds after DROP EXTENSION" \ + "$(aac_dbrun aac_dropx 'VACUUM FULL dxr;')" "rc=0" +check "CLUSTER succeeds after DROP EXTENSION" \ + "$(aac_dbrun aac_dropx 'CLUSTER dxr USING dxr_pkey;')" "rc=0" +check "ALTER COLUMN TYPE succeeds after DROP EXTENSION" \ + "$(aac_dbrun aac_dropx 'ALTER TABLE dxr ALTER COLUMN t TYPE varchar(64);')" "rc=0" +check "CREATE MATERIALIZED VIEW succeeds after DROP EXTENSION" \ + "$(aac_dbrun aac_dropx 'CREATE MATERIALIZED VIEW dxm AS SELECT * FROM dxr;')" "rc=0" +check "and REFRESH MATERIALIZED VIEW does too" \ + "$(aac_dbrun aac_dropx 'REFRESH MATERIALIZED VIEW dxm;')" "rc=0" +check "TRUNCATE succeeds after DROP EXTENSION" \ + "$(aac_dbrun aac_dropx 'TRUNCATE dxr;')" "rc=0" + +pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 53286c89..6682bb93 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -40,6 +40,7 @@ set -uo pipefail # lines and merge cleanly. Do not re-flow this into one line to save space. SUITES=( advisory_lock_class + alter_am_cleanup alter_column_type analyze_differential analyze_function