ALTER TABLE ... SET STORAGE does not propagate to indexes
ALTER TABLE ... SET STORAGE does not propagate to indexes, even though
indexes created afterwards get the new storage setting. So depending on
the order of commands, you can get inconsistent storage settings between
indexes and tables. For example:
create table foo1 (a text);
alter table foo1 alter column a set storage external;
create index foo1i on foo1(a);
insert into foo1 values(repeat('a', 10000));
ERROR: index row requires 10016 bytes, maximum size is 8191
(Storage "external" disables compression.)
but
create table foo1 (a text);
create index foo1i on foo1(a);
alter table foo1 alter column a set storage external;
insert into foo1 values(repeat('a', 10000));
-- no error
Also, this second state cannot be reproduced by pg_dump, so a possible
effect is that such a database would fail to restore.
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.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Propagate-ALTER-TABLE-.-SET-STORAGE-to-indexes.patchtext/plain; charset=UTF-8; name=0001-Propagate-ALTER-TABLE-.-SET-STORAGE-to-indexes.patch; x-mac-creator=0; x-mac-type=0Download
From 85f19d31a2def2e2b98d669daa4bd4a3a2c2c09f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 6 Jan 2020 11:54:03 +0100
Subject: [PATCH] Propagate ALTER TABLE ... SET STORAGE to indexes
---
contrib/test_decoding/expected/toast.out | 4 +--
contrib/test_decoding/sql/toast.sql | 4 +--
src/backend/commands/tablecmds.c | 33 ++++++++++++++++++++++++
src/test/regress/expected/vacuum.out | 4 +--
src/test/regress/sql/vacuum.sql | 4 +--
5 files changed, 41 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 1c4394abea..c4bc058e28 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6900,6 +6900,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);
@@ -6959,6 +6960,38 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc
heap_freetuple(tuple);
+ /*
+ * Apply the change to indexes as well.
+ *
+ * XXX should probably use pg_index.indkey to find the right columns, not
+ * the column name
+ */
+ foreach(lc, RelationGetIndexList(rel))
+ {
+ Oid indexoid = lfirst_oid(lc);
+ Relation indrel;
+
+ indrel = index_open(indexoid, lockmode);
+
+ tuple = SearchSysCacheCopyAttName(RelationGetRelid(indrel), colName);
+
+ 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/vacuum.out b/src/test/regress/expected/vacuum.out
index 9996d882d1..e2b9cd954d 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -98,7 +98,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;
@@ -112,7 +112,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/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 69987f75e9..aaacdfe082 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -81,7 +81,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;
@@ -95,7 +95,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.24.1
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
Attachments:
v2-0001-Propagate-ALTER-TABLE-.-SET-STORAGE-to-indexes.patchtext/plain; charset=UTF-8; name=v2-0001-Propagate-ALTER-TABLE-.-SET-STORAGE-to-indexes.patch; x-mac-creator=0; x-mac-type=0Download
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.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
Hello Peter,
On Mon, Feb 24, 2020 at 12:59 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
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.
I've reproduced the issue on head. And, the patch seems to solve the
problem. The patch looks good to me. But, I've a small doubt regarding
the changes in test_decoding regression file.
diff --git a/contrib/test_decoding/sql/toast.sql
b/contrib/test_decoding/sql/toast.sql
..
-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;This actually tests whether we can decode "old" tuples bigger than the
max heap tuple size correctly which is around 8KB. But, the above
changes will make the tuple size around 3KB. So, it'll not be able to
test that particular scenario.Thoughts?
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com
On 2020-02-24 12:21, Kuntal Ghosh wrote:
I've reproduced the issue on head. And, the patch seems to solve the
problem. The patch looks good to me. But, I've a small doubt regarding
the changes in test_decoding regression file.diff --git a/contrib/test_decoding/sql/toast.sql b/contrib/test_decoding/sql/toast.sql .. -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;This actually tests whether we can decode "old" tuples bigger than the
max heap tuple size correctly which is around 8KB. But, the above
changes will make the tuple size around 3KB. So, it'll not be able to
test that particular scenario.Thoughts?
OK, this is interesting. The details of this are somewhat unfamiliar to
me, but it appears that due to TOAST_INDEX_HACK in indextuple.c, an
index tuple cannot be larger than 8191 bytes when untoasted (but not
uncompressed).
What the test case above is testing is a situation where the heap tuple
is stored toasted uncompressed (storage external) but the index tuple is
not (probably compressed inline). This is exactly the situation that I
was contending should not be possible, because it cannot be dumped or
restored.
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.
Thoughts?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Feb 25, 2020 at 1:09 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 2020-02-24 12:21, Kuntal Ghosh wrote:
I've reproduced the issue on head. And, the patch seems to solve the
problem. The patch looks good to me. But, I've a small doubt regarding
the changes in test_decoding regression file.diff --git a/contrib/test_decoding/sql/toast.sql b/contrib/test_decoding/sql/toast.sql .. -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;This actually tests whether we can decode "old" tuples bigger than the
max heap tuple size correctly which is around 8KB. But, the above
changes will make the tuple size around 3KB. So, it'll not be able to
test that particular scenario.Thoughts?OK, this is interesting. The details of this are somewhat unfamiliar to
me, but it appears that due to TOAST_INDEX_HACK in indextuple.c, an
index tuple cannot be larger than 8191 bytes when untoasted (but not
uncompressed).What the test case above is testing is a situation where the heap tuple
is stored toasted uncompressed (storage external) but the index tuple is
not (probably compressed inline). This is exactly the situation that I
was contending should not be possible, because it cannot be dumped or
restored.
Yeah. If we only commit this patch to fix the issue, we're going to
put some restriction for the above situation, i.e., the index for an
external attribute has to be stored as an external (i.e. uncompressed)
value. So, a lot of existing workload might start failing after an
upgrade. I think there should be an option to store the index of an
external attribute as a compressed inline value.
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.Thoughts?
Yes. We need the support for this syntax along with the bug fix patch.
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com
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.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
ALTER TABLE ... SET STORAGE does not propagate to indexes, even though
indexes created afterwards get the new storage setting. So depending on
the order of commands, you can get inconsistent storage settings between
indexes and tables.
I've absolutely noticed this behavior, I just thought it was intentional
for some reason.
Having this behavior change as stated above would be very welcome in my
opinion. It's always something i've had to manually think about in my
migration scripts, so it would be welcome from my view.
-Adam
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
Attachments:
v3-0001-Propagate-ALTER-TABLE-.-SET-STORAGE-to-indexes.patchtext/plain; charset=UTF-8; name=v3-0001-Propagate-ALTER-TABLE-.-SET-STORAGE-to-indexes.patch; x-mac-creator=0; x-mac-type=0Download
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.out
index 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.sql
index 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.c
index 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.out
index 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.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 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.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,
--
2.26.0
I'm surprised that this hasn't applied yet, because:
On 2020-Apr-09, Peter Eisentraut wrote:
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.
So I have to ask -- are you planning to get this patch pushed and
backpatched?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020-04-22 01:56, Alvaro Herrera wrote:
I'm surprised that this hasn't applied yet, because:
On 2020-Apr-09, Peter Eisentraut wrote:
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.So I have to ask -- are you planning to get this patch pushed and
backpatched?
I think I should, but I figured I want to give some extra time for
people to consider the horror that I created in the test_decoding tests.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020-04-22 16:26, Peter Eisentraut wrote:
On 2020-04-22 01:56, Alvaro Herrera wrote:
I'm surprised that this hasn't applied yet, because:
On 2020-Apr-09, Peter Eisentraut wrote:
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.So I have to ask -- are you planning to get this patch pushed and
backpatched?I think I should, but I figured I want to give some extra time for
people to consider the horror that I created in the test_decoding tests.
OK then, if there are no last-minute objects, I'll commit this for the
upcoming minor releases.
This is the patch summary again:
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.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020-05-06 16:37, Peter Eisentraut wrote:
On 2020-04-22 16:26, Peter Eisentraut wrote:
On 2020-04-22 01:56, Alvaro Herrera wrote:
I'm surprised that this hasn't applied yet, because:
On 2020-Apr-09, Peter Eisentraut wrote:
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.So I have to ask -- are you planning to get this patch pushed and
backpatched?I think I should, but I figured I want to give some extra time for
people to consider the horror that I created in the test_decoding tests.OK then, if there are no last-minute objects, I'll commit this for the
upcoming minor releases.
I have committed this and backpatched to PG12 and PG11. Before that,
the catalog manipulation code is factored quite differently and it would
be more complicated to backpatch and I didn't find that worth it.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services