Hi, I got redirected here by a kind suggestion by Tom.
At Fri, 28 Sep 2018 22:58:36 +0200, Peter Eisentraut
<peter.eisentr...@2ndquadrant.com> wrote in
<61666008-d1cd-7a66-33c8-215449f5d...@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.kyot...@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
