On 2020-01-06 13:32, Peter Eisentraut wrote:
Attached is a patch that attempts to fix this by propagating the storage change to existing indexes. This triggers a few regression test failures (going from no error to error), which I attempted to fix up, but I haven't analyzed what the tests were trying to do, so it might need another look.
Attached is a more polished patch, with tests. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From b46e75ffb1bed1228fbde3f9ceedd04e105699cb Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pe...@eisentraut.org> Date: Mon, 24 Feb 2020 08:24:15 +0100 Subject: [PATCH v2] Propagate ALTER TABLE ... SET STORAGE to indexes When creating a new index, the attstorage setting of the table column is copied to regular (non-expression) index columns. But a later ALTER TABLE ... SET STORAGE is not propagated to indexes, thus creating an inconsistent and undumpable state. Discussion: https://www.postgresql.org/message-id/flat/9765d72b-37c0-06f5-e349-2a580aafd989%402ndquadrant.com --- contrib/test_decoding/expected/toast.out | 4 +- contrib/test_decoding/sql/toast.sql | 4 +- src/backend/commands/tablecmds.c | 47 +++++++++++++++++++++++ src/test/regress/expected/alter_table.out | 20 ++++++++++ src/test/regress/expected/vacuum.out | 4 +- src/test/regress/sql/alter_table.sql | 6 +++ src/test/regress/sql/vacuum.sql | 4 +- 7 files changed, 81 insertions(+), 8 deletions(-) diff --git a/contrib/test_decoding/expected/toast.out b/contrib/test_decoding/expected/toast.out index 91a9a1e86d..2baa06f304 100644 --- a/contrib/test_decoding/expected/toast.out +++ b/contrib/test_decoding/expected/toast.out @@ -305,8 +305,8 @@ ALTER TABLE toasted_several REPLICA IDENTITY FULL; ALTER TABLE toasted_several ALTER COLUMN toasted_key SET STORAGE EXTERNAL; ALTER TABLE toasted_several ALTER COLUMN toasted_col1 SET STORAGE EXTERNAL; ALTER TABLE toasted_several ALTER COLUMN toasted_col2 SET STORAGE EXTERNAL; -INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 10000)); -SELECT pg_column_size(toasted_key) > 2^16 FROM toasted_several; +INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 269)); +SELECT pg_column_size(toasted_key) > 2^11 FROM toasted_several; ?column? ---------- t diff --git a/contrib/test_decoding/sql/toast.sql b/contrib/test_decoding/sql/toast.sql index ef11d2bfff..5cf6b87b3a 100644 --- a/contrib/test_decoding/sql/toast.sql +++ b/contrib/test_decoding/sql/toast.sql @@ -279,8 +279,8 @@ CREATE TABLE toasted_several ( ALTER TABLE toasted_several ALTER COLUMN toasted_col1 SET STORAGE EXTERNAL; ALTER TABLE toasted_several ALTER COLUMN toasted_col2 SET STORAGE EXTERNAL; -INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 10000)); -SELECT pg_column_size(toasted_key) > 2^16 FROM toasted_several; +INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 269)); +SELECT pg_column_size(toasted_key) > 2^11 FROM toasted_several; SELECT regexp_replace(data, '^(.{100}).*(.{100})$', '\1..\2') FROM pg_logical_slot_peek_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1'); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index b7c8d663fc..8c24a7a0e3 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7383,6 +7383,7 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc Form_pg_attribute attrtuple; AttrNumber attnum; ObjectAddress address; + ListCell *lc; Assert(IsA(newValue, String)); storagemode = strVal(newValue); @@ -7442,6 +7443,52 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc heap_freetuple(tuple); + /* + * Apply the change to indexes as well (only for simple index columns, + * matching behavior of index.c ConstructTupleDescriptor()). + */ + foreach(lc, RelationGetIndexList(rel)) + { + Oid indexoid = lfirst_oid(lc); + Relation indrel; + AttrNumber indattnum = 0; + + indrel = index_open(indexoid, lockmode); + + for (int i = 0; i < indrel->rd_index->indnatts; i++) + { + if (indrel->rd_index->indkey.values[i] == attnum) + { + indattnum = i + 1; + break; + } + } + + if (indattnum == 0) + { + index_close(indrel, lockmode); + continue; + } + + tuple = SearchSysCacheCopyAttNum(RelationGetRelid(indrel), indattnum); + + if (HeapTupleIsValid(tuple)) + { + attrtuple = (Form_pg_attribute) GETSTRUCT(tuple); + attrtuple->attstorage = newstorage; + + CatalogTupleUpdate(attrelation, &tuple->t_self, tuple); + + InvokeObjectPostAlterHook(RelationRelationId, + RelationGetRelid(rel), + attrtuple->attnum); + + heap_freetuple(tuple); + } + + index_close(indrel, lockmode); + } + table_close(attrelation, RowExclusiveLock); ObjectAddressSubSet(address, RelationRelationId, diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index fb6d86a269..00aaa23def 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -2166,6 +2166,26 @@ where oid = 'test_storage'::regclass; t (1 row) +-- test that SET STORAGE propagates to index correctly +create index test_storage_idx on test_storage (b, a); +alter table test_storage alter column a set storage external; +\d+ test_storage + Table "public.test_storage" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+----------+--------------+------------- + a | text | | | | external | | + b | integer | | | 0 | plain | | +Indexes: + "test_storage_idx" btree (b, a) + +\d+ test_storage_idx + Index "public.test_storage_idx" + Column | Type | Key? | Definition | Storage | Stats target +--------+---------+------+------------+----------+-------------- + b | integer | yes | b | plain | + a | text | yes | a | external | +btree, for table "public.test_storage" + -- ALTER COLUMN TYPE with a check constraint and a child table (bug #13779) CREATE TABLE test_inh_check (a float check (a > 10.2), b float); CREATE TABLE test_inh_check_child() INHERITS(test_inh_check); diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index 0cfe28e63f..93dbd52548 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -132,7 +132,7 @@ CREATE TABLE no_index_cleanup (i INT PRIMARY KEY, t TEXT); CREATE INDEX no_index_cleanup_idx ON no_index_cleanup(t); ALTER TABLE no_index_cleanup ALTER COLUMN t SET STORAGE EXTERNAL; INSERT INTO no_index_cleanup(i, t) VALUES (generate_series(1,30), - repeat('1234567890',300)); + repeat('1234567890',269)); -- index cleanup option is ignored if VACUUM FULL VACUUM (INDEX_CLEANUP TRUE, FULL TRUE) no_index_cleanup; VACUUM (FULL TRUE) no_index_cleanup; @@ -146,7 +146,7 @@ ALTER TABLE no_index_cleanup SET (vacuum_index_cleanup = true); VACUUM no_index_cleanup; -- Parameter is set for both the parent table and its toast relation. INSERT INTO no_index_cleanup(i, t) VALUES (generate_series(31,60), - repeat('1234567890',300)); + repeat('1234567890',269)); DELETE FROM no_index_cleanup WHERE i < 45; -- Only toast index is cleaned up. ALTER TABLE no_index_cleanup SET (vacuum_index_cleanup = false, diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 3801f19c58..6ab44c8786 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1465,6 +1465,12 @@ CREATE TEMP TABLE FKTABLE (ftest1 int); from pg_class where oid = 'test_storage'::regclass; +-- test that SET STORAGE propagates to index correctly +create index test_storage_idx on test_storage (b, a); +alter table test_storage alter column a set storage external; +\d+ test_storage +\d+ test_storage_idx + -- ALTER COLUMN TYPE with a check constraint and a child table (bug #13779) CREATE TABLE test_inh_check (a float check (a > 10.2), b float); CREATE TABLE test_inh_check_child() INHERITS(test_inh_check); diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql index cf741f7b11..2a2b09c6ee 100644 --- a/src/test/regress/sql/vacuum.sql +++ b/src/test/regress/sql/vacuum.sql @@ -112,7 +112,7 @@ CREATE TABLE no_index_cleanup (i INT PRIMARY KEY, t TEXT); CREATE INDEX no_index_cleanup_idx ON no_index_cleanup(t); ALTER TABLE no_index_cleanup ALTER COLUMN t SET STORAGE EXTERNAL; INSERT INTO no_index_cleanup(i, t) VALUES (generate_series(1,30), - repeat('1234567890',300)); + repeat('1234567890',269)); -- index cleanup option is ignored if VACUUM FULL VACUUM (INDEX_CLEANUP TRUE, FULL TRUE) no_index_cleanup; VACUUM (FULL TRUE) no_index_cleanup; @@ -126,7 +126,7 @@ CREATE INDEX no_index_cleanup_idx ON no_index_cleanup(t); VACUUM no_index_cleanup; -- Parameter is set for both the parent table and its toast relation. INSERT INTO no_index_cleanup(i, t) VALUES (generate_series(31,60), - repeat('1234567890',300)); + repeat('1234567890',269)); DELETE FROM no_index_cleanup WHERE i < 45; -- Only toast index is cleaned up. ALTER TABLE no_index_cleanup SET (vacuum_index_cleanup = false, base-commit: 79c2385915dd4aa43127e766c3dce323ec562ba0 -- 2.25.0
