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(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <61666008-d1cd-7a66-33c8-215449f5d1b0(at)2ndquadrant(dot)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