On Sun, Apr 10, 2011 at 07:35:53AM -0400, Robert Haas wrote:
> On Sun, Apr 10, 2011 at 6:36 AM, Noah Misch <[email protected]> wrote:
> > 3. Make AlterTableCreateToastTable acquire only ShareUpdateExclusiveLock and
> > remove the pass-usage heuristic from ATRewriteCatalogs. For this to be
> > valid,
> > toast_insert_or_update() must behave reasonably in the face of a relation
> > concurrently acquiring a TOAST table. Since it takes reltoastrelid from the
> > relcache, toast_insert_or_update() will not act on the change in the middle
> > of a
> > single call. Even if it did, I don't see any risks.
> >
> > I'd lean toward #3 if someone else is also confident in its correctness.
> > Otherwise, #1 seems like the way to go. Preferences? Other ideas?
>
> I haven't scrutinized the code but I would prefer #3 if it's viable
> without too much of a code footprint.
It's certainly compact; patch attached.
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 5d5496d..cb0df40 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);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 886b656..03d1efa 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3014,18 +3014,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 d7d1b64..315b915 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 0930a6a..7cd26cb 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 c225d0f..34bde31 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 749584d..43a9ce9 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
--
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
