Re: ALTER TABLE on system catalogs

At Fri, 08 Feb 2019 12:03:31 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20190208.120331.167280496.horiguchi.kyotaro@lab.ntt.co.jp>
> By the way, I'm confused to see that attributes that don't want
> to go external are marked as 'x' in system catalogs. Currently
> (putting aside its necessity) the following operation ends with
> successful attaching a new TOAST relation, which we really don't
> want.
> 
> ALTER TABLE pg_attribute ALTER COLUMN attrelid SET STORAGE plain;
> 
> Might be silly, we may need another storage class, say,
> Compression, which means try compress but don't go external.
> 
> The attached patch does that.
> 
> - All varlen fields of pg_class and pg_attribute are marked as
>   'c'.  (Catalog.pm, genbki.pl, genbki.h, pg_attribute/class.h)
> 
> - Try compress but don't try external for 'c' storage.
>   (tuptoaster.c, toasting.c)
> 
> 
> We could remove toast relation when no toastable attribute
> remains after ATLER TABLE ALTER COLUMN SET STOAGE, but it doesn't
> seem that simple. So the storage class is for internal use only.

I found that "ALTER TABLE.. SET STORAGE plain" doesn't remove
toast relation, so it's not so bad even if compress does the
same?

The attached 0001 is fixed from the previous version. 0002 is the
syntax.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 14312f2b53edb053281b80cf3df16851ef474525 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 8 Feb 2019 11:22:57 +0900
Subject: [PATCH 1/2] Explicitly mark some attributes in catalog as no need for
 toast relation

Currently, there's some attributes of catalogs that storage class is
'x' but really don't need toasted. This causes several sorts of
unwanted things.  This patch adds a new storage class 'c' (compress),
which means "try compress in-line, but don't go external', then set
storage class of the attributes to it.
---
 src/backend/access/heap/tuptoaster.c | 4 +++-
 src/backend/catalog/Catalog.pm       | 6 +++++-
 src/backend/catalog/genbki.pl        | 5 ++++-
 src/backend/catalog/toasting.c       | 2 +-
 src/backend/commands/tablecmds.c     | 2 ++
 src/include/catalog/genbki.h         | 2 ++
 src/include/catalog/pg_attribute.h   | 8 ++++----
 src/include/catalog/pg_class.h       | 6 +++---
 8 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/src/backend/access/heap/tuptoaster.c b/src/backend/access/heap/tuptoaster.c
index cd921a4600..9718d15487 100644
--- a/src/backend/access/heap/tuptoaster.c
+++ b/src/backend/access/heap/tuptoaster.c
@@ -888,13 +888,15 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
          */
         for (i = 0; i < numAttrs; i++)
         {
+            Form_pg_attribute att = TupleDescAttr(tupleDesc, i);
+
             if (toast_action[i] != ' ')
                 continue;
             if (VARATT_IS_EXTERNAL(DatumGetPointer(toast_values[i])))
                 continue;        /* can't happen, toast_action would be 'p' */
             if (VARATT_IS_COMPRESSED(DatumGetPointer(toast_values[i])))
                 continue;
-            if (TupleDescAttr(tupleDesc, i)->attstorage != 'm')
+            if (att->attstorage != 'm' && att->attstorage != 'c' )
                 continue;
             if (toast_sizes[i] > biggest_size)
             {
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index 3bf308fe3b..e6e127645f 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -169,7 +169,7 @@ sub ParseHeader
 
                 $column{type}      = $atttype;
                 $column{name}      = $attname;
-                $column{is_varlen} = 1 if $is_varlen;
+                $column{is_varlen} = 1 if ($is_varlen);
 
                 foreach my $attopt (@attopts)
                 {
@@ -198,6 +198,10 @@ sub ParseHeader
                     {
                         $column{lookup} = $1;
                     }
+                    elsif ($attopt =~ /BKI_STORAGE\((\w)\)/)
+                    {
+                        $column{storage} = $1;
+                    }
                     else
                     {
                         die
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index be81094ffb..1d6818c96f 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -734,13 +734,16 @@ sub morph_row_for_pgattr
 
     $row->{attname} = $attname;
 
+    # copy explicitly specified storage
+    $row->{attstorage} = $attr->{storage} if ($attr->{storage});
+
     # Copy the type data from pg_type, and add some type-dependent items
     my $type = $types{$atttype};
 
     $row->{atttypid}   = $type->{oid};
     $row->{attlen}     = $type->{typlen};
     $row->{attbyval}   = $type->{typbyval};
-    $row->{attstorage} = $type->{typstorage};
+    $row->{attstorage} = $type->{typstorage} if (!$row->{attstorage});
     $row->{attalign}   = $type->{typalign};
 
     # set attndims if it's an array type
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 77be19175a..ac45c51286 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -435,7 +435,7 @@ needs_toast_table(Relation rel)
                 maxlength_unknown = true;
             else
                 data_length += maxlen;
-            if (att->attstorage != 'p')
+            if (att->attstorage != 'p' && att->attstorage != 'c')
                 has_toastable_attrs = true;
         }
     }
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 35a9ade059..a7b37d6e2b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1854,6 +1854,8 @@ storage_name(char c)
             return "EXTENDED";
         case 'e':
             return "EXTERNAL";
+        case 'c':
+            return "COMPRESS";
         default:
             return "???";
     }
diff --git a/src/include/catalog/genbki.h b/src/include/catalog/genbki.h
index 1b8e4e9e19..8e71d11964 100644
--- a/src/include/catalog/genbki.h
+++ b/src/include/catalog/genbki.h
@@ -40,6 +40,8 @@
  * OID-array field
  */
 #define BKI_LOOKUP(catalog)
+/* Indicates storage type of attribute */
+#define BKI_STORAGE(storage) 
 
 /* The following are never defined; they are here only for documentation. */
 
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index a6ec122389..3e02e908ef 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -164,19 +164,19 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
     /* NOTE: The following fields are not present in tuple descriptors. */
 
     /* Column-level access permissions */
-    aclitem        attacl[1] BKI_DEFAULT(_null_);
+    aclitem        attacl[1] BKI_DEFAULT(_null_) BKI_STORAGE(c);
 
     /* Column-level options */
-    text        attoptions[1] BKI_DEFAULT(_null_);
+    text        attoptions[1] BKI_DEFAULT(_null_) BKI_STORAGE(c);
 
     /* Column-level FDW options */
-    text        attfdwoptions[1] BKI_DEFAULT(_null_);
+    text        attfdwoptions[1] BKI_DEFAULT(_null_) BKI_STORAGE(c);
 
     /*
      * Missing value for added columns. This is a one element array which lets
      * us store a value of the attribute type here.
      */
-    anyarray    attmissingval BKI_DEFAULT(_null_);
+    anyarray    attmissingval BKI_DEFAULT(_null_) BKI_STORAGE(c);
 #endif
 } FormData_pg_attribute;
 
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 5d82ce09a6..46ad5c6d99 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -75,9 +75,9 @@ CATALOG(pg_class,1259,RelationRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83,Relat
 
 #ifdef CATALOG_VARLEN            /* variable-length fields start here */
     /* NOTE: These fields are not present in a relcache entry's rd_rel field. */
-    aclitem        relacl[1];        /* access permissions */
-    text        reloptions[1];    /* access-method-specific options */
-    pg_node_tree relpartbound;    /* partition bound node tree */
+    aclitem    relacl[1]        BKI_STORAGE(c);    /* access permissions */
+    text    reloptions[1]    BKI_STORAGE(c);    /* access-method-specific options */
+    pg_node_tree relpartbound BKI_STORAGE(c);/* partition bound node tree */
 #endif
 } FormData_pg_class;
 
-- 
2.16.3

From f3535de79039407511f815eecdba920b9b56408a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 8 Feb 2019 12:28:02 +0900
Subject: [PATCH 2/2] Let ALTER TABLE accept new storage class "compress"

This patch adds new choice of storage class in ALTER TABLE,
"compress", which means compress in-line, but don't make it
out-of-line. Likewise plain, toast relations once created won't be
removed even when no attribute that needs toast relation remains.
---
 src/backend/commands/tablecmds.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a7b37d6e2b..bd49a6f259 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6613,6 +6613,8 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc
         newstorage = 'x';
     else if (pg_strcasecmp(storagemode, "main") == 0)
         newstorage = 'm';
+    else if (pg_strcasecmp(storagemode, "compress") == 0)
+        newstorage = 'c';
     else
     {
         ereport(ERROR,
-- 
2.16.3