Re: ALTER TABLE on system catalogs

On 2018-08-20 14:38:25 +0200, Peter Eisentraut wrote:
> On 20/08/2018 12:48, Andres Freund wrote:
> > On 2018-08-20 11:37:49 +0900, Michael Paquier wrote:
> >> On Fri, Jul 13, 2018 at 11:05:10AM +0200, Peter Eisentraut wrote:
> >>> After reviewing that thread, I think my patch would still be relevant.
> >>> Because the pending proposal is to not add TOAST tables to some catalogs
> >>> such as pg_attribute, so my original use case of allowing ALTER TABLE /
> >>> SET on system catalogs would still be broken for those tables.
> >>
> >> Something has happened in this area in the shape of 96cdeae, and the
> >> following catalogs do not have toast tables: pg_class, pg_index,
> >> pg_attribute and pg_largeobject*. While 0001 proposed upthread is not
> >> really relevant anymore because there is already a test to list which
> >> catalogs do not have a toast table. For 0002, indeed the patch is still
> >> seems relevant. The comment block at the beginning of
> >> create_toast_table should be updated.
> >
> > I still object to the approach in 0002.
>
> Do you have an alternative in mind?

One is to just not do anything. I'm not sure I'm on board with the goal
of changing things to make DDL on system tables more palatable.

If we want to do something, the first thing to do is to move the
if (shared_relation && !IsBootstrapProcessingMode())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("shared tables cannot be toasted after initdb")));
bit below the
/*
* Is it already toasted?
*/
and
/*
* Check to see whether the table actually needs a TOAST table.
*/
blocks. There's no point in erroring out when we'd actually not do
squat anyway.

By that point there's just a few remaining tables where you couldn't do
the ALTER TABLE.

After that I think there's two options: First is to just follow to the
rules and add toast tables for the relations that formally need them and
review the VACUUM FULL/CLUSTER codepath around relation swapping. That's
imo the best course.

Third option is to move those checks to the layers where they more
reasonably belong. IMO that's needs_toast_table(). I disfavor this, but
it's better than what you did IMO.

Greetings,

Andres Freund