public inbox for [email protected]  

help / color / mirror / Atom feed

CREATE TABLE ( .. STORAGE ..)
15+ messages / 5 participants
[nested] [flat]

* CREATE TABLE ( .. STORAGE ..)
@ 2021-12-27 07:51 Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  0 siblings, 1 reply; 15+ messages in thread

From: Teodor Sigaev @ 2021-12-27 07:51 UTC (permalink / raw)
  To: pgsql-hackers

Hi!

Working on pluggable toaster (mostly, for JSONB improvements, see links 
below) I had found that STORAGE attribute on column is impossible to set 
  in CREATE TABLE command but COMPRESS option is possible. It looks 
unreasonable. Suggested patch implements this possibility.

[1] http://www.sai.msu.su/~megera/postgres/talks/jsonb-pgconfnyc-2021.pdf
[2] http://www.sai.msu.su/~megera/postgres/talks/jsonb-pgvision-2021.pdf
[3] http://www.sai.msu.su/~megera/postgres/talks/jsonb-pgconfonline-2021.pdf
[4] http://www.sai.msu.su/~megera/postgres/talks/bytea-pgconfonline-2021.pdf

PS I will propose pluggable toaster patch a bit later
-- 
Teodor Sigaev                      E-mail: [email protected]
                                       WWW: http://www.sigaev.ru/

Attachments:

  [application/gzip] create_table_storage-v1.patch.gz (3.7K, ../../[email protected]/2-create_table_storage-v1.patch.gz)
  download

^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
@ 2022-03-29 20:28 ` Matthias van de Meent <[email protected]>
  2022-06-15 14:51   ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-06-22 13:29   ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
  0 siblings, 2 replies; 15+ messages in thread

From: Matthias van de Meent @ 2022-03-29 20:28 UTC (permalink / raw)
  To: Teodor Sigaev <[email protected]>; +Cc: wenjing zeng <[email protected]>; pgsql-hackers

On Wed, 2 Feb 2022 at 11:13, Teodor Sigaev <[email protected]> wrote:
>
> Hi!
>
> > Are they both set to name or ColId? Although they are the same.
> >
>
> Thank you, fixed, that was just an oversight.
>
> > 2 For ColumnDef new member storage_name, did you miss the function _copyColumnDef()  _equalColumnDef()?
>
> Thank you, fixed

I noticed this and tried it out after needing it in a different
thread, so this is quite the useful addition.

I see that COMPRESSION and STORAGE now are handled slightly
differently in the grammar. Maybe we could standardize that a bit
more; so that we have only one `STORAGE [kind]` definition in the
grammar?

As I'm new to the grammar files; would you know the difference between
`name` and `ColId`, and why you would change from one to the other in
ALTER COLUMN STORAGE?

Thanks!

-Matthias





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
@ 2022-06-15 14:51   ` Aleksander Alekseev <[email protected]>
  2022-06-16 09:17     ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  2022-06-22 13:25     ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
  1 sibling, 2 replies; 15+ messages in thread

From: Aleksander Alekseev @ 2022-06-15 14:51 UTC (permalink / raw)
  To: pgsql-hackers; +Cc: Teodor Sigaev <[email protected]>; Matthias van de Meent <[email protected]>; wenjing zeng <[email protected]>

Hi hackers,

I noticed that cfbot is not entirely happy with the patch, so I rebased it.

> I see that COMPRESSION and STORAGE now are handled slightly
> differently in the grammar. Maybe we could standardize that a bit
> more; so that we have only one `STORAGE [kind]` definition in the
> grammar?
>
> As I'm new to the grammar files; would you know the difference between
> `name` and `ColId`, and why you would change from one to the other in
> ALTER COLUMN STORAGE?

Good point, Matthias. I addressed this in 0002. Does it look better now?

-- 
Best regards,
Aleksander Alekseev


Attachments:

  [application/octet-stream] v3-0001-CREATE-TABLE-.-STORAGE.patch (14.1K, ../../CAJ7c6TO_QBjMfLkDRgi2ceBZkvMYG_9GRvzboitBfN3PoJoqxQ@mail.gmail.com/2-v3-0001-CREATE-TABLE-.-STORAGE.patch)
  download | inline diff:
From 2c55b70d82d0769d3be2c446ae270e9c78c8759d Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email protected]>
Date: Wed, 15 Jun 2022 16:34:50 +0300
Subject: [PATCH v3 1/2] CREATE TABLE ( .. STORAGE .. )

TODO FIXME description

Author:	Teodor Sigaev <[email protected]>
Reviewed-by: TODO FIXME
Discussion: https://postgr.es/m/[email protected]
---
 doc/src/sgml/ref/create_table.sgml        | 24 +++++++-
 src/backend/commands/tablecmds.c          | 75 +++++++++++++++++------
 src/backend/nodes/copyfuncs.c             |  1 +
 src/backend/nodes/equalfuncs.c            |  1 +
 src/backend/parser/gram.y                 | 18 ++++--
 src/include/nodes/parsenodes.h            |  1 +
 src/test/regress/expected/alter_table.out |  6 +-
 src/test/regress/sql/alter_table.sql      |  5 +-
 8 files changed, 103 insertions(+), 28 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 6c9918b0a1..f1f00cf834 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
  <refsynopsisdiv>
 <synopsis>
 CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name</replaceable> ( [
-  { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [ COMPRESSION <replaceable>compression_method</replaceable> ] [ COLLATE <replaceable>collation</replaceable> ] [ <replaceable class="parameter">column_constraint</replaceable> [ ... ] ]
+  { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [ STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN } ] [ COMPRESSION <replaceable>compression_method</replaceable> ] [ COLLATE <replaceable>collation</replaceable> ] [ <replaceable class="parameter">column_constraint</replaceable> [ ... ] ]
     | <replaceable>table_constraint</replaceable>
     | LIKE <replaceable>source_table</replaceable> [ <replaceable>like_option</replaceable> ... ] }
     [, ... ]
@@ -297,6 +297,28 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term> <literal>SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }</literal></term>
+    <listitem>
+     <para>
+      This form sets the storage mode for a column. This controls whether this
+      column is 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. 
+      See <xref linkend="storage-toast"/> for more information.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term><literal>COMPRESSION <replaceable class="parameter">compression_method</replaceable></literal></term>
     <listitem>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2de0ebacec..708d7354b5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -633,6 +633,7 @@ static void refuseDupeIndexAttach(Relation parentIdx, Relation partIdx,
 static List *GetParentedForeignKeyRefs(Relation partition);
 static void ATDetachCheckNoForeignKeyRefs(Relation partition);
 static char GetAttributeCompression(Oid atttypid, char *compression);
+static char	GetAttributeStorage(const char *storagemode);
 
 
 /* ----------------------------------------------------------------
@@ -931,6 +932,22 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		if (colDef->compression)
 			attr->attcompression = GetAttributeCompression(attr->atttypid,
 														   colDef->compression);
+
+		if (colDef->storage_name)
+		{
+			attr->attstorage = GetAttributeStorage(colDef->storage_name);
+			/*
+			 * safety check: do not allow toasted storage modes unless column datatype
+			 * is TOAST-aware.
+			 */
+			if (!(attr->attstorage == TYPSTORAGE_PLAIN ||
+				  TypeIsToastable(attr->atttypid)))
+				ereport(ERROR,
+						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						 errmsg("column data type %s can only have storage PLAIN",
+								format_type_be(attr->atttypid))));
+		}
+
 	}
 
 	/*
@@ -6819,7 +6836,23 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	attribute.atttypmod = typmod;
 	attribute.attbyval = tform->typbyval;
 	attribute.attalign = tform->typalign;
-	attribute.attstorage = tform->typstorage;
+	if (colDef->storage_name)
+	{
+		attribute.attstorage = GetAttributeStorage(colDef->storage_name);
+		/*
+		 * safety check: do not allow toasted storage modes unless column datatype
+		 * is TOAST-aware.
+		 */
+		if (!(attribute.attstorage == TYPSTORAGE_PLAIN ||
+			  TypeIsToastable(attribute.atttypid)))
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("column data type %s can only have storage PLAIN",
+							format_type_be(attribute.atttypid))));
+	}
+	else
+		attribute.attstorage = tform->typstorage;
+
 	attribute.attcompression = GetAttributeCompression(typeOid,
 													   colDef->compression);
 	attribute.attnotnull = colDef->is_not_null;
@@ -8262,7 +8295,6 @@ SetIndexStorageProperties(Relation rel, Relation attrelation,
 static ObjectAddress
 ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE lockmode)
 {
-	char	   *storagemode;
 	char		newstorage;
 	Relation	attrelation;
 	HeapTuple	tuple;
@@ -8271,24 +8303,8 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc
 	ObjectAddress address;
 
 	Assert(IsA(newValue, String));
-	storagemode = strVal(newValue);
 
-	if (pg_strcasecmp(storagemode, "plain") == 0)
-		newstorage = TYPSTORAGE_PLAIN;
-	else if (pg_strcasecmp(storagemode, "external") == 0)
-		newstorage = TYPSTORAGE_EXTERNAL;
-	else if (pg_strcasecmp(storagemode, "extended") == 0)
-		newstorage = TYPSTORAGE_EXTENDED;
-	else if (pg_strcasecmp(storagemode, "main") == 0)
-		newstorage = TYPSTORAGE_MAIN;
-	else
-	{
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("invalid storage type \"%s\"",
-						storagemode)));
-		newstorage = 0;			/* keep compiler quiet */
-	}
+	newstorage = GetAttributeStorage(strVal(newValue));
 
 	attrelation = table_open(AttributeRelationId, RowExclusiveLock);
 
@@ -19288,3 +19304,24 @@ GetAttributeCompression(Oid atttypid, char *compression)
 
 	return cmethod;
 }
+
+static char
+GetAttributeStorage(const char *storagemode)
+{
+	if (pg_strcasecmp(storagemode, "plain") == 0)
+		return TYPSTORAGE_PLAIN;
+	else if (pg_strcasecmp(storagemode, "external") == 0)
+		return TYPSTORAGE_EXTERNAL;
+	else if (pg_strcasecmp(storagemode, "extended") == 0)
+		return TYPSTORAGE_EXTENDED;
+	else if (pg_strcasecmp(storagemode, "main") == 0)
+		return TYPSTORAGE_MAIN;
+	else
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("invalid storage type \"%s\"",
+						storagemode)));
+		return 0;			/* keep compiler quiet */
+	}
+}
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 51d630fa89..636654b291 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3560,6 +3560,7 @@ _copyColumnDef(const ColumnDef *from)
 	COPY_SCALAR_FIELD(is_not_null);
 	COPY_SCALAR_FIELD(is_from_type);
 	COPY_SCALAR_FIELD(storage);
+	COPY_STRING_FIELD(storage_name);
 	COPY_NODE_FIELD(raw_default);
 	COPY_NODE_FIELD(cooked_default);
 	COPY_SCALAR_FIELD(identity);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index e747e1667d..064943f52d 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -3050,6 +3050,7 @@ _equalColumnDef(const ColumnDef *a, const ColumnDef *b)
 	COMPARE_SCALAR_FIELD(is_not_null);
 	COMPARE_SCALAR_FIELD(is_from_type);
 	COMPARE_SCALAR_FIELD(storage);
+	COMPARE_STRING_FIELD(storage_name);
 	COMPARE_NODE_FIELD(raw_default);
 	COMPARE_NODE_FIELD(cooked_default);
 	COMPARE_SCALAR_FIELD(identity);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 969c9c158f..c88d6d406d 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -595,7 +595,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 
 %type <node>	TableConstraint TableLikeClause
 %type <ival>	TableLikeOptionList TableLikeOption
-%type <str>		column_compression opt_column_compression
+%type <str>		column_compression opt_column_compression opt_column_storage
 %type <list>	ColQualList
 %type <node>	ColConstraint ColConstraintElem ConstraintAttr
 %type <ival>	key_match
@@ -2537,7 +2537,7 @@ alter_table_cmd:
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET STORAGE <storagemode> */
-			| ALTER opt_column ColId SET STORAGE ColId
+			| ALTER opt_column ColId SET STORAGE name
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 
@@ -3778,13 +3778,14 @@ TypedTableElement:
 			| TableConstraint					{ $$ = $1; }
 		;
 
-columnDef:	ColId Typename opt_column_compression create_generic_options ColQualList
+columnDef:	ColId Typename opt_column_storage opt_column_compression create_generic_options ColQualList
 				{
 					ColumnDef *n = makeNode(ColumnDef);
 
 					n->colname = $1;
 					n->typeName = $2;
-					n->compression = $3;
+					n->storage_name = $3;
+					n->compression = $4;
 					n->inhcount = 0;
 					n->is_local = true;
 					n->is_not_null = false;
@@ -3793,8 +3794,8 @@ columnDef:	ColId Typename opt_column_compression create_generic_options ColQualL
 					n->raw_default = NULL;
 					n->cooked_default = NULL;
 					n->collOid = InvalidOid;
-					n->fdwoptions = $4;
-					SplitColQualList($5, &n->constraints, &n->collClause,
+					n->fdwoptions = $5;
+					SplitColQualList($6, &n->constraints, &n->collClause,
 									 yyscanner);
 					n->location = @1;
 					$$ = (Node *) n;
@@ -3851,6 +3852,11 @@ opt_column_compression:
 			| /*EMPTY*/								{ $$ = NULL; }
 		;
 
+opt_column_storage:
+			STORAGE	name							{ $$ = $2; }
+			| /*EMPTY*/								{ $$ = NULL; }
+		;
+
 ColQualList:
 			ColQualList ColConstraint				{ $$ = lappend($1, $2); }
 			| /*EMPTY*/								{ $$ = NIL; }
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 73f635b455..8247edd5a8 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -683,6 +683,7 @@ typedef struct ColumnDef
 	bool		is_not_null;	/* NOT NULL constraint specified? */
 	bool		is_from_type;	/* column definition came from table type */
 	char		storage;		/* attstorage setting, or 0 for default */
+	char		*storage_name;	/* attstorage setting name or NULL for default*/
 	Node	   *raw_default;	/* default value (untransformed parse tree) */
 	Node	   *cooked_default; /* default value (transformed expr tree) */
 	char		identity;		/* attidentity setting */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 5ede56d9b5..fa1ee9e9b6 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2244,7 +2244,7 @@ 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);
+create table test_storage (a text, c text storage plain);
 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
@@ -2256,6 +2256,9 @@ where oid = 'test_storage'::regclass;
  t
 (1 row)
 
+--check STORAGE correctness
+create table test_storage_failed (a text, b int storage extended);
+ERROR:  column data type integer can only have storage PLAIN
 -- 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;
@@ -2264,6 +2267,7 @@ alter table test_storage alter column a set storage external;
  Column |  Type   | Collation | Nullable | Default | Storage  | Stats target | Description 
 --------+---------+-----------+----------+---------+----------+--------------+-------------
  a      | text    |           |          |         | external |              | 
+ c      | text    |           |          |         | plain    |              | 
  b      | integer |           |          | 0       | plain    |              | 
 Indexes:
     "test_storage_idx" btree (b, a)
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 52001e3135..534166501c 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1527,7 +1527,7 @@ 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);
+create table test_storage (a text, c text storage plain);
 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
@@ -1536,6 +1536,9 @@ select reltoastrelid <> 0 as has_toast_table
 from pg_class
 where oid = 'test_storage'::regclass;
 
+--check STORAGE correctness
+create table test_storage_failed (a text, b int storage extended);
+
 -- 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;
-- 
2.36.1



  [application/octet-stream] v3-0002-Handle-SET-STORAGE-and-SET-COMPRESSION-similarly.patch (3.7K, ../../CAJ7c6TO_QBjMfLkDRgi2ceBZkvMYG_9GRvzboitBfN3PoJoqxQ@mail.gmail.com/3-v3-0002-Handle-SET-STORAGE-and-SET-COMPRESSION-similarly.patch)
  download | inline diff:
From 66b6680608044331969f82ba187b50bb33f37767 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email protected]>
Date: Wed, 15 Jun 2022 17:08:34 +0300
Subject: [PATCH v3 2/2] Handle SET STORAGE and SET COMPRESSION similarly

This patch cleans up the code a little in order to make code paths for
SET STORAGE and SET COMPRESSION similar.

Author: Aleksander Alekseev <[email protected]>
Reviewed-by: TODO FIXME
Discussion: https://postgr.es/m/[email protected]
---
 src/backend/commands/tablecmds.c |  9 ++++-----
 src/backend/parser/gram.y        | 12 ++++++++----
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 708d7354b5..ccb9353653 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -593,7 +593,7 @@ static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKM
 static void ATExecGenericOptions(Relation rel, List *options);
 static void ATExecSetRowSecurity(Relation rel, bool rls);
 static void ATExecForceNoForceRowSecurity(Relation rel, bool force_rls);
-static ObjectAddress ATExecSetCompression(AlteredTableInfo *tab, Relation rel,
+static ObjectAddress ATExecSetCompression(Relation rel,
 										  const char *column, Node *newValue, LOCKMODE lockmode);
 
 static void index_copy_data(Relation rel, RelFileNode newrnode);
@@ -4980,8 +4980,8 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
 		case AT_SetStorage:		/* ALTER COLUMN SET STORAGE */
 			address = ATExecSetStorage(rel, cmd->name, cmd->def, lockmode);
 			break;
-		case AT_SetCompression:
-			address = ATExecSetCompression(tab, rel, cmd->name, cmd->def,
+		case AT_SetCompression: /* ALTER COLUMN SET COMPRESSION */
+			address = ATExecSetCompression(rel, cmd->name, cmd->def,
 										   lockmode);
 			break;
 		case AT_DropColumn:		/* DROP COLUMN */
@@ -16172,8 +16172,7 @@ ATExecGenericOptions(Relation rel, List *options)
  * Return value is the address of the modified column
  */
 static ObjectAddress
-ATExecSetCompression(AlteredTableInfo *tab,
-					 Relation rel,
+ATExecSetCompression(Relation rel,
 					 const char *column,
 					 Node *newValue,
 					 LOCKMODE lockmode)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index c88d6d406d..7e7fb70ef2 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -595,7 +595,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 
 %type <node>	TableConstraint TableLikeClause
 %type <ival>	TableLikeOptionList TableLikeOption
-%type <str>		column_compression opt_column_compression opt_column_storage
+%type <str>		column_compression opt_column_compression column_storage opt_column_storage
 %type <list>	ColQualList
 %type <node>	ColConstraint ColConstraintElem ConstraintAttr
 %type <ival>	key_match
@@ -2537,13 +2537,13 @@ alter_table_cmd:
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET STORAGE <storagemode> */
-			| ALTER opt_column ColId SET STORAGE name
+			| ALTER opt_column ColId SET column_storage
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 
 					n->subtype = AT_SetStorage;
 					n->name = $3;
-					n->def = (Node *) makeString($6);
+					n->def = (Node *) makeString($5);
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET COMPRESSION <cm> */
@@ -3852,8 +3852,12 @@ opt_column_compression:
 			| /*EMPTY*/								{ $$ = NULL; }
 		;
 
+column_storage:
+			STORAGE ColId							{ $$ = $2; }
+		;
+
 opt_column_storage:
-			STORAGE	name							{ $$ = $2; }
+			column_storage							{ $$ = $1; }
 			| /*EMPTY*/								{ $$ = NULL; }
 		;
 
-- 
2.36.1



^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  2022-06-15 14:51   ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
@ 2022-06-16 09:17     ` Matthias van de Meent <[email protected]>
  2022-06-16 13:40       ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  1 sibling, 1 reply; 15+ messages in thread

From: Matthias van de Meent @ 2022-06-16 09:17 UTC (permalink / raw)
  To: Aleksander Alekseev <[email protected]>; +Cc: pgsql-hackers; Teodor Sigaev <[email protected]>; wenjing zeng <[email protected]>

On Wed, 15 Jun 2022 at 16:51, Aleksander Alekseev
<[email protected]> wrote:
>
> Hi hackers,
>
> I noticed that cfbot is not entirely happy with the patch, so I rebased it.
>
> > I see that COMPRESSION and STORAGE now are handled slightly
> > differently in the grammar. Maybe we could standardize that a bit
> > more; so that we have only one `STORAGE [kind]` definition in the
> > grammar?
> >
> > As I'm new to the grammar files; would you know the difference between
> > `name` and `ColId`, and why you would change from one to the other in
> > ALTER COLUMN STORAGE?
>
> Good point, Matthias. I addressed this in 0002. Does it look better now?

When updating a patchset generally we try to keep the patches
self-contained, and update patches as opposed to adding incremental
patches to the set.

Apart from this comment on the format of the patch, the result seems solid.

- Matthias





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  2022-06-15 14:51   ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-06-16 09:17     ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
@ 2022-06-16 13:40       ` Aleksander Alekseev <[email protected]>
  2022-06-17 02:45         ` Re: CREATE TABLE ( .. STORAGE ..) Kyotaro Horiguchi <[email protected]>
  0 siblings, 1 reply; 15+ messages in thread

From: Aleksander Alekseev @ 2022-06-16 13:40 UTC (permalink / raw)
  To: Matthias van de Meent <[email protected]>; +Cc: pgsql-hackers; Teodor Sigaev <[email protected]>; wenjing zeng <[email protected]>

Hi Matthias,

> Apart from this comment on the format of the patch, the result seems solid.

Many thanks.

> When updating a patchset generally we try to keep the patches
> self-contained, and update patches as opposed to adding incremental
> patches to the set.

My reasoning was to separate my changes from the ones originally
proposed by Teodor. After doing `git am` locally a reviewer can see
them separately, or together with `git diff origin/master`, whatever
he or she prefers. The committer can choose between committing two
patches ony by one, or rebasing them to a single commit.

I will avoid the "patch for the patch" practice from now on. Sorry for
the inconvenience.

-- 
Best regards,
Aleksander Alekseev





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  2022-06-15 14:51   ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-06-16 09:17     ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  2022-06-16 13:40       ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
@ 2022-06-17 02:45         ` Kyotaro Horiguchi <[email protected]>
  0 siblings, 0 replies; 15+ messages in thread

From: Kyotaro Horiguchi @ 2022-06-17 02:45 UTC (permalink / raw)
  To: [email protected]; +Cc: [email protected]; pgsql-hackers; [email protected]; [email protected]

Thanks! I have been annoyed sometimes by the lack of this feature.

At Thu, 16 Jun 2022 16:40:55 +0300, Aleksander Alekseev <[email protected]> wrote in 
> Hi Matthias,
> 
> > Apart from this comment on the format of the patch, the result seems solid.
> 
> Many thanks.
> 
> > When updating a patchset generally we try to keep the patches
> > self-contained, and update patches as opposed to adding incremental
> > patches to the set.
> 
> My reasoning was to separate my changes from the ones originally
> proposed by Teodor. After doing `git am` locally a reviewer can see
> them separately, or together with `git diff origin/master`, whatever
> he or she prefers. The committer can choose between committing two
> patches ony by one, or rebasing them to a single commit.
> 
> I will avoid the "patch for the patch" practice from now on. Sorry for
> the inconvenience.

0001 contains one tranling whitespace error. (which "git diff --check"
can detect)

The modified doc line gets too long to me. Maybe we should wrap it as
done in other lines of the same page.

I think we should avoid descriptions dead-copied between pages. In
this case, I think we should remove the duplicate part of the
description of ALTER TABLE then replace with something like "See
CREATE TABLE for details".

As the result of copying-in the description, SET-STORAGE and
COMPRESSION in the page of CREATE-TABLE use different articles in the
same context.

> SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
>   This form sets the storage mode for *a* column.

> COMPRESSION compression_method
>   The COMPRESSION clause sets the compression method for *the* column.

FWIW I feel "the" is better here, but anyway we should unify them.

  
 static char GetAttributeCompression(Oid atttypid, char *compression);
+static char	GetAttributeStorage(const char *storagemode);

The whitespace after "char" is TAB which differs from SPC used in
neigbouring lines.

In the grammar, COMPRESSION uses ColId, but STORAGE uses name. It
seems to me the STORAGE is correct here, though.. (So, do we need to
fix COMPRESSION syntax?)


This adds support for "ADD COLUMN SET STORAGE" but it is not described
in the doc.  COMPRESSION is not described, too.  Shouldn't we add the
both this time?  Or the fix for COMPRESSION can be a different patch.

Now that we have three column options COMPRESSION, COLLATE and STORGE
which has the strict order in syntax.  I wonder it can be relaxed but
it might be too much..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  2022-06-15 14:51   ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
@ 2022-06-22 13:25     ` Peter Eisentraut <[email protected]>
  1 sibling, 0 replies; 15+ messages in thread

From: Peter Eisentraut @ 2022-06-22 13:25 UTC (permalink / raw)
  To: Aleksander Alekseev <[email protected]>; pgsql-hackers; +Cc: Teodor Sigaev <[email protected]>; Matthias van de Meent <[email protected]>; wenjing zeng <[email protected]>

On 15.06.22 16:51, Aleksander Alekseev wrote:
> I noticed that cfbot is not entirely happy with the patch, so I rebased it.
> 
>> I see that COMPRESSION and STORAGE now are handled slightly
>> differently in the grammar. Maybe we could standardize that a bit
>> more; so that we have only one `STORAGE [kind]` definition in the
>> grammar?
>>
>> As I'm new to the grammar files; would you know the difference between
>> `name` and `ColId`, and why you would change from one to the other in
>> ALTER COLUMN STORAGE?
> 
> Good point, Matthias. I addressed this in 0002. Does it look better now?

In your patch, the documentation for CREATE TABLE says "SET STORAGE", 
but the actual syntax does not contain "SET".






^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
@ 2022-06-22 13:29   ` Peter Eisentraut <[email protected]>
  2022-06-24 11:44     ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  1 sibling, 1 reply; 15+ messages in thread

From: Peter Eisentraut @ 2022-06-22 13:29 UTC (permalink / raw)
  To: Matthias van de Meent <[email protected]>; Teodor Sigaev <[email protected]>; +Cc: wenjing zeng <[email protected]>; pgsql-hackers

On 29.03.22 22:28, Matthias van de Meent wrote:
> As I'm new to the grammar files; would you know the difference between
> `name` and `ColId`, and why you would change from one to the other in
> ALTER COLUMN STORAGE?

The grammar says

name:       ColId                                   { $$ = $1; };

so it doesn't matter technically.

It seems we are using "name" mostly for names of objects, so I wouldn't 
use it here for storage or compression types.





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  2022-06-22 13:29   ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
@ 2022-06-24 11:44     ` Aleksander Alekseev <[email protected]>
  2022-06-24 12:30       ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  0 siblings, 1 reply; 15+ messages in thread

From: Aleksander Alekseev @ 2022-06-24 11:44 UTC (permalink / raw)
  To: Peter Eisentraut <[email protected]>; +Cc: Matthias van de Meent <[email protected]>; Teodor Sigaev <[email protected]>; wenjing zeng <[email protected]>; pgsql-hackers

Hi hackers,

Many thanks for the review!

Here is a patch updated according to all the recent feedback, except
for two suggestions:

> This adds support for "ADD COLUMN SET STORAGE" but it is not described
> in the doc.  COMPRESSION is not described, too.  Shouldn't we add the
> both this time?  Or the fix for COMPRESSION can be a different patch.

The documentation for ADD COLUMN simply says:

```
 <para>
 This form adds a new column to the table, using the same syntax as
 <link linkend="sql-createtable"><command>CREATE
TABLE</command></link>. If <literal>IF NOT EXISTS</literal>
 is specified and a column already exists with this name,
 no error is thrown.
 </para>
```

I suggest keeping a reference to CREATE TABLE, similarly as it was
done for ALTER COLUMN.

> Now that we have three column options COMPRESSION, COLLATE and STORGE
> which has the strict order in syntax.  I wonder it can be relaxed but
> it might be too much..

Agree, this could be a bit too much for this particular discussion.
Although this shouldn't be a difficult change, and I agree that this
should be useful, personally I don't feel enthusiastic enough to
deliver it right now. I suggest we address this later.

-- 
Best regards,
Aleksander Alekseev


Attachments:

  [application/octet-stream] v4-0001-Allow-specifying-STORAGE-attribute-for-a-new-tabl.patch (19.3K, ../../CAJ7c6TP5sdpRLbzEvu5yDAw2LRB8OXZFo=ENjhq=bouBzSxh9A@mail.gmail.com/2-v4-0001-Allow-specifying-STORAGE-attribute-for-a-new-tabl.patch)
  download | inline diff:
From db62d9027a1eccd3e1314b64ef7071c8422b8416 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email protected]>
Date: Wed, 15 Jun 2022 16:34:50 +0300
Subject: [PATCH v4] Allow specifying STORAGE attribute for a new table.

Also make the code and the documentation for STORAGE and COMPRESSION
attributes consistent.

Author:	Teodor Sigaev <[email protected]>
Author: Aleksander Alekseev <[email protected]>
Reviewed-by: wenjing zeng <[email protected]>
Reviewed-by: Matthias van de Meent <[email protected]>
Reviewed-by: Kyotaro Horiguchi <[email protected]>
Reviewed-by: Peter Eisentraut <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
 doc/src/sgml/ref/alter_table.sgml         | 38 +---------
 doc/src/sgml/ref/create_table.sgml        | 23 ++++++-
 src/backend/commands/tablecmds.c          | 84 ++++++++++++++++-------
 src/backend/nodes/copyfuncs.c             |  1 +
 src/backend/nodes/equalfuncs.c            |  1 +
 src/backend/parser/gram.y                 | 24 +++++--
 src/include/nodes/parsenodes.h            |  1 +
 src/test/regress/expected/alter_table.out |  6 +-
 src/test/regress/sql/alter_table.sql      |  5 +-
 9 files changed, 113 insertions(+), 70 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index a3c62bf056..a7f0aa439e 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -375,22 +375,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     </term>
     <listitem>
      <para>
-      This form sets the storage mode for a column. This controls whether this
-      column is 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 table,
-      it just sets the strategy to be pursued during future table updates.
-      See <xref linkend="storage-toast"/> for more information.
+      See <xref linkend="sql-createtable"/> for details.
      </para>
     </listitem>
    </varlistentry>
@@ -401,26 +386,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     </term>
     <listitem>
      <para>
-      This form sets the compression method for a column, determining how
-      values inserted in future will be compressed (if the storage mode
-      permits compression at all).
-      This does not cause the table to be rewritten, so existing data may still
-      be compressed with other compression methods.  If the table is restored
-      with <application>pg_restore</application>, then all values are rewritten
-      with the configured compression method.
-      However, when data is inserted from another relation (for example,
-      by <command>INSERT ... SELECT</command>), values from the source table are
-      not necessarily detoasted, so any previously compressed data may retain
-      its existing compression method, rather than being recompressed with the
-      compression method of the target column.
-      The supported compression
-      methods are <literal>pglz</literal> and <literal>lz4</literal>.
-      (<literal>lz4</literal> is available only if <option>--with-lz4</option>
-      was used when building <productname>PostgreSQL</productname>.)  In
-      addition, <replaceable class="parameter">compression_method</replaceable>
-      can be <literal>default</literal>, which selects the default behavior of
-      consulting the <xref linkend="guc-default-toast-compression"/> setting
-      at the time of data insertion to determine the method to use.
+      See <xref linkend="sql-createtable"/> for details.
      </para>
     </listitem>
    </varlistentry>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 6c9918b0a1..2e0933ad78 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
  <refsynopsisdiv>
 <synopsis>
 CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name</replaceable> ( [
-  { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [ COMPRESSION <replaceable>compression_method</replaceable> ] [ COLLATE <replaceable>collation</replaceable> ] [ <replaceable class="parameter">column_constraint</replaceable> [ ... ] ]
+  { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [ STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN } ] [ COMPRESSION <replaceable>compression_method</replaceable> ] [ COLLATE <replaceable>collation</replaceable> ] [ <replaceable class="parameter">column_constraint</replaceable> [ ... ] ]
     | <replaceable>table_constraint</replaceable>
     | LIKE <replaceable>source_table</replaceable> [ <replaceable>like_option</replaceable> ... ] }
     [, ... ]
@@ -297,6 +297,27 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term> <literal>STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }</literal></term>
+    <listitem>
+     <para>
+      This form sets the storage mode for the column. This controls whether this
+      column is 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. See <xref linkend="storage-toast"/> for more
+      information.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term><literal>COMPRESSION <replaceable class="parameter">compression_method</replaceable></literal></term>
     <listitem>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2de0ebacec..e91aaf6247 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -593,7 +593,7 @@ static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKM
 static void ATExecGenericOptions(Relation rel, List *options);
 static void ATExecSetRowSecurity(Relation rel, bool rls);
 static void ATExecForceNoForceRowSecurity(Relation rel, bool force_rls);
-static ObjectAddress ATExecSetCompression(AlteredTableInfo *tab, Relation rel,
+static ObjectAddress ATExecSetCompression(Relation rel,
 										  const char *column, Node *newValue, LOCKMODE lockmode);
 
 static void index_copy_data(Relation rel, RelFileNode newrnode);
@@ -633,6 +633,7 @@ static void refuseDupeIndexAttach(Relation parentIdx, Relation partIdx,
 static List *GetParentedForeignKeyRefs(Relation partition);
 static void ATDetachCheckNoForeignKeyRefs(Relation partition);
 static char GetAttributeCompression(Oid atttypid, char *compression);
+static char GetAttributeStorage(const char *storagemode);
 
 
 /* ----------------------------------------------------------------
@@ -931,6 +932,22 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		if (colDef->compression)
 			attr->attcompression = GetAttributeCompression(attr->atttypid,
 														   colDef->compression);
+
+		if (colDef->storage_name)
+		{
+			attr->attstorage = GetAttributeStorage(colDef->storage_name);
+			/*
+			 * safety check: do not allow toasted storage modes unless column datatype
+			 * is TOAST-aware.
+			 */
+			if (!(attr->attstorage == TYPSTORAGE_PLAIN ||
+				  TypeIsToastable(attr->atttypid)))
+				ereport(ERROR,
+						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						 errmsg("column data type %s can only have storage PLAIN",
+								format_type_be(attr->atttypid))));
+		}
+
 	}
 
 	/*
@@ -4963,8 +4980,8 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
 		case AT_SetStorage:		/* ALTER COLUMN SET STORAGE */
 			address = ATExecSetStorage(rel, cmd->name, cmd->def, lockmode);
 			break;
-		case AT_SetCompression:
-			address = ATExecSetCompression(tab, rel, cmd->name, cmd->def,
+		case AT_SetCompression: /* ALTER COLUMN SET COMPRESSION */
+			address = ATExecSetCompression(rel, cmd->name, cmd->def,
 										   lockmode);
 			break;
 		case AT_DropColumn:		/* DROP COLUMN */
@@ -6819,7 +6836,23 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	attribute.atttypmod = typmod;
 	attribute.attbyval = tform->typbyval;
 	attribute.attalign = tform->typalign;
-	attribute.attstorage = tform->typstorage;
+	if (colDef->storage_name)
+	{
+		attribute.attstorage = GetAttributeStorage(colDef->storage_name);
+		/*
+		 * safety check: do not allow toasted storage modes unless column datatype
+		 * is TOAST-aware.
+		 */
+		if (!(attribute.attstorage == TYPSTORAGE_PLAIN ||
+			  TypeIsToastable(attribute.atttypid)))
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("column data type %s can only have storage PLAIN",
+							format_type_be(attribute.atttypid))));
+	}
+	else
+		attribute.attstorage = tform->typstorage;
+
 	attribute.attcompression = GetAttributeCompression(typeOid,
 													   colDef->compression);
 	attribute.attnotnull = colDef->is_not_null;
@@ -8262,7 +8295,6 @@ SetIndexStorageProperties(Relation rel, Relation attrelation,
 static ObjectAddress
 ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE lockmode)
 {
-	char	   *storagemode;
 	char		newstorage;
 	Relation	attrelation;
 	HeapTuple	tuple;
@@ -8271,24 +8303,8 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc
 	ObjectAddress address;
 
 	Assert(IsA(newValue, String));
-	storagemode = strVal(newValue);
 
-	if (pg_strcasecmp(storagemode, "plain") == 0)
-		newstorage = TYPSTORAGE_PLAIN;
-	else if (pg_strcasecmp(storagemode, "external") == 0)
-		newstorage = TYPSTORAGE_EXTERNAL;
-	else if (pg_strcasecmp(storagemode, "extended") == 0)
-		newstorage = TYPSTORAGE_EXTENDED;
-	else if (pg_strcasecmp(storagemode, "main") == 0)
-		newstorage = TYPSTORAGE_MAIN;
-	else
-	{
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("invalid storage type \"%s\"",
-						storagemode)));
-		newstorage = 0;			/* keep compiler quiet */
-	}
+	newstorage = GetAttributeStorage(strVal(newValue));
 
 	attrelation = table_open(AttributeRelationId, RowExclusiveLock);
 
@@ -16156,8 +16172,7 @@ ATExecGenericOptions(Relation rel, List *options)
  * Return value is the address of the modified column
  */
 static ObjectAddress
-ATExecSetCompression(AlteredTableInfo *tab,
-					 Relation rel,
+ATExecSetCompression(Relation rel,
 					 const char *column,
 					 Node *newValue,
 					 LOCKMODE lockmode)
@@ -19288,3 +19303,24 @@ GetAttributeCompression(Oid atttypid, char *compression)
 
 	return cmethod;
 }
+
+static char
+GetAttributeStorage(const char *storagemode)
+{
+	if (pg_strcasecmp(storagemode, "plain") == 0)
+		return TYPSTORAGE_PLAIN;
+	else if (pg_strcasecmp(storagemode, "external") == 0)
+		return TYPSTORAGE_EXTERNAL;
+	else if (pg_strcasecmp(storagemode, "extended") == 0)
+		return TYPSTORAGE_EXTENDED;
+	else if (pg_strcasecmp(storagemode, "main") == 0)
+		return TYPSTORAGE_MAIN;
+	else
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("invalid storage type \"%s\"",
+						storagemode)));
+		return 0;			/* keep compiler quiet */
+	}
+}
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 51d630fa89..636654b291 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3560,6 +3560,7 @@ _copyColumnDef(const ColumnDef *from)
 	COPY_SCALAR_FIELD(is_not_null);
 	COPY_SCALAR_FIELD(is_from_type);
 	COPY_SCALAR_FIELD(storage);
+	COPY_STRING_FIELD(storage_name);
 	COPY_NODE_FIELD(raw_default);
 	COPY_NODE_FIELD(cooked_default);
 	COPY_SCALAR_FIELD(identity);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index e747e1667d..064943f52d 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -3050,6 +3050,7 @@ _equalColumnDef(const ColumnDef *a, const ColumnDef *b)
 	COMPARE_SCALAR_FIELD(is_not_null);
 	COMPARE_SCALAR_FIELD(is_from_type);
 	COMPARE_SCALAR_FIELD(storage);
+	COMPARE_STRING_FIELD(storage_name);
 	COMPARE_NODE_FIELD(raw_default);
 	COMPARE_NODE_FIELD(cooked_default);
 	COMPARE_SCALAR_FIELD(identity);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 969c9c158f..7e7fb70ef2 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -595,7 +595,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 
 %type <node>	TableConstraint TableLikeClause
 %type <ival>	TableLikeOptionList TableLikeOption
-%type <str>		column_compression opt_column_compression
+%type <str>		column_compression opt_column_compression column_storage opt_column_storage
 %type <list>	ColQualList
 %type <node>	ColConstraint ColConstraintElem ConstraintAttr
 %type <ival>	key_match
@@ -2537,13 +2537,13 @@ alter_table_cmd:
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET STORAGE <storagemode> */
-			| ALTER opt_column ColId SET STORAGE ColId
+			| ALTER opt_column ColId SET column_storage
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 
 					n->subtype = AT_SetStorage;
 					n->name = $3;
-					n->def = (Node *) makeString($6);
+					n->def = (Node *) makeString($5);
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET COMPRESSION <cm> */
@@ -3778,13 +3778,14 @@ TypedTableElement:
 			| TableConstraint					{ $$ = $1; }
 		;
 
-columnDef:	ColId Typename opt_column_compression create_generic_options ColQualList
+columnDef:	ColId Typename opt_column_storage opt_column_compression create_generic_options ColQualList
 				{
 					ColumnDef *n = makeNode(ColumnDef);
 
 					n->colname = $1;
 					n->typeName = $2;
-					n->compression = $3;
+					n->storage_name = $3;
+					n->compression = $4;
 					n->inhcount = 0;
 					n->is_local = true;
 					n->is_not_null = false;
@@ -3793,8 +3794,8 @@ columnDef:	ColId Typename opt_column_compression create_generic_options ColQualL
 					n->raw_default = NULL;
 					n->cooked_default = NULL;
 					n->collOid = InvalidOid;
-					n->fdwoptions = $4;
-					SplitColQualList($5, &n->constraints, &n->collClause,
+					n->fdwoptions = $5;
+					SplitColQualList($6, &n->constraints, &n->collClause,
 									 yyscanner);
 					n->location = @1;
 					$$ = (Node *) n;
@@ -3851,6 +3852,15 @@ opt_column_compression:
 			| /*EMPTY*/								{ $$ = NULL; }
 		;
 
+column_storage:
+			STORAGE ColId							{ $$ = $2; }
+		;
+
+opt_column_storage:
+			column_storage							{ $$ = $1; }
+			| /*EMPTY*/								{ $$ = NULL; }
+		;
+
 ColQualList:
 			ColQualList ColConstraint				{ $$ = lappend($1, $2); }
 			| /*EMPTY*/								{ $$ = NIL; }
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 73f635b455..8247edd5a8 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -683,6 +683,7 @@ typedef struct ColumnDef
 	bool		is_not_null;	/* NOT NULL constraint specified? */
 	bool		is_from_type;	/* column definition came from table type */
 	char		storage;		/* attstorage setting, or 0 for default */
+	char		*storage_name;	/* attstorage setting name or NULL for default*/
 	Node	   *raw_default;	/* default value (untransformed parse tree) */
 	Node	   *cooked_default; /* default value (transformed expr tree) */
 	char		identity;		/* attidentity setting */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 5ede56d9b5..fa1ee9e9b6 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2244,7 +2244,7 @@ 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);
+create table test_storage (a text, c text storage plain);
 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
@@ -2256,6 +2256,9 @@ where oid = 'test_storage'::regclass;
  t
 (1 row)
 
+--check STORAGE correctness
+create table test_storage_failed (a text, b int storage extended);
+ERROR:  column data type integer can only have storage PLAIN
 -- 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;
@@ -2264,6 +2267,7 @@ alter table test_storage alter column a set storage external;
  Column |  Type   | Collation | Nullable | Default | Storage  | Stats target | Description 
 --------+---------+-----------+----------+---------+----------+--------------+-------------
  a      | text    |           |          |         | external |              | 
+ c      | text    |           |          |         | plain    |              | 
  b      | integer |           |          | 0       | plain    |              | 
 Indexes:
     "test_storage_idx" btree (b, a)
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 52001e3135..534166501c 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1527,7 +1527,7 @@ 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);
+create table test_storage (a text, c text storage plain);
 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
@@ -1536,6 +1536,9 @@ select reltoastrelid <> 0 as has_toast_table
 from pg_class
 where oid = 'test_storage'::regclass;
 
+--check STORAGE correctness
+create table test_storage_failed (a text, b int storage extended);
+
 -- 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;
-- 
2.36.1



^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  2022-06-22 13:29   ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
  2022-06-24 11:44     ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
@ 2022-06-24 12:30       ` Aleksander Alekseev <[email protected]>
  2022-07-11 09:27         ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  0 siblings, 1 reply; 15+ messages in thread

From: Aleksander Alekseev @ 2022-06-24 12:30 UTC (permalink / raw)
  To: pgsql-hackers; +Cc: Matthias van de Meent <[email protected]>; Peter Eisentraut <[email protected]>; Teodor Sigaev <[email protected]>; wenjing zeng <[email protected]>

Hi hackers,

> Here is a patch updated according to all the recent feedback, except
> for two suggestions:

In v4 I forgot to list possible arguments for STORAGE in
alter_table.sgml, similarly as it is done for other subcommands. Here
is a corrected patch.

- <literal>SET STORAGE</literal>
+ <literal>SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }</literal>

-- 
Best regards,
Aleksander Alekseev


Attachments:

  [application/octet-stream] v5-0001-Allow-specifying-STORAGE-attribute-for-a-new-tabl.patch (19.6K, ../../CAJ7c6TM-O3=duY8Zk3S4Joi6s=wuLfePFTFuBgNu65bVZY+PdQ@mail.gmail.com/2-v5-0001-Allow-specifying-STORAGE-attribute-for-a-new-tabl.patch)
  download | inline diff:
From 4ca418f3249ff386e4482a7861f90945dfb1e724 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email protected]>
Date: Wed, 15 Jun 2022 16:34:50 +0300
Subject: [PATCH v5] Allow specifying STORAGE attribute for a new table.

Also make the code and the documentation for STORAGE and COMPRESSION
attributes consistent.

Author:	Teodor Sigaev <[email protected]>
Author: Aleksander Alekseev <[email protected]>
Reviewed-by: wenjing zeng <[email protected]>
Reviewed-by: Matthias van de Meent <[email protected]>
Reviewed-by: Kyotaro Horiguchi <[email protected]>
Reviewed-by: Peter Eisentraut <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
 doc/src/sgml/ref/alter_table.sgml         | 40 +----------
 doc/src/sgml/ref/create_table.sgml        | 23 ++++++-
 src/backend/commands/tablecmds.c          | 84 ++++++++++++++++-------
 src/backend/nodes/copyfuncs.c             |  1 +
 src/backend/nodes/equalfuncs.c            |  1 +
 src/backend/parser/gram.y                 | 24 +++++--
 src/include/nodes/parsenodes.h            |  1 +
 src/test/regress/expected/alter_table.out |  6 +-
 src/test/regress/sql/alter_table.sql      |  5 +-
 9 files changed, 114 insertions(+), 71 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index a3c62bf056..e83837088a 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -367,7 +367,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
 
    <varlistentry>
     <term>
-     <literal>SET STORAGE</literal>
+     <literal>SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }</literal>
      <indexterm>
       <primary>TOAST</primary>
       <secondary>per-column storage settings</secondary>
@@ -375,22 +375,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     </term>
     <listitem>
      <para>
-      This form sets the storage mode for a column. This controls whether this
-      column is 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 table,
-      it just sets the strategy to be pursued during future table updates.
-      See <xref linkend="storage-toast"/> for more information.
+      See <xref linkend="sql-createtable"/> for details.
      </para>
     </listitem>
    </varlistentry>
@@ -401,26 +386,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     </term>
     <listitem>
      <para>
-      This form sets the compression method for a column, determining how
-      values inserted in future will be compressed (if the storage mode
-      permits compression at all).
-      This does not cause the table to be rewritten, so existing data may still
-      be compressed with other compression methods.  If the table is restored
-      with <application>pg_restore</application>, then all values are rewritten
-      with the configured compression method.
-      However, when data is inserted from another relation (for example,
-      by <command>INSERT ... SELECT</command>), values from the source table are
-      not necessarily detoasted, so any previously compressed data may retain
-      its existing compression method, rather than being recompressed with the
-      compression method of the target column.
-      The supported compression
-      methods are <literal>pglz</literal> and <literal>lz4</literal>.
-      (<literal>lz4</literal> is available only if <option>--with-lz4</option>
-      was used when building <productname>PostgreSQL</productname>.)  In
-      addition, <replaceable class="parameter">compression_method</replaceable>
-      can be <literal>default</literal>, which selects the default behavior of
-      consulting the <xref linkend="guc-default-toast-compression"/> setting
-      at the time of data insertion to determine the method to use.
+      See <xref linkend="sql-createtable"/> for details.
      </para>
     </listitem>
    </varlistentry>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 6c9918b0a1..2e0933ad78 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
  <refsynopsisdiv>
 <synopsis>
 CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name</replaceable> ( [
-  { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [ COMPRESSION <replaceable>compression_method</replaceable> ] [ COLLATE <replaceable>collation</replaceable> ] [ <replaceable class="parameter">column_constraint</replaceable> [ ... ] ]
+  { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [ STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN } ] [ COMPRESSION <replaceable>compression_method</replaceable> ] [ COLLATE <replaceable>collation</replaceable> ] [ <replaceable class="parameter">column_constraint</replaceable> [ ... ] ]
     | <replaceable>table_constraint</replaceable>
     | LIKE <replaceable>source_table</replaceable> [ <replaceable>like_option</replaceable> ... ] }
     [, ... ]
@@ -297,6 +297,27 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term> <literal>STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }</literal></term>
+    <listitem>
+     <para>
+      This form sets the storage mode for the column. This controls whether this
+      column is 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. See <xref linkend="storage-toast"/> for more
+      information.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term><literal>COMPRESSION <replaceable class="parameter">compression_method</replaceable></literal></term>
     <listitem>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2de0ebacec..e91aaf6247 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -593,7 +593,7 @@ static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKM
 static void ATExecGenericOptions(Relation rel, List *options);
 static void ATExecSetRowSecurity(Relation rel, bool rls);
 static void ATExecForceNoForceRowSecurity(Relation rel, bool force_rls);
-static ObjectAddress ATExecSetCompression(AlteredTableInfo *tab, Relation rel,
+static ObjectAddress ATExecSetCompression(Relation rel,
 										  const char *column, Node *newValue, LOCKMODE lockmode);
 
 static void index_copy_data(Relation rel, RelFileNode newrnode);
@@ -633,6 +633,7 @@ static void refuseDupeIndexAttach(Relation parentIdx, Relation partIdx,
 static List *GetParentedForeignKeyRefs(Relation partition);
 static void ATDetachCheckNoForeignKeyRefs(Relation partition);
 static char GetAttributeCompression(Oid atttypid, char *compression);
+static char GetAttributeStorage(const char *storagemode);
 
 
 /* ----------------------------------------------------------------
@@ -931,6 +932,22 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		if (colDef->compression)
 			attr->attcompression = GetAttributeCompression(attr->atttypid,
 														   colDef->compression);
+
+		if (colDef->storage_name)
+		{
+			attr->attstorage = GetAttributeStorage(colDef->storage_name);
+			/*
+			 * safety check: do not allow toasted storage modes unless column datatype
+			 * is TOAST-aware.
+			 */
+			if (!(attr->attstorage == TYPSTORAGE_PLAIN ||
+				  TypeIsToastable(attr->atttypid)))
+				ereport(ERROR,
+						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						 errmsg("column data type %s can only have storage PLAIN",
+								format_type_be(attr->atttypid))));
+		}
+
 	}
 
 	/*
@@ -4963,8 +4980,8 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
 		case AT_SetStorage:		/* ALTER COLUMN SET STORAGE */
 			address = ATExecSetStorage(rel, cmd->name, cmd->def, lockmode);
 			break;
-		case AT_SetCompression:
-			address = ATExecSetCompression(tab, rel, cmd->name, cmd->def,
+		case AT_SetCompression: /* ALTER COLUMN SET COMPRESSION */
+			address = ATExecSetCompression(rel, cmd->name, cmd->def,
 										   lockmode);
 			break;
 		case AT_DropColumn:		/* DROP COLUMN */
@@ -6819,7 +6836,23 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	attribute.atttypmod = typmod;
 	attribute.attbyval = tform->typbyval;
 	attribute.attalign = tform->typalign;
-	attribute.attstorage = tform->typstorage;
+	if (colDef->storage_name)
+	{
+		attribute.attstorage = GetAttributeStorage(colDef->storage_name);
+		/*
+		 * safety check: do not allow toasted storage modes unless column datatype
+		 * is TOAST-aware.
+		 */
+		if (!(attribute.attstorage == TYPSTORAGE_PLAIN ||
+			  TypeIsToastable(attribute.atttypid)))
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("column data type %s can only have storage PLAIN",
+							format_type_be(attribute.atttypid))));
+	}
+	else
+		attribute.attstorage = tform->typstorage;
+
 	attribute.attcompression = GetAttributeCompression(typeOid,
 													   colDef->compression);
 	attribute.attnotnull = colDef->is_not_null;
@@ -8262,7 +8295,6 @@ SetIndexStorageProperties(Relation rel, Relation attrelation,
 static ObjectAddress
 ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE lockmode)
 {
-	char	   *storagemode;
 	char		newstorage;
 	Relation	attrelation;
 	HeapTuple	tuple;
@@ -8271,24 +8303,8 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc
 	ObjectAddress address;
 
 	Assert(IsA(newValue, String));
-	storagemode = strVal(newValue);
 
-	if (pg_strcasecmp(storagemode, "plain") == 0)
-		newstorage = TYPSTORAGE_PLAIN;
-	else if (pg_strcasecmp(storagemode, "external") == 0)
-		newstorage = TYPSTORAGE_EXTERNAL;
-	else if (pg_strcasecmp(storagemode, "extended") == 0)
-		newstorage = TYPSTORAGE_EXTENDED;
-	else if (pg_strcasecmp(storagemode, "main") == 0)
-		newstorage = TYPSTORAGE_MAIN;
-	else
-	{
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("invalid storage type \"%s\"",
-						storagemode)));
-		newstorage = 0;			/* keep compiler quiet */
-	}
+	newstorage = GetAttributeStorage(strVal(newValue));
 
 	attrelation = table_open(AttributeRelationId, RowExclusiveLock);
 
@@ -16156,8 +16172,7 @@ ATExecGenericOptions(Relation rel, List *options)
  * Return value is the address of the modified column
  */
 static ObjectAddress
-ATExecSetCompression(AlteredTableInfo *tab,
-					 Relation rel,
+ATExecSetCompression(Relation rel,
 					 const char *column,
 					 Node *newValue,
 					 LOCKMODE lockmode)
@@ -19288,3 +19303,24 @@ GetAttributeCompression(Oid atttypid, char *compression)
 
 	return cmethod;
 }
+
+static char
+GetAttributeStorage(const char *storagemode)
+{
+	if (pg_strcasecmp(storagemode, "plain") == 0)
+		return TYPSTORAGE_PLAIN;
+	else if (pg_strcasecmp(storagemode, "external") == 0)
+		return TYPSTORAGE_EXTERNAL;
+	else if (pg_strcasecmp(storagemode, "extended") == 0)
+		return TYPSTORAGE_EXTENDED;
+	else if (pg_strcasecmp(storagemode, "main") == 0)
+		return TYPSTORAGE_MAIN;
+	else
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("invalid storage type \"%s\"",
+						storagemode)));
+		return 0;			/* keep compiler quiet */
+	}
+}
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 51d630fa89..636654b291 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3560,6 +3560,7 @@ _copyColumnDef(const ColumnDef *from)
 	COPY_SCALAR_FIELD(is_not_null);
 	COPY_SCALAR_FIELD(is_from_type);
 	COPY_SCALAR_FIELD(storage);
+	COPY_STRING_FIELD(storage_name);
 	COPY_NODE_FIELD(raw_default);
 	COPY_NODE_FIELD(cooked_default);
 	COPY_SCALAR_FIELD(identity);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index e747e1667d..064943f52d 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -3050,6 +3050,7 @@ _equalColumnDef(const ColumnDef *a, const ColumnDef *b)
 	COMPARE_SCALAR_FIELD(is_not_null);
 	COMPARE_SCALAR_FIELD(is_from_type);
 	COMPARE_SCALAR_FIELD(storage);
+	COMPARE_STRING_FIELD(storage_name);
 	COMPARE_NODE_FIELD(raw_default);
 	COMPARE_NODE_FIELD(cooked_default);
 	COMPARE_SCALAR_FIELD(identity);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 969c9c158f..7e7fb70ef2 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -595,7 +595,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 
 %type <node>	TableConstraint TableLikeClause
 %type <ival>	TableLikeOptionList TableLikeOption
-%type <str>		column_compression opt_column_compression
+%type <str>		column_compression opt_column_compression column_storage opt_column_storage
 %type <list>	ColQualList
 %type <node>	ColConstraint ColConstraintElem ConstraintAttr
 %type <ival>	key_match
@@ -2537,13 +2537,13 @@ alter_table_cmd:
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET STORAGE <storagemode> */
-			| ALTER opt_column ColId SET STORAGE ColId
+			| ALTER opt_column ColId SET column_storage
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 
 					n->subtype = AT_SetStorage;
 					n->name = $3;
-					n->def = (Node *) makeString($6);
+					n->def = (Node *) makeString($5);
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET COMPRESSION <cm> */
@@ -3778,13 +3778,14 @@ TypedTableElement:
 			| TableConstraint					{ $$ = $1; }
 		;
 
-columnDef:	ColId Typename opt_column_compression create_generic_options ColQualList
+columnDef:	ColId Typename opt_column_storage opt_column_compression create_generic_options ColQualList
 				{
 					ColumnDef *n = makeNode(ColumnDef);
 
 					n->colname = $1;
 					n->typeName = $2;
-					n->compression = $3;
+					n->storage_name = $3;
+					n->compression = $4;
 					n->inhcount = 0;
 					n->is_local = true;
 					n->is_not_null = false;
@@ -3793,8 +3794,8 @@ columnDef:	ColId Typename opt_column_compression create_generic_options ColQualL
 					n->raw_default = NULL;
 					n->cooked_default = NULL;
 					n->collOid = InvalidOid;
-					n->fdwoptions = $4;
-					SplitColQualList($5, &n->constraints, &n->collClause,
+					n->fdwoptions = $5;
+					SplitColQualList($6, &n->constraints, &n->collClause,
 									 yyscanner);
 					n->location = @1;
 					$$ = (Node *) n;
@@ -3851,6 +3852,15 @@ opt_column_compression:
 			| /*EMPTY*/								{ $$ = NULL; }
 		;
 
+column_storage:
+			STORAGE ColId							{ $$ = $2; }
+		;
+
+opt_column_storage:
+			column_storage							{ $$ = $1; }
+			| /*EMPTY*/								{ $$ = NULL; }
+		;
+
 ColQualList:
 			ColQualList ColConstraint				{ $$ = lappend($1, $2); }
 			| /*EMPTY*/								{ $$ = NIL; }
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 73f635b455..8247edd5a8 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -683,6 +683,7 @@ typedef struct ColumnDef
 	bool		is_not_null;	/* NOT NULL constraint specified? */
 	bool		is_from_type;	/* column definition came from table type */
 	char		storage;		/* attstorage setting, or 0 for default */
+	char		*storage_name;	/* attstorage setting name or NULL for default*/
 	Node	   *raw_default;	/* default value (untransformed parse tree) */
 	Node	   *cooked_default; /* default value (transformed expr tree) */
 	char		identity;		/* attidentity setting */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 5ede56d9b5..fa1ee9e9b6 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2244,7 +2244,7 @@ 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);
+create table test_storage (a text, c text storage plain);
 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
@@ -2256,6 +2256,9 @@ where oid = 'test_storage'::regclass;
  t
 (1 row)
 
+--check STORAGE correctness
+create table test_storage_failed (a text, b int storage extended);
+ERROR:  column data type integer can only have storage PLAIN
 -- 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;
@@ -2264,6 +2267,7 @@ alter table test_storage alter column a set storage external;
  Column |  Type   | Collation | Nullable | Default | Storage  | Stats target | Description 
 --------+---------+-----------+----------+---------+----------+--------------+-------------
  a      | text    |           |          |         | external |              | 
+ c      | text    |           |          |         | plain    |              | 
  b      | integer |           |          | 0       | plain    |              | 
 Indexes:
     "test_storage_idx" btree (b, a)
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 52001e3135..534166501c 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1527,7 +1527,7 @@ 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);
+create table test_storage (a text, c text storage plain);
 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
@@ -1536,6 +1536,9 @@ select reltoastrelid <> 0 as has_toast_table
 from pg_class
 where oid = 'test_storage'::regclass;
 
+--check STORAGE correctness
+create table test_storage_failed (a text, b int storage extended);
+
 -- 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;
-- 
2.36.1



^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  2022-06-22 13:29   ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
  2022-06-24 11:44     ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-06-24 12:30       ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
@ 2022-07-11 09:27         ` Aleksander Alekseev <[email protected]>
  2022-07-11 13:17           ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
  0 siblings, 1 reply; 15+ messages in thread

From: Aleksander Alekseev @ 2022-07-11 09:27 UTC (permalink / raw)
  To: pgsql-hackers; +Cc: Matthias van de Meent <[email protected]>; Peter Eisentraut <[email protected]>; Teodor Sigaev <[email protected]>; wenjing zeng <[email protected]>

Hi hackers,

> > Here is a patch updated according to all the recent feedback, except
> > for two suggestions:
>
> In v4 I forgot to list possible arguments for STORAGE in
> alter_table.sgml, similarly as it is done for other subcommands. Here
> is a corrected patch.

Here is the rebased patch.

-- 
Best regards,
Aleksander Alekseev


Attachments:

  [application/octet-stream] v6-0001-Allow-specifying-STORAGE-attribute-for-a-new-tabl.patch (18.6K, ../../CAJ7c6TOn1BrFAxWxK7zMfsGyGNqg3YWsou9DpvMp1=JX80909A@mail.gmail.com/2-v6-0001-Allow-specifying-STORAGE-attribute-for-a-new-tabl.patch)
  download | inline diff:
From 66a21bffdd6772d122977aab4cb3d47f117154b2 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email protected]>
Date: Wed, 15 Jun 2022 16:34:50 +0300
Subject: [PATCH v6] Allow specifying STORAGE attribute for a new table.

Also make the code and the documentation for STORAGE and COMPRESSION
attributes consistent.

Author:	Teodor Sigaev <[email protected]>
Author: Aleksander Alekseev <[email protected]>
Reviewed-by: wenjing zeng <[email protected]>
Reviewed-by: Matthias van de Meent <[email protected]>
Reviewed-by: Kyotaro Horiguchi <[email protected]>
Reviewed-by: Peter Eisentraut <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
 doc/src/sgml/ref/alter_table.sgml         | 40 +----------
 doc/src/sgml/ref/create_table.sgml        | 23 ++++++-
 src/backend/commands/tablecmds.c          | 84 ++++++++++++++++-------
 src/backend/parser/gram.y                 | 24 +++++--
 src/include/nodes/parsenodes.h            |  1 +
 src/test/regress/expected/alter_table.out |  6 +-
 src/test/regress/sql/alter_table.sql      |  5 +-
 7 files changed, 112 insertions(+), 71 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index a3c62bf056..e83837088a 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -367,7 +367,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
 
    <varlistentry>
     <term>
-     <literal>SET STORAGE</literal>
+     <literal>SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }</literal>
      <indexterm>
       <primary>TOAST</primary>
       <secondary>per-column storage settings</secondary>
@@ -375,22 +375,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     </term>
     <listitem>
      <para>
-      This form sets the storage mode for a column. This controls whether this
-      column is 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 table,
-      it just sets the strategy to be pursued during future table updates.
-      See <xref linkend="storage-toast"/> for more information.
+      See <xref linkend="sql-createtable"/> for details.
      </para>
     </listitem>
    </varlistentry>
@@ -401,26 +386,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     </term>
     <listitem>
      <para>
-      This form sets the compression method for a column, determining how
-      values inserted in future will be compressed (if the storage mode
-      permits compression at all).
-      This does not cause the table to be rewritten, so existing data may still
-      be compressed with other compression methods.  If the table is restored
-      with <application>pg_restore</application>, then all values are rewritten
-      with the configured compression method.
-      However, when data is inserted from another relation (for example,
-      by <command>INSERT ... SELECT</command>), values from the source table are
-      not necessarily detoasted, so any previously compressed data may retain
-      its existing compression method, rather than being recompressed with the
-      compression method of the target column.
-      The supported compression
-      methods are <literal>pglz</literal> and <literal>lz4</literal>.
-      (<literal>lz4</literal> is available only if <option>--with-lz4</option>
-      was used when building <productname>PostgreSQL</productname>.)  In
-      addition, <replaceable class="parameter">compression_method</replaceable>
-      can be <literal>default</literal>, which selects the default behavior of
-      consulting the <xref linkend="guc-default-toast-compression"/> setting
-      at the time of data insertion to determine the method to use.
+      See <xref linkend="sql-createtable"/> for details.
      </para>
     </listitem>
    </varlistentry>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 6c9918b0a1..da90698046 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
  <refsynopsisdiv>
 <synopsis>
 CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name</replaceable> ( [
-  { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [ COMPRESSION <replaceable>compression_method</replaceable> ] [ COLLATE <replaceable>collation</replaceable> ] [ <replaceable class="parameter">column_constraint</replaceable> [ ... ] ]
+  { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [ STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN } ] [ COMPRESSION <replaceable>compression_method</replaceable> ] [ COLLATE <replaceable>collation</replaceable> ] [ <replaceable class="parameter">column_constraint</replaceable> [ ... ] ]
     | <replaceable>table_constraint</replaceable>
     | LIKE <replaceable>source_table</replaceable> [ <replaceable>like_option</replaceable> ... ] }
     [, ... ]
@@ -297,6 +297,27 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term> <literal>STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }</literal></term>
+    <listitem>
+     <para>
+      This form sets the storage mode for the column. This controls whether this
+      column is 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. See <xref linkend="storage-toast"/> for more
+      information.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term><literal>COMPRESSION <replaceable class="parameter">compression_method</replaceable></literal></term>
     <listitem>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ef5b34a312..b1a46cdf2a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -593,7 +593,7 @@ static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKM
 static void ATExecGenericOptions(Relation rel, List *options);
 static void ATExecSetRowSecurity(Relation rel, bool rls);
 static void ATExecForceNoForceRowSecurity(Relation rel, bool force_rls);
-static ObjectAddress ATExecSetCompression(AlteredTableInfo *tab, Relation rel,
+static ObjectAddress ATExecSetCompression(Relation rel,
 										  const char *column, Node *newValue, LOCKMODE lockmode);
 
 static void index_copy_data(Relation rel, RelFileLocator newrlocator);
@@ -633,6 +633,7 @@ static void refuseDupeIndexAttach(Relation parentIdx, Relation partIdx,
 static List *GetParentedForeignKeyRefs(Relation partition);
 static void ATDetachCheckNoForeignKeyRefs(Relation partition);
 static char GetAttributeCompression(Oid atttypid, char *compression);
+static char GetAttributeStorage(const char *storagemode);
 
 
 /* ----------------------------------------------------------------
@@ -931,6 +932,22 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		if (colDef->compression)
 			attr->attcompression = GetAttributeCompression(attr->atttypid,
 														   colDef->compression);
+
+		if (colDef->storage_name)
+		{
+			attr->attstorage = GetAttributeStorage(colDef->storage_name);
+			/*
+			 * safety check: do not allow toasted storage modes unless column datatype
+			 * is TOAST-aware.
+			 */
+			if (!(attr->attstorage == TYPSTORAGE_PLAIN ||
+				  TypeIsToastable(attr->atttypid)))
+				ereport(ERROR,
+						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						 errmsg("column data type %s can only have storage PLAIN",
+								format_type_be(attr->atttypid))));
+		}
+
 	}
 
 	/*
@@ -4963,8 +4980,8 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
 		case AT_SetStorage:		/* ALTER COLUMN SET STORAGE */
 			address = ATExecSetStorage(rel, cmd->name, cmd->def, lockmode);
 			break;
-		case AT_SetCompression:
-			address = ATExecSetCompression(tab, rel, cmd->name, cmd->def,
+		case AT_SetCompression: /* ALTER COLUMN SET COMPRESSION */
+			address = ATExecSetCompression(rel, cmd->name, cmd->def,
 										   lockmode);
 			break;
 		case AT_DropColumn:		/* DROP COLUMN */
@@ -6820,7 +6837,23 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	attribute.atttypmod = typmod;
 	attribute.attbyval = tform->typbyval;
 	attribute.attalign = tform->typalign;
-	attribute.attstorage = tform->typstorage;
+	if (colDef->storage_name)
+	{
+		attribute.attstorage = GetAttributeStorage(colDef->storage_name);
+		/*
+		 * safety check: do not allow toasted storage modes unless column datatype
+		 * is TOAST-aware.
+		 */
+		if (!(attribute.attstorage == TYPSTORAGE_PLAIN ||
+			  TypeIsToastable(attribute.atttypid)))
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("column data type %s can only have storage PLAIN",
+							format_type_be(attribute.atttypid))));
+	}
+	else
+		attribute.attstorage = tform->typstorage;
+
 	attribute.attcompression = GetAttributeCompression(typeOid,
 													   colDef->compression);
 	attribute.attnotnull = colDef->is_not_null;
@@ -8263,7 +8296,6 @@ SetIndexStorageProperties(Relation rel, Relation attrelation,
 static ObjectAddress
 ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE lockmode)
 {
-	char	   *storagemode;
 	char		newstorage;
 	Relation	attrelation;
 	HeapTuple	tuple;
@@ -8272,24 +8304,8 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc
 	ObjectAddress address;
 
 	Assert(IsA(newValue, String));
-	storagemode = strVal(newValue);
 
-	if (pg_strcasecmp(storagemode, "plain") == 0)
-		newstorage = TYPSTORAGE_PLAIN;
-	else if (pg_strcasecmp(storagemode, "external") == 0)
-		newstorage = TYPSTORAGE_EXTERNAL;
-	else if (pg_strcasecmp(storagemode, "extended") == 0)
-		newstorage = TYPSTORAGE_EXTENDED;
-	else if (pg_strcasecmp(storagemode, "main") == 0)
-		newstorage = TYPSTORAGE_MAIN;
-	else
-	{
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("invalid storage type \"%s\"",
-						storagemode)));
-		newstorage = 0;			/* keep compiler quiet */
-	}
+	newstorage = GetAttributeStorage(strVal(newValue));
 
 	attrelation = table_open(AttributeRelationId, RowExclusiveLock);
 
@@ -16157,8 +16173,7 @@ ATExecGenericOptions(Relation rel, List *options)
  * Return value is the address of the modified column
  */
 static ObjectAddress
-ATExecSetCompression(AlteredTableInfo *tab,
-					 Relation rel,
+ATExecSetCompression(Relation rel,
 					 const char *column,
 					 Node *newValue,
 					 LOCKMODE lockmode)
@@ -19289,3 +19304,24 @@ GetAttributeCompression(Oid atttypid, char *compression)
 
 	return cmethod;
 }
+
+static char
+GetAttributeStorage(const char *storagemode)
+{
+	if (pg_strcasecmp(storagemode, "plain") == 0)
+		return TYPSTORAGE_PLAIN;
+	else if (pg_strcasecmp(storagemode, "external") == 0)
+		return TYPSTORAGE_EXTERNAL;
+	else if (pg_strcasecmp(storagemode, "extended") == 0)
+		return TYPSTORAGE_EXTENDED;
+	else if (pg_strcasecmp(storagemode, "main") == 0)
+		return TYPSTORAGE_MAIN;
+	else
+	{
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("invalid storage type \"%s\"",
+						storagemode)));
+		return 0;			/* keep compiler quiet */
+	}
+}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 0523013f53..c018140afe 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -595,7 +595,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 
 %type <node>	TableConstraint TableLikeClause
 %type <ival>	TableLikeOptionList TableLikeOption
-%type <str>		column_compression opt_column_compression
+%type <str>		column_compression opt_column_compression column_storage opt_column_storage
 %type <list>	ColQualList
 %type <node>	ColConstraint ColConstraintElem ConstraintAttr
 %type <ival>	key_match
@@ -2537,13 +2537,13 @@ alter_table_cmd:
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET STORAGE <storagemode> */
-			| ALTER opt_column ColId SET STORAGE ColId
+			| ALTER opt_column ColId SET column_storage
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 
 					n->subtype = AT_SetStorage;
 					n->name = $3;
-					n->def = (Node *) makeString($6);
+					n->def = (Node *) makeString($5);
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET COMPRESSION <cm> */
@@ -3778,13 +3778,14 @@ TypedTableElement:
 			| TableConstraint					{ $$ = $1; }
 		;
 
-columnDef:	ColId Typename opt_column_compression create_generic_options ColQualList
+columnDef:	ColId Typename opt_column_storage opt_column_compression create_generic_options ColQualList
 				{
 					ColumnDef *n = makeNode(ColumnDef);
 
 					n->colname = $1;
 					n->typeName = $2;
-					n->compression = $3;
+					n->storage_name = $3;
+					n->compression = $4;
 					n->inhcount = 0;
 					n->is_local = true;
 					n->is_not_null = false;
@@ -3793,8 +3794,8 @@ columnDef:	ColId Typename opt_column_compression create_generic_options ColQualL
 					n->raw_default = NULL;
 					n->cooked_default = NULL;
 					n->collOid = InvalidOid;
-					n->fdwoptions = $4;
-					SplitColQualList($5, &n->constraints, &n->collClause,
+					n->fdwoptions = $5;
+					SplitColQualList($6, &n->constraints, &n->collClause,
 									 yyscanner);
 					n->location = @1;
 					$$ = (Node *) n;
@@ -3851,6 +3852,15 @@ opt_column_compression:
 			| /*EMPTY*/								{ $$ = NULL; }
 		;
 
+column_storage:
+			STORAGE ColId							{ $$ = $2; }
+		;
+
+opt_column_storage:
+			column_storage							{ $$ = $1; }
+			| /*EMPTY*/								{ $$ = NULL; }
+		;
+
 ColQualList:
 			ColQualList ColConstraint				{ $$ = lappend($1, $2); }
 			| /*EMPTY*/								{ $$ = NIL; }
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 0b6a7bb365..d471bf0ebc 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -695,6 +695,7 @@ typedef struct ColumnDef
 	bool		is_not_null;	/* NOT NULL constraint specified? */
 	bool		is_from_type;	/* column definition came from table type */
 	char		storage;		/* attstorage setting, or 0 for default */
+	char		*storage_name;	/* attstorage setting name or NULL for default*/
 	Node	   *raw_default;	/* default value (untransformed parse tree) */
 	Node	   *cooked_default; /* default value (transformed expr tree) */
 	char		identity;		/* attidentity setting */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 5ede56d9b5..fa1ee9e9b6 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2244,7 +2244,7 @@ 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);
+create table test_storage (a text, c text storage plain);
 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
@@ -2256,6 +2256,9 @@ where oid = 'test_storage'::regclass;
  t
 (1 row)
 
+--check STORAGE correctness
+create table test_storage_failed (a text, b int storage extended);
+ERROR:  column data type integer can only have storage PLAIN
 -- 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;
@@ -2264,6 +2267,7 @@ alter table test_storage alter column a set storage external;
  Column |  Type   | Collation | Nullable | Default | Storage  | Stats target | Description 
 --------+---------+-----------+----------+---------+----------+--------------+-------------
  a      | text    |           |          |         | external |              | 
+ c      | text    |           |          |         | plain    |              | 
  b      | integer |           |          | 0       | plain    |              | 
 Indexes:
     "test_storage_idx" btree (b, a)
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 52001e3135..534166501c 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1527,7 +1527,7 @@ 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);
+create table test_storage (a text, c text storage plain);
 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
@@ -1536,6 +1536,9 @@ select reltoastrelid <> 0 as has_toast_table
 from pg_class
 where oid = 'test_storage'::regclass;
 
+--check STORAGE correctness
+create table test_storage_failed (a text, b int storage extended);
+
 -- 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;
-- 
2.36.1



^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  2022-06-22 13:29   ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
  2022-06-24 11:44     ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-06-24 12:30       ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-07-11 09:27         ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
@ 2022-07-11 13:17           ` Peter Eisentraut <[email protected]>
  2022-07-12 10:10             ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  0 siblings, 1 reply; 15+ messages in thread

From: Peter Eisentraut @ 2022-07-11 13:17 UTC (permalink / raw)
  To: Aleksander Alekseev <[email protected]>; pgsql-hackers; +Cc: Matthias van de Meent <[email protected]>; Teodor Sigaev <[email protected]>; wenjing zeng <[email protected]>

On 11.07.22 11:27, Aleksander Alekseev wrote:
>>> Here is a patch updated according to all the recent feedback, except
>>> for two suggestions:
>>
>> In v4 I forgot to list possible arguments for STORAGE in
>> alter_table.sgml, similarly as it is done for other subcommands. Here
>> is a corrected patch.
> 
> Here is the rebased patch.

The "safety check: do not allow toasted storage modes unless column 
datatype is TOAST-aware" could be moved into GetAttributeStorage(), so 
it doesn't have to be repeated.  (Note that GetAttributeCompression() 
does similar checking.)

ATExecSetStorage() currently doesn't do any such check, and your patch 
isn't adding one.  Is there a reason for that?





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  2022-06-22 13:29   ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
  2022-06-24 11:44     ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-06-24 12:30       ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-07-11 09:27         ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-07-11 13:17           ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
@ 2022-07-12 10:10             ` Aleksander Alekseev <[email protected]>
  2022-07-13 10:28               ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
  0 siblings, 1 reply; 15+ messages in thread

From: Aleksander Alekseev @ 2022-07-12 10:10 UTC (permalink / raw)
  To: pgsql-hackers; +Cc: Peter Eisentraut <[email protected]>; Matthias van de Meent <[email protected]>; Teodor Sigaev <[email protected]>; wenjing zeng <[email protected]>

Hi Peter,

> The "safety check: do not allow toasted storage modes unless column
> datatype is TOAST-aware" could be moved into GetAttributeStorage(), so
> it doesn't have to be repeated.  (Note that GetAttributeCompression()
> does similar checking.)

Good point. Fixed.

> ATExecSetStorage() currently doesn't do any such check, and your patch
> isn't adding one.  Is there a reason for that?

ATExecSetStorage() does this, but the check is a bit below [1]. In v7
I moved the check to GetAttributeStorage() as well.

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/commands/tablecmds.c#l8312

-- 
Best regards,
Aleksander Alekseev


Attachments:

  [application/octet-stream] v7-0001-Allow-specifying-STORAGE-attribute-for-a-new-tabl.patch (19.5K, ../../CAJ7c6TPsg6iUGZGaEGKi5vdG6VpEcLyZMbXTUQFnCNdi+Kd00Q@mail.gmail.com/2-v7-0001-Allow-specifying-STORAGE-attribute-for-a-new-tabl.patch)
  download | inline diff:
From 4ffaae8b743c5288f7512097bb60da41f131e116 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email protected]>
Date: Wed, 15 Jun 2022 16:34:50 +0300
Subject: [PATCH v7] Allow specifying STORAGE attribute for a new table.

Also make the code and the documentation for STORAGE and COMPRESSION
attributes consistent.

Author:	Teodor Sigaev <[email protected]>
Author: Aleksander Alekseev <[email protected]>
Reviewed-by: Peter Eisentraut <[email protected]>
Reviewed-by: wenjing zeng <[email protected]>
Reviewed-by: Matthias van de Meent <[email protected]>
Reviewed-by: Kyotaro Horiguchi <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
 doc/src/sgml/ref/alter_table.sgml         | 40 +---------
 doc/src/sgml/ref/create_table.sgml        | 23 +++++-
 src/backend/commands/tablecmds.c          | 93 +++++++++++++----------
 src/backend/parser/gram.y                 | 24 ++++--
 src/include/nodes/parsenodes.h            |  1 +
 src/test/regress/expected/alter_table.out |  6 +-
 src/test/regress/sql/alter_table.sql      |  5 +-
 7 files changed, 103 insertions(+), 89 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index a3c62bf056..e83837088a 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -367,7 +367,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
 
    <varlistentry>
     <term>
-     <literal>SET STORAGE</literal>
+     <literal>SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }</literal>
      <indexterm>
       <primary>TOAST</primary>
       <secondary>per-column storage settings</secondary>
@@ -375,22 +375,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     </term>
     <listitem>
      <para>
-      This form sets the storage mode for a column. This controls whether this
-      column is 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 table,
-      it just sets the strategy to be pursued during future table updates.
-      See <xref linkend="storage-toast"/> for more information.
+      See <xref linkend="sql-createtable"/> for details.
      </para>
     </listitem>
    </varlistentry>
@@ -401,26 +386,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     </term>
     <listitem>
      <para>
-      This form sets the compression method for a column, determining how
-      values inserted in future will be compressed (if the storage mode
-      permits compression at all).
-      This does not cause the table to be rewritten, so existing data may still
-      be compressed with other compression methods.  If the table is restored
-      with <application>pg_restore</application>, then all values are rewritten
-      with the configured compression method.
-      However, when data is inserted from another relation (for example,
-      by <command>INSERT ... SELECT</command>), values from the source table are
-      not necessarily detoasted, so any previously compressed data may retain
-      its existing compression method, rather than being recompressed with the
-      compression method of the target column.
-      The supported compression
-      methods are <literal>pglz</literal> and <literal>lz4</literal>.
-      (<literal>lz4</literal> is available only if <option>--with-lz4</option>
-      was used when building <productname>PostgreSQL</productname>.)  In
-      addition, <replaceable class="parameter">compression_method</replaceable>
-      can be <literal>default</literal>, which selects the default behavior of
-      consulting the <xref linkend="guc-default-toast-compression"/> setting
-      at the time of data insertion to determine the method to use.
+      See <xref linkend="sql-createtable"/> for details.
      </para>
     </listitem>
    </varlistentry>
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 6c9918b0a1..da90698046 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
  <refsynopsisdiv>
 <synopsis>
 CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name</replaceable> ( [
-  { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [ COMPRESSION <replaceable>compression_method</replaceable> ] [ COLLATE <replaceable>collation</replaceable> ] [ <replaceable class="parameter">column_constraint</replaceable> [ ... ] ]
+  { <replaceable class="parameter">column_name</replaceable> <replaceable class="parameter">data_type</replaceable> [ STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN } ] [ COMPRESSION <replaceable>compression_method</replaceable> ] [ COLLATE <replaceable>collation</replaceable> ] [ <replaceable class="parameter">column_constraint</replaceable> [ ... ] ]
     | <replaceable>table_constraint</replaceable>
     | LIKE <replaceable>source_table</replaceable> [ <replaceable>like_option</replaceable> ... ] }
     [, ... ]
@@ -297,6 +297,27 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term> <literal>STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }</literal></term>
+    <listitem>
+     <para>
+      This form sets the storage mode for the column. This controls whether this
+      column is 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. See <xref linkend="storage-toast"/> for more
+      information.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term><literal>COMPRESSION <replaceable class="parameter">compression_method</replaceable></literal></term>
     <listitem>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ef5b34a312..1df3a4c1d0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -593,7 +593,7 @@ static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKM
 static void ATExecGenericOptions(Relation rel, List *options);
 static void ATExecSetRowSecurity(Relation rel, bool rls);
 static void ATExecForceNoForceRowSecurity(Relation rel, bool force_rls);
-static ObjectAddress ATExecSetCompression(AlteredTableInfo *tab, Relation rel,
+static ObjectAddress ATExecSetCompression(Relation rel,
 										  const char *column, Node *newValue, LOCKMODE lockmode);
 
 static void index_copy_data(Relation rel, RelFileLocator newrlocator);
@@ -633,6 +633,7 @@ static void refuseDupeIndexAttach(Relation parentIdx, Relation partIdx,
 static List *GetParentedForeignKeyRefs(Relation partition);
 static void ATDetachCheckNoForeignKeyRefs(Relation partition);
 static char GetAttributeCompression(Oid atttypid, char *compression);
+static char GetAttributeStorage(Oid atttypid, const char *storagemode);
 
 
 /* ----------------------------------------------------------------
@@ -931,6 +932,10 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		if (colDef->compression)
 			attr->attcompression = GetAttributeCompression(attr->atttypid,
 														   colDef->compression);
+
+		if (colDef->storage_name)
+			attr->attstorage = GetAttributeStorage(attr->atttypid, colDef->storage_name);
+
 	}
 
 	/*
@@ -4963,8 +4968,8 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
 		case AT_SetStorage:		/* ALTER COLUMN SET STORAGE */
 			address = ATExecSetStorage(rel, cmd->name, cmd->def, lockmode);
 			break;
-		case AT_SetCompression:
-			address = ATExecSetCompression(tab, rel, cmd->name, cmd->def,
+		case AT_SetCompression: /* ALTER COLUMN SET COMPRESSION */
+			address = ATExecSetCompression(rel, cmd->name, cmd->def,
 										   lockmode);
 			break;
 		case AT_DropColumn:		/* DROP COLUMN */
@@ -6820,7 +6825,11 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	attribute.atttypmod = typmod;
 	attribute.attbyval = tform->typbyval;
 	attribute.attalign = tform->typalign;
-	attribute.attstorage = tform->typstorage;
+	if (colDef->storage_name)
+		attribute.attstorage = GetAttributeStorage(typeOid, colDef->storage_name);
+	else
+		attribute.attstorage = tform->typstorage;
+
 	attribute.attcompression = GetAttributeCompression(typeOid,
 													   colDef->compression);
 	attribute.attnotnull = colDef->is_not_null;
@@ -8263,34 +8272,12 @@ SetIndexStorageProperties(Relation rel, Relation attrelation,
 static ObjectAddress
 ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE lockmode)
 {
-	char	   *storagemode;
-	char		newstorage;
 	Relation	attrelation;
 	HeapTuple	tuple;
 	Form_pg_attribute attrtuple;
 	AttrNumber	attnum;
 	ObjectAddress address;
 
-	Assert(IsA(newValue, String));
-	storagemode = strVal(newValue);
-
-	if (pg_strcasecmp(storagemode, "plain") == 0)
-		newstorage = TYPSTORAGE_PLAIN;
-	else if (pg_strcasecmp(storagemode, "external") == 0)
-		newstorage = TYPSTORAGE_EXTERNAL;
-	else if (pg_strcasecmp(storagemode, "extended") == 0)
-		newstorage = TYPSTORAGE_EXTENDED;
-	else if (pg_strcasecmp(storagemode, "main") == 0)
-		newstorage = TYPSTORAGE_MAIN;
-	else
-	{
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("invalid storage type \"%s\"",
-						storagemode)));
-		newstorage = 0;			/* keep compiler quiet */
-	}
-
 	attrelation = table_open(AttributeRelationId, RowExclusiveLock);
 
 	tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), colName);
@@ -8309,17 +8296,8 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc
 				 errmsg("cannot alter system column \"%s\"",
 						colName)));
 
-	/*
-	 * safety check: do not allow toasted storage modes unless column datatype
-	 * is TOAST-aware.
-	 */
-	if (newstorage == TYPSTORAGE_PLAIN || TypeIsToastable(attrtuple->atttypid))
-		attrtuple->attstorage = newstorage;
-	else
-		ereport(ERROR,
-				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("column data type %s can only have storage PLAIN",
-						format_type_be(attrtuple->atttypid))));
+	Assert(IsA(newValue, String));
+	attrtuple->attstorage = GetAttributeStorage(attrtuple->atttypid, strVal(newValue));
 
 	CatalogTupleUpdate(attrelation, &tuple->t_self, tuple);
 
@@ -8327,17 +8305,17 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc
 							  RelationGetRelid(rel),
 							  attrtuple->attnum);
 
-	heap_freetuple(tuple);
-
 	/*
 	 * Apply the change to indexes as well (only for simple index columns,
 	 * matching behavior of index.c ConstructTupleDescriptor()).
 	 */
 	SetIndexStorageProperties(rel, attrelation, attnum,
-							  true, newstorage,
+							  true, attrtuple->attstorage,
 							  false, 0,
 							  lockmode);
 
+	heap_freetuple(tuple);
+
 	table_close(attrelation, RowExclusiveLock);
 
 	ObjectAddressSubSet(address, RelationRelationId,
@@ -16157,8 +16135,7 @@ ATExecGenericOptions(Relation rel, List *options)
  * Return value is the address of the modified column
  */
 static ObjectAddress
-ATExecSetCompression(AlteredTableInfo *tab,
-					 Relation rel,
+ATExecSetCompression(Relation rel,
 					 const char *column,
 					 Node *newValue,
 					 LOCKMODE lockmode)
@@ -19289,3 +19266,35 @@ GetAttributeCompression(Oid atttypid, char *compression)
 
 	return cmethod;
 }
+
+static char
+GetAttributeStorage(Oid atttypid, const char *storagemode)
+{
+	char		cstorage;
+
+	if (pg_strcasecmp(storagemode, "plain") == 0)
+		cstorage = TYPSTORAGE_PLAIN;
+	else if (pg_strcasecmp(storagemode, "external") == 0)
+		cstorage = TYPSTORAGE_EXTERNAL;
+	else if (pg_strcasecmp(storagemode, "extended") == 0)
+		cstorage = TYPSTORAGE_EXTENDED;
+	else if (pg_strcasecmp(storagemode, "main") == 0)
+		cstorage = TYPSTORAGE_MAIN;
+	else
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("invalid storage type \"%s\"",
+						storagemode)));
+
+	/*
+	 * safety check: do not allow toasted storage modes unless column datatype
+	 * is TOAST-aware.
+	 */
+	if (!(cstorage == TYPSTORAGE_PLAIN || TypeIsToastable(atttypid)))
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("column data type %s can only have storage PLAIN",
+						format_type_be(atttypid))));
+
+	return cstorage;
+}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 0523013f53..c018140afe 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -595,7 +595,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 
 %type <node>	TableConstraint TableLikeClause
 %type <ival>	TableLikeOptionList TableLikeOption
-%type <str>		column_compression opt_column_compression
+%type <str>		column_compression opt_column_compression column_storage opt_column_storage
 %type <list>	ColQualList
 %type <node>	ColConstraint ColConstraintElem ConstraintAttr
 %type <ival>	key_match
@@ -2537,13 +2537,13 @@ alter_table_cmd:
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET STORAGE <storagemode> */
-			| ALTER opt_column ColId SET STORAGE ColId
+			| ALTER opt_column ColId SET column_storage
 				{
 					AlterTableCmd *n = makeNode(AlterTableCmd);
 
 					n->subtype = AT_SetStorage;
 					n->name = $3;
-					n->def = (Node *) makeString($6);
+					n->def = (Node *) makeString($5);
 					$$ = (Node *) n;
 				}
 			/* ALTER TABLE <name> ALTER [COLUMN] <colname> SET COMPRESSION <cm> */
@@ -3778,13 +3778,14 @@ TypedTableElement:
 			| TableConstraint					{ $$ = $1; }
 		;
 
-columnDef:	ColId Typename opt_column_compression create_generic_options ColQualList
+columnDef:	ColId Typename opt_column_storage opt_column_compression create_generic_options ColQualList
 				{
 					ColumnDef *n = makeNode(ColumnDef);
 
 					n->colname = $1;
 					n->typeName = $2;
-					n->compression = $3;
+					n->storage_name = $3;
+					n->compression = $4;
 					n->inhcount = 0;
 					n->is_local = true;
 					n->is_not_null = false;
@@ -3793,8 +3794,8 @@ columnDef:	ColId Typename opt_column_compression create_generic_options ColQualL
 					n->raw_default = NULL;
 					n->cooked_default = NULL;
 					n->collOid = InvalidOid;
-					n->fdwoptions = $4;
-					SplitColQualList($5, &n->constraints, &n->collClause,
+					n->fdwoptions = $5;
+					SplitColQualList($6, &n->constraints, &n->collClause,
 									 yyscanner);
 					n->location = @1;
 					$$ = (Node *) n;
@@ -3851,6 +3852,15 @@ opt_column_compression:
 			| /*EMPTY*/								{ $$ = NULL; }
 		;
 
+column_storage:
+			STORAGE ColId							{ $$ = $2; }
+		;
+
+opt_column_storage:
+			column_storage							{ $$ = $1; }
+			| /*EMPTY*/								{ $$ = NULL; }
+		;
+
 ColQualList:
 			ColQualList ColConstraint				{ $$ = lappend($1, $2); }
 			| /*EMPTY*/								{ $$ = NIL; }
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 0b6a7bb365..280bf03795 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -695,6 +695,7 @@ typedef struct ColumnDef
 	bool		is_not_null;	/* NOT NULL constraint specified? */
 	bool		is_from_type;	/* column definition came from table type */
 	char		storage;		/* attstorage setting, or 0 for default */
+	char	   *storage_name;	/* attstorage setting name or NULL for default */
 	Node	   *raw_default;	/* default value (untransformed parse tree) */
 	Node	   *cooked_default; /* default value (transformed expr tree) */
 	char		identity;		/* attidentity setting */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 5ede56d9b5..fa1ee9e9b6 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2244,7 +2244,7 @@ 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);
+create table test_storage (a text, c text storage plain);
 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
@@ -2256,6 +2256,9 @@ where oid = 'test_storage'::regclass;
  t
 (1 row)
 
+--check STORAGE correctness
+create table test_storage_failed (a text, b int storage extended);
+ERROR:  column data type integer can only have storage PLAIN
 -- 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;
@@ -2264,6 +2267,7 @@ alter table test_storage alter column a set storage external;
  Column |  Type   | Collation | Nullable | Default | Storage  | Stats target | Description 
 --------+---------+-----------+----------+---------+----------+--------------+-------------
  a      | text    |           |          |         | external |              | 
+ c      | text    |           |          |         | plain    |              | 
  b      | integer |           |          | 0       | plain    |              | 
 Indexes:
     "test_storage_idx" btree (b, a)
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 52001e3135..534166501c 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1527,7 +1527,7 @@ 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);
+create table test_storage (a text, c text storage plain);
 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
@@ -1536,6 +1536,9 @@ select reltoastrelid <> 0 as has_toast_table
 from pg_class
 where oid = 'test_storage'::regclass;
 
+--check STORAGE correctness
+create table test_storage_failed (a text, b int storage extended);
+
 -- 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;
-- 
2.36.1



^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  2022-06-22 13:29   ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
  2022-06-24 11:44     ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-06-24 12:30       ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-07-11 09:27         ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-07-11 13:17           ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
  2022-07-12 10:10             ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
@ 2022-07-13 10:28               ` Peter Eisentraut <[email protected]>
  2022-07-13 10:38                 ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  0 siblings, 1 reply; 15+ messages in thread

From: Peter Eisentraut @ 2022-07-13 10:28 UTC (permalink / raw)
  To: Aleksander Alekseev <[email protected]>; pgsql-hackers; +Cc: Matthias van de Meent <[email protected]>; Teodor Sigaev <[email protected]>; wenjing zeng <[email protected]>

On 12.07.22 12:10, Aleksander Alekseev wrote:
> Hi Peter,
> 
>> The "safety check: do not allow toasted storage modes unless column
>> datatype is TOAST-aware" could be moved into GetAttributeStorage(), so
>> it doesn't have to be repeated.  (Note that GetAttributeCompression()
>> does similar checking.)
> 
> Good point. Fixed.
> 
>> ATExecSetStorage() currently doesn't do any such check, and your patch
>> isn't adding one.  Is there a reason for that?
> 
> ATExecSetStorage() does this, but the check is a bit below [1]. In v7
> I moved the check to GetAttributeStorage() as well.
> 
> [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/commands/tablecmds.c#l8312

Committed.

I thought the removal of the documentation details of SET COMPRESSION 
and SET STORAGE from the ALTER TABLE ref page was a bit excessive, since 
that material actually contained useful information about what happens 
when you change compression or storage on a table with existing data. 
So I left that in.  Maybe there is room to deduplicate that material a 
bit, but it would need to be more fine-grained than just removing one 
side of it.






^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: CREATE TABLE ( .. STORAGE ..)
  2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
  2022-03-29 20:28 ` Re: CREATE TABLE ( .. STORAGE ..) Matthias van de Meent <[email protected]>
  2022-06-22 13:29   ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
  2022-06-24 11:44     ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-06-24 12:30       ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-07-11 09:27         ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-07-11 13:17           ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
  2022-07-12 10:10             ` Re: CREATE TABLE ( .. STORAGE ..) Aleksander Alekseev <[email protected]>
  2022-07-13 10:28               ` Re: CREATE TABLE ( .. STORAGE ..) Peter Eisentraut <[email protected]>
@ 2022-07-13 10:38                 ` Aleksander Alekseev <[email protected]>
  0 siblings, 0 replies; 15+ messages in thread

From: Aleksander Alekseev @ 2022-07-13 10:38 UTC (permalink / raw)
  To: pgsql-hackers; +Cc: Peter Eisentraut <[email protected]>; Matthias van de Meent <[email protected]>; Teodor Sigaev <[email protected]>; wenjing zeng <[email protected]>

Hi,

> Committed.
>
> I thought the removal of the documentation details of SET COMPRESSION
> and SET STORAGE from the ALTER TABLE ref page was a bit excessive, since
> that material actually contained useful information about what happens
> when you change compression or storage on a table with existing data.
> So I left that in.  Maybe there is room to deduplicate that material a
> bit, but it would need to be more fine-grained than just removing one
> side of it.

Thanks, Peter!

-- 
Best regards,
Aleksander Alekseev





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

end of thread, other threads:[~2022-07-13 10:38 UTC | newest]

Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-12-27 07:51 CREATE TABLE ( .. STORAGE ..) Teodor Sigaev <[email protected]>
2022-03-29 20:28 ` Matthias van de Meent <[email protected]>
2022-06-15 14:51   ` Aleksander Alekseev <[email protected]>
2022-06-16 09:17     ` Matthias van de Meent <[email protected]>
2022-06-16 13:40       ` Aleksander Alekseev <[email protected]>
2022-06-17 02:45         ` Kyotaro Horiguchi <[email protected]>
2022-06-22 13:25     ` Peter Eisentraut <[email protected]>
2022-06-22 13:29   ` Peter Eisentraut <[email protected]>
2022-06-24 11:44     ` Aleksander Alekseev <[email protected]>
2022-06-24 12:30       ` Aleksander Alekseev <[email protected]>
2022-07-11 09:27         ` Aleksander Alekseev <[email protected]>
2022-07-11 13:17           ` Peter Eisentraut <[email protected]>
2022-07-12 10:10             ` Aleksander Alekseev <[email protected]>
2022-07-13 10:28               ` Peter Eisentraut <[email protected]>
2022-07-13 10:38                 ` Aleksander Alekseev <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox