From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Kuntal Ghosh <kuntalghosh.2007@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: ALTER TABLE ... SET STORAGE does not propagate to indexes
Date: Thu, 9 Apr 2020 15:07:40 +0200
Message-ID: <4e53fcdd-c427-6d3b-e871-9b7af0c36b70@2ndquadrant.com> (raw)
In-Reply-To: <20200330161720.GA17240@alvherre.pgsql>
References: <20200330161720.GA17240@alvherre.pgsql>
On 2020-03-30 18:17, Alvaro Herrera wrote:
> On 2020-Feb-25, Peter Eisentraut wrote:
>> An alternative would be that we make this situation fully supported. Then
>> we'd probably need at least ALTER INDEX ... ALTER COLUMN ... SET STORAGE,
>> and some pg_dump support.
>
> I think this is a more promising direction.
I have started implementing the ALTER INDEX command, which by itself
isn't very hard, but it requires significant new infrastructure in
pg_dump, and probably also a bit of work in psql, and that's all a bit
too much right now.
An alternative for the short term is the attached patch. It's the same
as before, but I have hacked up the test_decoding test to achieve the
effect of ALTER INDEX with direct catalog manipulation. This preserves
the spirit of the test case, but allows us to fix everything else about
this situation.
One thing to remember is that the current situation is broken. While
you can set index columns to have different storage than the
corresponding table columns, pg_dump does not preserve that, because it
dumps indexes after ALTER TABLE commands. So at the moment, having
these two things different isn't really supported. The proposed patch
just makes this behave consistently and allows adding an ALTER INDEX
command later on if desired.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From b349cb8ff56c8ec547420994fb037d6c4b397193 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 9 Apr 2020 14:10:01 +0200
Subject: [PATCH v3] 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 | 5 +++
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, 86 insertions(+), 4 deletions(-)
diff --git a/contrib/test_decoding/expected/toast.out b/contrib/test_decoding/expected/toast.outindex 91a9a1e86d..75c4d22d80 100644--- a/contrib/test_decoding/expected/toast.out+++ b/contrib/test_decoding/expected/toast.out@@ -305,6 +305,10 @@ 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;
+-- Change the storage of the index back to EXTENDED, separately from+-- the table. This is currently not doable via DDL, but it is+-- supported internally.+UPDATE pg_attribute SET attstorage = 'x' WHERE attrelid = 'toasted_several_pkey'::regclass AND attname = 'toasted_key';
INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 10000));
SELECT pg_column_size(toasted_key) > 2^16 FROM toasted_several;
?column?
diff --git a/contrib/test_decoding/sql/toast.sql b/contrib/test_decoding/sql/toast.sqlindex ef11d2bfff..016c3ab784 100644--- a/contrib/test_decoding/sql/toast.sql+++ b/contrib/test_decoding/sql/toast.sql@@ -279,6 +279,11 @@ 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;
+-- Change the storage of the index back to EXTENDED, separately from+-- the table. This is currently not doable via DDL, but it is+-- supported internally.+UPDATE pg_attribute SET attstorage = 'x' WHERE attrelid = 'toasted_several_pkey'::regclass AND attname = 'toasted_key';+
INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 10000));
SELECT pg_column_size(toasted_key) > 2^16 FROM toasted_several;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.cindex 6162fb018c..8b0194e17f 100644--- a/src/backend/commands/tablecmds.c+++ b/src/backend/commands/tablecmds.c@@ -7382,6 +7382,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);
@@ -7441,6 +7442,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 f343f9b42e..99ff6973bc 100644--- a/src/test/regress/expected/alter_table.out+++ b/src/test/regress/expected/alter_table.out@@ -2172,6 +2172,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 fb5c9139d1..b45ba2a322 100644--- a/src/test/regress/sql/alter_table.sql+++ b/src/test/regress/sql/alter_table.sql@@ -1472,6 +1472,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,
--
2.26.0
Attachments:
[text/plain] v3-0001-Propagate-ALTER-TABLE-.-SET-STORAGE-to-indexes.patch (8.9K, ../4e53fcdd-c427-6d3b-e871-9b7af0c36b70@2ndquadrant.com/2-v3-0001-Propagate-ALTER-TABLE-.-SET-STORAGE-to-indexes.patch)
download | inline diff:
From b349cb8ff56c8ec547420994fb037d6c4b397193 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 9 Apr 2020 14:10:01 +0200
Subject: [PATCH v3] 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 | 5 +++
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, 86 insertions(+), 4 deletions(-)
diff --git a/contrib/test_decoding/expected/toast.out b/contrib/test_decoding/expected/toast.outindex 91a9a1e86d..75c4d22d80 100644--- a/contrib/test_decoding/expected/toast.out+++ b/contrib/test_decoding/expected/toast.out@@ -305,6 +305,10 @@ 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;
+-- Change the storage of the index back to EXTENDED, separately from+-- the table. This is currently not doable via DDL, but it is+-- supported internally.+UPDATE pg_attribute SET attstorage = 'x' WHERE attrelid = 'toasted_several_pkey'::regclass AND attname = 'toasted_key';
INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 10000));
SELECT pg_column_size(toasted_key) > 2^16 FROM toasted_several;
?column?
diff --git a/contrib/test_decoding/sql/toast.sql b/contrib/test_decoding/sql/toast.sqlindex ef11d2bfff..016c3ab784 100644--- a/contrib/test_decoding/sql/toast.sql+++ b/contrib/test_decoding/sql/toast.sql@@ -279,6 +279,11 @@ 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;
+-- Change the storage of the index back to EXTENDED, separately from+-- the table. This is currently not doable via DDL, but it is+-- supported internally.+UPDATE pg_attribute SET attstorage = 'x' WHERE attrelid = 'toasted_several_pkey'::regclass AND attname = 'toasted_key';+
INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 10000));
SELECT pg_column_size(toasted_key) > 2^16 FROM toasted_several;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.cindex 6162fb018c..8b0194e17f 100644--- a/src/backend/commands/tablecmds.c+++ b/src/backend/commands/tablecmds.c@@ -7382,6 +7382,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);
@@ -7441,6 +7442,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 f343f9b42e..99ff6973bc 100644--- a/src/test/regress/expected/alter_table.out+++ b/src/test/regress/expected/alter_table.out@@ -2172,6 +2172,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 fb5c9139d1..b45ba2a322 100644--- a/src/test/regress/sql/alter_table.sql+++ b/src/test/regress/sql/alter_table.sql@@ -1472,6 +1472,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,
--
2.26.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, alvherre@2ndquadrant.com, kuntalghosh.2007@gmail.com
Subject: Re: ALTER TABLE ... SET STORAGE does not propagate to indexes
In-Reply-To: <4e53fcdd-c427-6d3b-e871-9b7af0c36b70@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