From 39a68e5c6ca7b41b889e773ca58535324af69630 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Wed, 13 Apr 2011 18:07:14 -0700 Subject: [PATCH] Fix toast table creation. Instead of using slightly-too-clever heuristics to decide when we must create a TOAST table, just check whether one is needed every time the table is altered. Checking whether a toast table is needed is cheap enough that we needn't worry about doing it on every ALTER TABLE command, and the previous coding is apparently prone to accidental breakage: commit 04e17bae50a73af524731fa11210d5c3f7d8e1f9 broken ALTER TABLE .. SET STORAGE, which moved some actions from AT_PASS_COL_ATTRS to AT_PASS_MISC, and commit 6c5723998594dffa5d47c3cf8c96ccf89c033aae broke ALTER TABLE .. ADD COLUMN by changing the way that adding columns recurses into child tables. Noah Misch, with one comment change by me --- src/backend/catalog/toasting.c | 10 +++++----- src/backend/commands/tablecmds.c | 10 ++-------- src/test/regress/expected/alter_table.out | 13 +++++++++++++ src/test/regress/input/misc.source | 6 ++++++ src/test/regress/output/misc.source | 11 +++++++++++ src/test/regress/sql/alter_table.sql | 10 ++++++++++ 6 files changed, 47 insertions(+), 13 deletions(-) diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c index 472237c4a0f..85fe57fb2a5 100644 --- a/src/backend/catalog/toasting.c +++ b/src/backend/catalog/toasting.c @@ -59,11 +59,11 @@ AlterTableCreateToastTable(Oid relOid, Datum reloptions) Relation rel; /* - * Grab an exclusive lock on the target table, which we will NOT release - * until end of transaction. (This is probably redundant in all present - * uses...) + * Grab a DDL-exclusive lock on the target table, since we'll update the + * pg_class tuple. This is redundant for all present users. Tuple toasting + * behaves safely in the face of a concurrent TOAST table add. */ - rel = heap_open(relOid, AccessExclusiveLock); + rel = heap_open(relOid, ShareUpdateExclusiveLock); /* create_toast_table does all the work */ (void) create_toast_table(rel, InvalidOid, InvalidOid, reloptions); @@ -103,7 +103,7 @@ BootstrapToastTable(char *relName, Oid toastOid, Oid toastIndexOid) /* * create_toast_table --- internal workhorse * - * rel is already opened and exclusive-locked + * rel is already opened and locked * toastOid and toastIndexOid are normally InvalidOid, but during * bootstrap they can be nonzero to specify hand-assigned OIDs */ diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 6afebc728f5..d3692754959 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -3022,18 +3022,12 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode) } } - /* - * Check to see if a toast table must be added, if we executed any - * subcommands that might have added a column or changed column storage. - */ + /* Check to see if a toast table must be added. */ foreach(ltab, *wqueue) { AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab); - if (tab->relkind == RELKIND_RELATION && - (tab->subcmds[AT_PASS_ADD_COL] || - tab->subcmds[AT_PASS_ALTER_TYPE] || - tab->subcmds[AT_PASS_COL_ATTRS])) + if (tab->relkind == RELKIND_RELATION) AlterTableCreateToastTable(tab->relid, (Datum) 0); } } diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index d7d1b646cfa..315b915c13c 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1522,6 +1522,19 @@ ERROR: composite type recur1 cannot be made a member of itself alter table recur1 add column f2 int; alter table recur1 alter column f2 type recur2; -- fails ERROR: composite type recur1 cannot be made a member of itself +-- SET STORAGE may need to add a TOAST table +create table test_storage (a text); +alter table test_storage alter a set storage plain; +alter table test_storage add b int default 0; -- rewrite table to remove its TOAST table +alter table test_storage alter a set storage extended; -- re-add TOAST table +select reltoastrelid <> 0 as has_toast_table +from pg_class +where oid = 'test_storage'::regclass; + has_toast_table +----------------- + t +(1 row) + -- -- lock levels -- diff --git a/src/test/regress/input/misc.source b/src/test/regress/input/misc.source index 0930a6a4bb1..7cd26cb192d 100644 --- a/src/test/regress/input/misc.source +++ b/src/test/regress/input/misc.source @@ -153,6 +153,12 @@ SELECT * FROM e_star*; ALTER TABLE a_star* ADD COLUMN a text; +-- That ALTER TABLE should have added TOAST tables. +SELECT relname, reltoastrelid <> 0 AS has_toast_table + FROM pg_class + WHERE oid::regclass IN ('a_star', 'c_star') + ORDER BY 1; + --UPDATE b_star* -- SET a = text 'gazpacho' -- WHERE aa > 4; diff --git a/src/test/regress/output/misc.source b/src/test/regress/output/misc.source index c225d0f37f0..34bde31b9f1 100644 --- a/src/test/regress/output/misc.source +++ b/src/test/regress/output/misc.source @@ -376,6 +376,17 @@ SELECT * FROM e_star*; ALTER TABLE a_star* ADD COLUMN a text; NOTICE: merging definition of column "a" for child "d_star" +-- That ALTER TABLE should have added TOAST tables. +SELECT relname, reltoastrelid <> 0 AS has_toast_table + FROM pg_class + WHERE oid::regclass IN ('a_star', 'c_star') + ORDER BY 1; + relname | has_toast_table +---------+----------------- + a_star | t + c_star | t +(2 rows) + --UPDATE b_star* -- SET a = text 'gazpacho' -- WHERE aa > 4; diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 749584d9e63..43a9ce971f6 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1133,6 +1133,16 @@ alter table recur1 add column f2 recur2; -- fails alter table recur1 add column f2 int; alter table recur1 alter column f2 type recur2; -- fails +-- SET STORAGE may need to add a TOAST table +create table test_storage (a text); +alter table test_storage alter a set storage plain; +alter table test_storage add b int default 0; -- rewrite table to remove its TOAST table +alter table test_storage alter a set storage extended; -- re-add TOAST table + +select reltoastrelid <> 0 as has_toast_table +from pg_class +where oid = 'test_storage'::regclass; + -- -- lock levels --