Hi,

From time to time I run into the limitation that ALTER TYPE does not
allow changing storage strategy. I've written a bunch of data types over
the years - in some cases I simply forgot to specify storage in CREATE
TYPE (so it defaulted to PLAIN) or I expected PLAIN to be sufficient and
then later wished I could enable TOAST.

Obviously, without ALTER TYPE supporting that it's rather tricky. You
either need to do dump/restore, or tweak the pg_type catalog directly.
So here is an initial patch extending ALTER TYPE to support this.

The question is why this was not implemented before - my assumption is
this is not simply because no one wanted that. We track the storage in
pg_attribute too, and ALTER TABLE allows changing that ...

My understanding is that pg_type.typstorage essentially specifies two
things: (a) default storage strategy for the attributes with this type,
and (b) whether the type implementation is prepared to handle TOAST-ed
values or not. And pg_attribute.attstorage has to respect this - when
the type says PLAIN then the attribute can't simply use strategy that
would enable TOAST.

Luckily, this is only a problem when switching typstorage to PLAIN (i.e.
when disabling TOAST for the type). The attached v1 patch checks if
there are attributes with non-PLAIN storage for this type, and errors
out if it finds one. But unfortunately that's not entirely correct,
because ALTER TABLE only sets storage for new data. A table may be
created with e.g. EXTENDED storage for an attribute, a bunch of rows may
be loaded and then the storage for the attribute may be changed to
PLAIN. That would pass the check as it's currently in the patch, yet
there may be TOAST-ed values for the type with PLAIN storage :-(

I'm not entirely sure what to do about this, but I think it's OK to just
reject changes in this direction (from non-PLAIN to PLAIN storage). I've
never needed it, and it seems pretty useless - it seems fine to just
instruct the user to do a dump/restore.

In the opposite direction - when switching from PLAIN to a TOAST-enabled
storage, or enabling/disabling compression, this is not an issue at all.
It's legal for type to specify e.g. EXTENDED but attribute to use PLAIN,
for example. So the attached v1 patch simply allows this direction.


regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
index 67be1dd568..03a6a59468 100644
--- a/doc/src/sgml/ref/alter_type.sgml
+++ b/doc/src/sgml/ref/alter_type.sgml
@@ -30,6 +30,7 @@ ALTER TYPE <replaceable class="parameter">name</replaceable> 
RENAME TO <replacea
 ALTER TYPE <replaceable class="parameter">name</replaceable> SET SCHEMA 
<replaceable class="parameter">new_schema</replaceable>
 ALTER TYPE <replaceable class="parameter">name</replaceable> ADD VALUE [ IF 
NOT EXISTS ] <replaceable class="parameter">new_enum_value</replaceable> [ { 
BEFORE | AFTER } <replaceable 
class="parameter">neighbor_enum_value</replaceable> ]
 ALTER TYPE <replaceable class="parameter">name</replaceable> RENAME VALUE 
<replaceable class="parameter">existing_enum_value</replaceable> TO 
<replaceable class="parameter">new_enum_value</replaceable>
+ALTER TYPE <replaceable class="parameter">name</replaceable> SET STORAGE { 
PLAIN | EXTERNAL | EXTENDED | MAIN }
 
 <phrase>where <replaceable class="parameter">action</replaceable> is one 
of:</phrase>
 
@@ -97,6 +98,38 @@ ALTER TYPE <replaceable class="parameter">name</replaceable> 
RENAME VALUE <repla
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term>
+     <literal>SET STORAGE</literal>
+     <indexterm>
+      <primary>TOAST</primary>
+      <secondary>per-type storage settings</secondary>
+     </indexterm>
+    </term>
+    <listitem>
+     <para>
+      This form sets the storage mode for a data type, controlling whether the
+      values are held inline or in a secondary <acronym>TOAST</acronym> table,
+      and whether the data should be compressed or not.
+      <literal>PLAIN</literal> must be used for fixed-length values such as
+      <type>integer</type> and is inline, uncompressed. <literal>MAIN</literal>
+      is for inline, compressible data. <literal>EXTERNAL</literal> is for
+      external, uncompressed data, and <literal>EXTENDED</literal> is for
+      external, compressed data.  <literal>EXTENDED</literal> is the default
+      for most data types that support non-<literal>PLAIN</literal> storage.
+      Use of <literal>EXTERNAL</literal> will make substring operations on
+      very large <type>text</type> and <type>bytea</type> values run faster,
+      at the penalty of increased storage space.  Note that
+      <literal>SET STORAGE</literal> doesn't itself change anything in the
+      tables, it just sets the strategy to be used by tables created in the
+      future. See <xref linkend="storage-toast"/> for more information.
+      Note that this merely modifies the default for a data type, but each
+      attribute specifies it's own strategy that overrides this value.
+      See <xref linkend="sql-altertable"/> for how to change that.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term><literal>SET SCHEMA</literal></term>
     <listitem>
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 1cb84182b0..d021b1d272 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -1042,3 +1042,48 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, 
Oid new_ownerId)
 
        InvokeObjectPostAlterHook(classId, objectId, 0);
 }
+
+/*
+ * Executes an ALTER TYPE / SET STORAGE statement.  At the moment this is
+ * supported only for OBJECT_TYPE nad OBJECT_DOMAIN.
+ */
+ObjectAddress
+ExecAlterTypeStorageStmt(AlterTypeStorageStmt *stmt)
+{
+       char   *storagemode;
+       char    newstorage;
+
+       Assert(IsA(stmt->newstorage, String));
+       storagemode = strVal(stmt->newstorage);
+
+       if (pg_strcasecmp(storagemode, "plain") == 0)
+               newstorage = 'p';
+       else if (pg_strcasecmp(storagemode, "external") == 0)
+               newstorage = 'e';
+       else if (pg_strcasecmp(storagemode, "extended") == 0)
+               newstorage = 'x';
+       else if (pg_strcasecmp(storagemode, "main") == 0)
+               newstorage = 'm';
+       else
+       {
+               ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                errmsg("invalid storage type \"%s\"",
+                                               storagemode)));
+               newstorage = 0;                 /* keep compiler quiet */
+       }
+
+       switch (stmt->objectType)
+       {
+               case OBJECT_TYPE:
+               case OBJECT_DOMAIN:             /* same as TYPE */
+                       return AlterTypeStorage(castNode(List, stmt->object),
+                                                                       
newstorage, stmt->objectType);
+                       break;
+
+               default:
+                       elog(ERROR, "unrecognized AlterTypeStorageStmt type: 
%d",
+                                (int) stmt->objectType);
+                       return InvalidObjectAddress;    /* keep compiler happy 
*/
+       }
+}
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 52097363fd..3909ed6b72 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -41,6 +41,7 @@
 #include "catalog/heap.h"
 #include "catalog/objectaccess.h"
 #include "catalog/pg_am.h"
+#include "catalog/pg_attribute.h"
 #include "catalog/pg_authid.h"
 #include "catalog/pg_collation.h"
 #include "catalog/pg_constraint.h"
@@ -3394,6 +3395,187 @@ AlterTypeOwner(List *names, Oid newOwnerId, ObjectType 
objecttype)
        return address;
 }
 
+static bool
+type_has_toasted_attributes(Oid typeoid)
+{
+       Relation        attrel;
+       SysScanDesc scan;
+       ScanKeyData key[1];
+       HeapTuple       tuple;
+       bool            found = false;  /* no TOAST-ed attributes found */
+
+       /*
+        * Must scan pg_attribute.  Right now, it is a seqscan because there is
+        * no available index on atttypid.
+        */
+
+       attrel = table_open(AttributeRelationId, AccessShareLock);
+
+       /* Use the index to scan only attributes of the target relation */
+       ScanKeyInit(&key[0],
+                               Anum_pg_attribute_atttypid,
+                               BTEqualStrategyNumber, F_OIDEQ,
+                               ObjectIdGetDatum(typeoid));
+
+       scan = systable_beginscan(attrel, InvalidOid, true,
+                                                         NULL, 1, key);
+
+       /* There should be at most one matching tuple, but we loop anyway */
+       while (HeapTupleIsValid(tuple = systable_getnext(scan)))
+       {
+               Form_pg_attribute attr = (Form_pg_attribute) GETSTRUCT(tuple);
+
+               if (attr->attstorage != 'p')
+               {
+                       found = true;
+                       break;
+               }
+       }
+
+       /* Clean up after the scan */
+       systable_endscan(scan);
+       table_close(attrel, AccessShareLock);
+
+       return found;
+}
+
+/*
+ * Change the storage of a type.
+ */
+ObjectAddress
+AlterTypeStorage(List *names, char newStorage, ObjectType objecttype)
+{
+       TypeName   *typename;
+       Oid                     typeOid;
+       Relation        rel;
+       HeapTuple       tup;
+       HeapTuple       newtup;
+       Form_pg_type typTup;
+       ObjectAddress address;
+
+       rel = table_open(TypeRelationId, RowExclusiveLock);
+
+       /* Make a TypeName so we can use standard type lookup machinery */
+       typename = makeTypeNameFromNameList(names);
+
+       /* Use LookupTypeName here so that shell types can be processed */
+       tup = LookupTypeName(NULL, typename, NULL, false);
+       if (tup == NULL)
+               ereport(ERROR,
+                               (errcode(ERRCODE_UNDEFINED_OBJECT),
+                                errmsg("type \"%s\" does not exist",
+                                               TypeNameToString(typename))));
+       typeOid = typeTypeId(tup);
+
+       /* Copy the syscache entry so we can scribble on it below */
+       newtup = heap_copytuple(tup);
+       ReleaseSysCache(tup);
+       tup = newtup;
+       typTup = (Form_pg_type) GETSTRUCT(tup);
+
+       /* Don't allow ALTER DOMAIN on a type */
+       if (objecttype == OBJECT_DOMAIN && typTup->typtype != TYPTYPE_DOMAIN)
+               ereport(ERROR,
+                               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                                errmsg("%s is not a domain",
+                                               format_type_be(typeOid))));
+
+       /*
+        * If it's a composite type, we need to check that it really is a
+        * free-standing composite type, and not a table's rowtype. We want 
people
+        * to use ALTER TABLE not ALTER TYPE for that case.
+        *
+        * XXX Maybe this should be allowed?
+        */
+       if (typTup->typtype == TYPTYPE_COMPOSITE &&
+               get_rel_relkind(typTup->typrelid) != RELKIND_COMPOSITE_TYPE)
+               ereport(ERROR,
+                               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                                errmsg("%s is a table's row type",
+                                               format_type_be(typeOid)),
+                                errhint("Use ALTER TABLE instead.")));
+
+       /*
+        * don't allow direct alteration of array types, either
+        *
+        * XXX Maybe this should be allowed?
+        */
+       if (OidIsValid(typTup->typelem) &&
+               get_array_type(typTup->typelem) == typeOid)
+               ereport(ERROR,
+                               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                                errmsg("cannot alter array type %s",
+                                               format_type_be(typeOid)),
+                                errhint("You can alter type %s, which will 
alter the array type as well.",
+                                                
format_type_be(typTup->typelem))));
+
+       /*
+        * If the new storage is the same as the existing storage, consider the
+        * command to have succeeded.
+        */
+       if (typTup->typstorage != newStorage)
+       {
+               Datum   repl_val[Natts_pg_type];
+               bool    repl_null[Natts_pg_type];
+               bool    repl_repl[Natts_pg_type];
+
+               /* Check the user has right to do ALTER TYPE */
+               if (!pg_type_ownercheck(typTup->oid, GetUserId()))
+                       aclcheck_error_type(ACLCHECK_NOT_OWNER, typTup->oid);
+
+               /*
+                * Verify the transition to the new storage value is allowed 
given the
+                * other type parameters (especially length) and existing 
attributes
+                * using the type.
+                */
+
+               /* Only varlena types can be toasted */
+               if (newStorage != 'p' && typTup->typlen != -1)
+                       ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                                errmsg("fixed-size types must have storage 
PLAIN")));
+
+               /*
+                * We must not allow switching to PLAIN when there are 
attributes with
+                * this type using some other storage value. PLAIN may indicate 
the
+                * type code does not expect the values to be TOAST-ed, so 
allowing
+                * that at the attribute level might lead to crashes.
+                *
+                * Any other transition is correct, because it makes the type 
TOAST
+                * aware. But it's allowed to restrict it at the attribute 
level, or
+                * enable/disable compression.
+                *
+                * So maybe 
+                *
+                * XXX Maybe this could also support CASCADE mode, i.e. we'd 
set the
+                * storage strategy for all attributes using the data type.
+                */
+               if (newStorage == 'p' && type_has_toasted_attributes(typeOid))
+                       ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                                errmsg("can't set storage to PLAIN when there 
are attributes with non-PLAIN storage values")));
+
+               /* do the actual change */
+               memset(repl_null, false, sizeof(repl_null));
+               memset(repl_repl, false, sizeof(repl_repl));
+
+               repl_repl[Anum_pg_type_typstorage - 1] = true;
+               repl_val[Anum_pg_type_typstorage - 1] = 
CharGetDatum(newStorage);
+
+               tup = heap_modify_tuple(tup, RelationGetDescr(rel), repl_val, 
repl_null,
+                                                               repl_repl);
+
+               CatalogTupleUpdate(rel, &tup->t_self, tup);
+       }
+
+       ObjectAddressSet(address, TypeRelationId, typeOid);
+
+       /* Clean up */
+       table_close(rel, RowExclusiveLock);
+
+       return address;
+}
+
 /*
  * AlterTypeOwner_oid - change type owner unconditionally
  *
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 96e7fdbcfe..3249a67eca 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -253,7 +253,7 @@ static Node *makeRecursiveViewSelect(char *relname, List 
*aliases, Node *query);
                AlterTblSpcStmt AlterExtensionStmt AlterExtensionContentsStmt 
AlterForeignTableStmt
                AlterCompositeTypeStmt AlterUserMappingStmt
                AlterRoleStmt AlterRoleSetStmt AlterPolicyStmt AlterStatsStmt
-               AlterDefaultPrivilegesStmt DefACLAction
+               AlterDefaultPrivilegesStmt AlterTypeStorageStmt DefACLAction
                AnalyzeStmt CallStmt ClosePortalStmt ClusterStmt CommentStmt
                ConstraintsSetStmt CopyStmt CreateAsStmt CreateCastStmt
                CreateDomainStmt CreateExtensionStmt CreateGroupStmt 
CreateOpClassStmt
@@ -847,6 +847,7 @@ stmt :
                        | AlterObjectSchemaStmt
                        | AlterOwnerStmt
                        | AlterOperatorStmt
+                       | AlterTypeStorageStmt
                        | AlterPolicyStmt
                        | AlterSeqStmt
                        | AlterSystemStmt
@@ -9566,6 +9567,31 @@ AlterOwnerStmt: ALTER AGGREGATE aggregate_with_argtypes 
OWNER TO RoleSpec
                ;
 
 
+/*****************************************************************************
+ *
+ * ALTER TTYPE name SET STORAGE storage
+ *
+ *****************************************************************************/
+
+AlterTypeStorageStmt:
+                       ALTER TYPE_P any_name SET STORAGE ColId
+                               {
+                                       AlterTypeStorageStmt *n = 
makeNode(AlterTypeStorageStmt);
+                                       n->objectType = OBJECT_TYPE;
+                                       n->object = (Node *) $3;
+                                       n->newstorage = (Node *) makeString($6);
+                                       $$ = (Node *)n;
+                               }
+                       | ALTER DOMAIN_P any_name SET STORAGE ColId
+                               {
+                                       AlterTypeStorageStmt *n = 
makeNode(AlterTypeStorageStmt);
+                                       n->objectType = OBJECT_DOMAIN;
+                                       n->object = (Node *) $3;
+                                       n->newstorage = (Node *) makeString($6);
+                                       $$ = (Node *)n;
+                               }
+               ;
+
 /*****************************************************************************
  *
  * CREATE PUBLICATION name [ FOR TABLE ] [ WITH options ]
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index bb85b5e52a..3f3da70b88 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -158,6 +158,7 @@ ClassifyUtilityCommandAsReadOnly(Node *parsetree)
                case T_AlterSeqStmt:
                case T_AlterStatsStmt:
                case T_AlterSubscriptionStmt:
+               case T_AlterTypeStorageStmt:
                case T_AlterTSConfigurationStmt:
                case T_AlterTSDictionaryStmt:
                case T_AlterTableMoveAllStmt:
@@ -1045,6 +1046,19 @@ standard_ProcessUtility(PlannedStmt *pstmt,
                        }
                        break;
 
+               case T_AlterTypeStorageStmt:
+                       {
+                               AlterTypeStorageStmt *stmt = 
(AlterTypeStorageStmt *) parsetree;
+
+                               if 
(EventTriggerSupportsObjectType(stmt->objectType))
+                                       ProcessUtilitySlow(pstate, pstmt, 
queryString,
+                                                                          
context, params, queryEnv,
+                                                                          
dest, completionTag);
+                               else
+                                       ExecAlterTypeStorageStmt(stmt);
+                       }
+                       break;
+
                case T_CommentStmt:
                        {
                                CommentStmt *stmt = (CommentStmt *) parsetree;
@@ -1723,6 +1737,10 @@ ProcessUtilitySlow(ParseState *pstate,
                                address = AlterOperator((AlterOperatorStmt *) 
parsetree);
                                break;
 
+                       case T_AlterTypeStorageStmt:
+                               address = 
ExecAlterTypeStorageStmt((AlterTypeStorageStmt *) parsetree);
+                               break;
+
                        case T_CommentStmt:
                                address = CommentObject((CommentStmt *) 
parsetree);
                                break;
@@ -2575,6 +2593,10 @@ CreateCommandTag(Node *parsetree)
                        tag = AlterObjectTypeCommandTag(((AlterOwnerStmt *) 
parsetree)->objectType);
                        break;
 
+               case T_AlterTypeStorageStmt:
+                       tag = "ALTER TYPE";
+                       break;
+
                case T_AlterTableMoveAllStmt:
                        tag = AlterObjectTypeCommandTag(((AlterTableMoveAllStmt 
*) parsetree)->objtype);
                        break;
@@ -3264,6 +3286,10 @@ GetCommandLogLevel(Node *parsetree)
                        lev = LOGSTMT_DDL;
                        break;
 
+               case T_AlterTypeStorageStmt:
+                       lev = LOGSTMT_DDL;
+                       break;
+
                case T_AlterTableMoveAllStmt:
                case T_AlterTableStmt:
                        lev = LOGSTMT_DDL;
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index b6b08d0ccb..9a98306b06 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2150,7 +2150,7 @@ psql_completion(const char *text, int start, int end)
        else if (Matches("ALTER", "TYPE", MatchAny))
                COMPLETE_WITH("ADD ATTRIBUTE", "ADD VALUE", "ALTER ATTRIBUTE",
                                          "DROP ATTRIBUTE",
-                                         "OWNER TO", "RENAME", "SET SCHEMA");
+                                         "OWNER TO", "RENAME", "SET SCHEMA", 
"SET STORAGE");
        /* complete ALTER TYPE <foo> ADD with actions */
        else if (Matches("ALTER", "TYPE", MatchAny, "ADD"))
                COMPLETE_WITH("ATTRIBUTE", "VALUE");
diff --git a/src/include/commands/alter.h b/src/include/commands/alter.h
index f9d6ba13c9..c55a1da0af 100644
--- a/src/include/commands/alter.h
+++ b/src/include/commands/alter.h
@@ -31,5 +31,6 @@ extern Oid    AlterObjectNamespace_oid(Oid classId, Oid 
objid, Oid nspOid,
 extern ObjectAddress ExecAlterOwnerStmt(AlterOwnerStmt *stmt);
 extern void AlterObjectOwner_internal(Relation catalog, Oid objectId,
                                                                          Oid 
new_ownerId);
+extern ObjectAddress ExecAlterTypeStorageStmt(AlterTypeStorageStmt *stmt);
 
 #endif                                                 /* ALTER_H */
diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h
index fc18d64347..1f45fc670a 100644
--- a/src/include/commands/typecmds.h
+++ b/src/include/commands/typecmds.h
@@ -54,4 +54,6 @@ extern Oid    AlterTypeNamespaceInternal(Oid typeOid, Oid 
nspOid,
                                                                           bool 
errorOnTableType,
                                                                           
ObjectAddresses *objsMoved);
 
+extern ObjectAddress AlterTypeStorage(List *names, char newStorage, ObjectType 
objecttype);
+
 #endif                                                 /* TYPECMDS_H */
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index baced7eec0..2eb227efdc 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -380,6 +380,7 @@ typedef enum NodeTag
        T_AlterObjectSchemaStmt,
        T_AlterOwnerStmt,
        T_AlterOperatorStmt,
+       T_AlterTypeStorageStmt,
        T_DropOwnedStmt,
        T_ReassignOwnedStmt,
        T_CompositeTypeStmt,
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index da0706add5..4d40acc097 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2959,6 +2959,18 @@ typedef struct AlterOwnerStmt
        RoleSpec   *newowner;           /* the new owner */
 } AlterOwnerStmt;
 
+/* ----------------------
+ *             Alter Type Storage Statement
+ * ----------------------
+ */
+typedef struct AlterTypeStorageStmt
+{
+       NodeTag         type;
+       ObjectType      objectType;             /* OBJECT_TYPE or OBJECT_DOMAIN 
*/
+       Node       *object;
+       Node       *newstorage;         /* the new storage */
+} AlterTypeStorageStmt;
+
 
 /* ----------------------
  *             Alter Operator Set Restrict, Join

Reply via email to