[PATCH] ALTER TABLE ... SET STORAGE default
Hi hackers,
I noticed that we support 'ALTER TABLE ... SET COMPRESSION default'
syntax, but not 'SET STORAGE default' which seems to be a bit
inconsistent. When the user changes the storage mode for a column
there is no convenient way to revert the change.
The proposed patch fixes this.
--
Best regards,
Aleksander Alekseev
Attachments:
v1-0001-ALTER-TABLE-.-SET-STORAGE-default.patchapplication/octet-stream; name=v1-0001-ALTER-TABLE-.-SET-STORAGE-default.patchDownload
From a32e4d5bb18aeb5ed132dfb37d4c8d8ca4fc1353 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Mon, 22 Aug 2022 15:10:57 +0300
Subject: [PATCH v1] ALTER TABLE ... SET STORAGE default
Add support of 'ALTER TABLE .. SET STORAGE default' syntax similarly to
'SET COMPRESSION default', for consistency. Also add corresponding tab
completion to psql.
Author: Aleksander Alekseev <aleksander@timescale.com>
Reviewed-by: TODO FIXME
Discussion: TODO FIXME
---
doc/src/sgml/ref/alter_foreign_table.sgml | 2 +-
doc/src/sgml/ref/alter_materialized_view.sgml | 2 +-
doc/src/sgml/ref/alter_table.sgml | 13 ++++++++-----
src/backend/commands/tablecmds.c | 4 +++-
src/backend/parser/gram.y | 1 +
src/bin/psql/tab-complete.c | 2 +-
src/test/regress/expected/alter_table.out | 2 +-
src/test/regress/sql/alter_table.sql | 2 +-
8 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/ref/alter_foreign_table.sgml b/doc/src/sgml/ref/alter_foreign_table.sgml
index 7ca03f3ac9..84c298e7a5 100644
--- a/doc/src/sgml/ref/alter_foreign_table.sgml
+++ b/doc/src/sgml/ref/alter_foreign_table.sgml
@@ -41,7 +41,7 @@ ALTER FOREIGN TABLE [ IF EXISTS ] <replaceable class="parameter">name</replaceab
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STATISTICS <replaceable class="parameter">integer</replaceable>
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET ( <replaceable class="parameter">attribute_option</replaceable> = <replaceable class="parameter">value</replaceable> [, ... ] )
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> RESET ( <replaceable class="parameter">attribute_option</replaceable> [, ... ] )
- ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
+ ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STORAGE { DEFAULT | PLAIN | EXTERNAL | EXTENDED | MAIN }
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> OPTIONS ( [ ADD | SET | DROP ] <replaceable class="parameter">option</replaceable> ['<replaceable class="parameter">value</replaceable>'] [, ... ])
ADD <replaceable class="parameter">table_constraint</replaceable> [ NOT VALID ]
VALIDATE CONSTRAINT <replaceable class="parameter">constraint_name</replaceable>
diff --git a/doc/src/sgml/ref/alter_materialized_view.sgml b/doc/src/sgml/ref/alter_materialized_view.sgml
index cae135c27a..ecb8f68db5 100644
--- a/doc/src/sgml/ref/alter_materialized_view.sgml
+++ b/doc/src/sgml/ref/alter_materialized_view.sgml
@@ -39,7 +39,7 @@ ALTER MATERIALIZED VIEW ALL IN TABLESPACE <replaceable class="parameter">name</r
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STATISTICS <replaceable class="parameter">integer</replaceable>
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET ( <replaceable class="parameter">attribute_option</replaceable> = <replaceable class="parameter">value</replaceable> [, ... ] )
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> RESET ( <replaceable class="parameter">attribute_option</replaceable> [, ... ] )
- ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
+ ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STORAGE { DEFAULT | PLAIN | EXTERNAL | EXTENDED | MAIN }
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET COMPRESSION <replaceable class="parameter">compression_method</replaceable>
CLUSTER ON <replaceable class="parameter">index_name</replaceable>
SET WITHOUT CLUSTER
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index f0f912a56c..99a23f40e3 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -53,7 +53,7 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STATISTICS <replaceable class="parameter">integer</replaceable>
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET ( <replaceable class="parameter">attribute_option</replaceable> = <replaceable class="parameter">value</replaceable> [, ... ] )
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> RESET ( <replaceable class="parameter">attribute_option</replaceable> [, ... ] )
- ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
+ ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET STORAGE { DEFAULT | PLAIN | EXTERNAL | EXTENDED | MAIN }
ALTER [ COLUMN ] <replaceable class="parameter">column_name</replaceable> SET COMPRESSION <replaceable class="parameter">compression_method</replaceable>
ADD <replaceable class="parameter">table_constraint</replaceable> [ NOT VALID ]
ADD <replaceable class="parameter">table_constraint_using_index</replaceable>
@@ -367,7 +367,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<varlistentry>
<term>
- <literal>SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }</literal>
+ <literal>SET STORAGE { DEFAULT | PLAIN | EXTERNAL | EXTENDED | MAIN }</literal>
<indexterm>
<primary>TOAST</primary>
<secondary>per-column storage settings</secondary>
@@ -387,9 +387,12 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
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.
+ at the penalty of increased storage space.
+ Choosing <literal>DEFAULT</literal> resets the storage mode to the default
+ one for a given type.
+ 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.
</para>
</listitem>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9be04c8a1e..ccbe5cef12 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -19289,7 +19289,9 @@ GetAttributeStorage(Oid atttypid, const char *storagemode)
{
char cstorage = 0;
- if (pg_strcasecmp(storagemode, "plain") == 0)
+ if (pg_strcasecmp(storagemode, "default") == 0)
+ cstorage = get_typstorage(atttypid);
+ else if (pg_strcasecmp(storagemode, "plain") == 0)
cstorage = TYPSTORAGE_PLAIN;
else if (pg_strcasecmp(storagemode, "external") == 0)
cstorage = TYPSTORAGE_EXTERNAL;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index f9037761f9..f1cdf9e1ae 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -3872,6 +3872,7 @@ opt_column_compression:
column_storage:
STORAGE ColId { $$ = $2; }
+ | STORAGE DEFAULT { $$ = pstrdup("default"); }
;
opt_column_storage:
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 62a39779b9..4c15348d87 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2388,7 +2388,7 @@ psql_completion(const char *text, int start, int end)
/* ALTER TABLE ALTER [COLUMN] <foo> SET STORAGE */
else if (Matches("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny, "SET", "STORAGE") ||
Matches("ALTER", "TABLE", MatchAny, "ALTER", MatchAny, "SET", "STORAGE"))
- COMPLETE_WITH("PLAIN", "EXTERNAL", "EXTENDED", "MAIN");
+ COMPLETE_WITH("DEFAULT", "PLAIN", "EXTERNAL", "EXTENDED", "MAIN");
/* ALTER TABLE ALTER [COLUMN] <foo> SET STATISTICS */
else if (Matches("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny, "SET", "STATISTICS") ||
Matches("ALTER", "TABLE", MatchAny, "ALTER", MatchAny, "SET", "STATISTICS"))
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index d63f4f1cba..b9dd835df8 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2247,7 +2247,7 @@ ERROR: composite type recur1 cannot be made a member of itself
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
+alter table test_storage alter a set storage default; -- re-add TOAST table
select reltoastrelid <> 0 as has_toast_table
from pg_class
where oid = 'test_storage'::regclass;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index e7013f5e15..c29718327e 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1530,7 +1530,7 @@ alter table recur1 alter column f2 type recur2; -- fails
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
+alter table test_storage alter a set storage default; -- re-add TOAST table
select reltoastrelid <> 0 as has_toast_table
from pg_class
--
2.37.2
Hi hackers!
This seems a little bit confusing and thus very unfriendly for the user,
because the actual meaning
of the same 'DEFAULT' option will be different for each data type, and
to check storage mode user
has to query full table (or column) description.
I'd rather add a paragraph in documentation describing each data type
default storage mode.
On Mon, Aug 22, 2022 at 3:34 PM Aleksander Alekseev <
aleksander@timescale.com> wrote:
Hi hackers,
I noticed that we support 'ALTER TABLE ... SET COMPRESSION default'
syntax, but not 'SET STORAGE default' which seems to be a bit
inconsistent. When the user changes the storage mode for a column
there is no convenient way to revert the change.The proposed patch fixes this.
--
Best regards,
Aleksander Alekseev
--
Regards,
Nikita Malakhov
https://postgrespro.ru/
Hi Nikita,
This seems a little bit confusing and thus very unfriendly for the user, because the actual meaning
of the same 'DEFAULT' option will be different for each data type, and to check storage mode user
has to query full table (or column) description.
I'd rather add a paragraph in documentation describing each data type default storage mode.
I agree that "SET STORAGE default" syntax leaves much to be desired.
Personally I would prefer "RESET STORAGE" and "RESET COMPRESSION". But
since we already have "SET COMPRESSION default" this going to be
either two commands that do the same thing, or a broken backward
compatibility. Simply removing "SET COMPRESSION default" will make the
syntax consistent too, but again, this would be a broken backward
compatibility. I would argue that a sub-optimal but consistent syntax
that does the job is better than inconsistent syntax and figuring out
the default storage strategy manually.
But let's see what is others people opinion.
--
Best regards,
Aleksander Alekseev
Hi!
Anyway, adding a paragraph with default storage mode for each standard data
type seems
like a good idea and I'd prepare a patch for it.
Thank you!
On Mon, Aug 22, 2022 at 4:28 PM Aleksander Alekseev <
aleksander@timescale.com> wrote:
Hi Nikita,
This seems a little bit confusing and thus very unfriendly for the user,
because the actual meaning
of the same 'DEFAULT' option will be different for each data type, and
to check storage mode user
has to query full table (or column) description.
I'd rather add a paragraph in documentation describing each data typedefault storage mode.
I agree that "SET STORAGE default" syntax leaves much to be desired.
Personally I would prefer "RESET STORAGE" and "RESET COMPRESSION". But
since we already have "SET COMPRESSION default" this going to be
either two commands that do the same thing, or a broken backward
compatibility. Simply removing "SET COMPRESSION default" will make the
syntax consistent too, but again, this would be a broken backward
compatibility. I would argue that a sub-optimal but consistent syntax
that does the job is better than inconsistent syntax and figuring out
the default storage strategy manually.But let's see what is others people opinion.
--
Best regards,
Aleksander Alekseev
--
Regards,
Nikita Malakhov
https://postgrespro.ru/
Aleksander Alekseev <aleksander@timescale.com> writes:
Hi Nikita,
This seems a little bit confusing and thus very unfriendly for the user, because the actual meaning
of the same 'DEFAULT' option will be different for each data type, and to check storage mode user
has to query full table (or column) description.
I'd rather add a paragraph in documentation describing each data type default storage mode.
I agree that "SET STORAGE default" syntax leaves much to be desired.
FWIW, I don't buy that argument at all. If you believe that then
you must also think that
INSERT INTO mytab VALUES (..., DEFAULT, ...);
is a poorly-designed feature because you have to go consult the table
definition to find out what will be inserted. (Well, maybe you do
think that, but the SQL committee won't agree with you ;-)) So I don't
see any problem with DEFAULT representing a data-type-specific default
in this situation.
Personally I would prefer "RESET STORAGE" and "RESET COMPRESSION".
Perhaps, but what's done is done, and I agree that STORAGE had better
follow the existing precedent.
I've not read the patch in any detail, but I don't see a problem
with the design.
regards, tom lane
I wrote:
I've not read the patch in any detail, but I don't see a problem
with the design.
Hearing no push-back on that position, I reviewed and pushed the
patch. You'd missed that it also affects CREATE TABLE, but
otherwise it was in pretty good shape.
regards, tom lane