Re: ALTER TABLE on system catalogs

Hi, I got redirected here by a kind suggestion by Tom.

At Fri, 28 Sep 2018 22:58:36 +0200, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
<61666008-d1cd-7a66-33c8-215449f5d1b0@2ndquadrant.com>
> On 21/08/2018 17:24, Andres Freund wrote:
> >> Attached is a patch that instead moves those special cases into
> >> needs_toast_table(), which was one of the options suggested by Andres.
> >> There were already similar checks there, so I guess this makes a bit of
> >> sense.
> > The big difference is that that then only takes effect when called with
> > check=true. The callers without it, importantly NewHeapCreateToastTable
> > & NewRelationCreateToastTable, then won't run into this check. But still
> > into the error (see below).
> 
> I don't follow.  The call to needs_toast_table() is not conditional on
> the check argument.  The check argument only checks that the correct
> lock level is passed in.
> 
> >> @@ -145,21 +146,6 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
> >>      ObjectAddress baseobject,
> >>                  toastobject;
> >>  
> >> -    /*
> >> -     * Toast table is shared if and only if its parent is.
> >> -     *
> >> -     * We cannot allow toasting a shared relation after initdb (because
> >> -     * there's no way to mark it toasted in other databases' pg_class).
> >> -     */
> >> -    shared_relation = rel->rd_rel->relisshared;
> >> -    if (shared_relation && !IsBootstrapProcessingMode())
> >> -        ereport(ERROR,
> >> -                (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> >> -                 errmsg("shared tables cannot be toasted after initdb")));
> > This error check imo shouldn't be removed, but moved down.
> 
> We could keep it, but it would probably be dead code since
> needs_toast_table() would return false for all shared tables anyway.

FWIW I agree to this point.

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, but couldn't we have 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.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 0849c0f1c5cbbba2bedd0dc841b564f67a32b612 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] 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/include/catalog/genbki.h         | 2 ++
 src/include/catalog/pg_attribute.h   | 8 ++++----
 src/include/catalog/pg_class.h       | 6 +++---
 7 files changed, 22 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/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