From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: ALTER TABLE ... SET STORAGE does not propagate to indexes
Date: Mon, 24 Feb 2020 08:28:57 +0100
Message-ID: <d283ff5e-5cc0-d141-998e-1b0983a84859@2ndquadrant.com> (raw)
In-Reply-To: <9765d72b-37c0-06f5-e349-2a580aafd989@2ndquadrant.com>
References: <9765d72b-37c0-06f5-e349-2a580aafd989@2ndquadrant.com>
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 <peter@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.outindex 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.sqlindex 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.cindex 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.outindex 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.outindex 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.sqlindex 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.sqlindex 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
Attachments:
[text/plain] v2-0001-Propagate-ALTER-TABLE-.-SET-STORAGE-to-indexes.patch (8.9K, ../d283ff5e-5cc0-d141-998e-1b0983a84859@2ndquadrant.com/2-v2-0001-Propagate-ALTER-TABLE-.-SET-STORAGE-to-indexes.patch)
download | inline diff:
From b46e75ffb1bed1228fbde3f9ceedd04e105699cb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@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.outindex 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.sqlindex 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.cindex 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.outindex 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.outindex 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.sqlindex 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.sqlindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: peter.eisentraut@2ndquadrant.com
Subject: Re: ALTER TABLE ... SET STORAGE does not propagate to indexes
In-Reply-To: <d283ff5e-5cc0-d141-998e-1b0983a84859@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox