ALTER TABLE on system catalogs

Hi,

On 2018-06-27 22:31:30 +0200, Peter Eisentraut wrote:

ALTER TABLE on system catalogs is occasionally useful, for example

ALTER TABLE pg_attribute SET (autovacuum_vacuum_scale_factor=0);

However, this doesn't actually work. The above command produces

ERROR: AccessExclusiveLock required to add toast table.

If it's a shared catalog, it will produce

ERROR: shared tables cannot be toasted after initdb

In other cases it will work but then silently add a TOAST table to a
catalog, which I think we don't want.

The problem is that for (almost) any ALTER TABLE command, it afterwards
checks if the just-altered table now needs a TOAST table and tries to
add it if so, which will either fail or add a TOAST table that we don't
want.

I propose that we instead silently ignore attempts to add TOAST tables
to shared and system catalogs after bootstrapping.

That seems like an extremely bad idea to me. I'd rather go ahead and
just add the necessary toast tables than silently violate preconditions
that code assumes are fulfilled.

This fixes the above
issues. I have attached a patch for this, and also a test that
enshrines which system catalogs are supposed to have TOAST tables.

As an alternative, I tried to modify the ALTER TABLE code to avoid the
try-to-add-TOAST-table path depending on what ALTER TABLE actions were
done, but that seemed incredibly more complicated and harder to maintain
in the long run.

(You still need allow_system_table_mods=on for all of this. Perhaps
that's also worth revisiting, but it's a separate issue.)

I think we should make it harder, not easier to modify system
catalogs. In fact, I think we should require allow_system_table_mods=on
on catalog DML, not just DDL.

I think we can add explicit exceptions - like changing autovac settings
- however.

Greetings,

Andres Freund

On Wed, Jun 27, 2018 at 01:37:33PM -0700, Andres Freund wrote:

On 2018-06-27 22:31:30 +0200, Peter Eisentraut wrote:

I propose that we instead silently ignore attempts to add TOAST tables
to shared and system catalogs after bootstrapping.

That seems like an extremely bad idea to me. I'd rather go ahead and
just add the necessary toast tables than silently violate preconditions
that code assumes are fulfilled.

Agreed. Joe Conway was working on a patch to do exactly that. I was
personally looking for the possibility of having one with pg_authid in
v12 :)
--
Michael

On 6/28/18 01:10, Michael Paquier wrote:

On Wed, Jun 27, 2018 at 01:37:33PM -0700, Andres Freund wrote:

On 2018-06-27 22:31:30 +0200, Peter Eisentraut wrote:

I propose that we instead silently ignore attempts to add TOAST tables
to shared and system catalogs after bootstrapping.

That seems like an extremely bad idea to me. I'd rather go ahead and
just add the necessary toast tables than silently violate preconditions
that code assumes are fulfilled.

Agreed. Joe Conway was working on a patch to do exactly that. I was
personally looking for the possibility of having one with pg_authid in
v12 :)

OK, that would change things a bit, in that the silent addition of a
TOAST table would no longer be a problem, but it wouldn't fix the other
scenarios that end up in an error. If such a patch is forthcoming, we
can revisit this again afterwards.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 28.06.18 10:14, Peter Eisentraut wrote:

On 6/28/18 01:10, Michael Paquier wrote:

On Wed, Jun 27, 2018 at 01:37:33PM -0700, Andres Freund wrote:

On 2018-06-27 22:31:30 +0200, Peter Eisentraut wrote:

I propose that we instead silently ignore attempts to add TOAST tables
to shared and system catalogs after bootstrapping.

That seems like an extremely bad idea to me. I'd rather go ahead and
just add the necessary toast tables than silently violate preconditions
that code assumes are fulfilled.

Agreed. Joe Conway was working on a patch to do exactly that. I was
personally looking for the possibility of having one with pg_authid in
v12 :)

OK, that would change things a bit, in that the silent addition of a
TOAST table would no longer be a problem, but it wouldn't fix the other
scenarios that end up in an error. If such a patch is forthcoming, we
can revisit this again afterwards.

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.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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. Some tests would also be nice to
have.
--
Michael

On 20/08/2018 04:37, Michael Paquier wrote:

For 0002, indeed the patch is still
seems relevant. The comment block at the beginning of
create_toast_table should be updated. Some tests would also be nice to
have.

Tests would require enabling allow_system_table_mods, which is
PGC_POSTMASTER, so we couldn't do it inside the normal regression test
suite. I'm not sure setting up a whole new test suite for this is worth it.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

Greetings,

Andres Freund

On Mon, Aug 20, 2018 at 12:43:20PM +0200, Peter Eisentraut wrote:

Tests would require enabling allow_system_table_mods, which is
PGC_POSTMASTER, so we couldn't do it inside the normal regression test
suite. I'm not sure setting up a whole new test suite for this is worth it.

I forgot this point. Likely that's not worth it.
--
Michael

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?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Andres Freund <andres@anarazel.de> writes:

On 2018-08-20 14:38:25 +0200, Peter Eisentraut wrote:

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.

FWIW, I agree with doing as little as possible here. I'd be on board
with Andres' suggestion of just swapping the two code stanzas so that
the no-op case doesn't error out. As soon as you go beyond that, you
are in wildly unsafe and untested territory.

regards, tom lane

On 20/08/2018 15:34, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2018-08-20 14:38:25 +0200, Peter Eisentraut wrote:

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.

FWIW, I agree with doing as little as possible here. I'd be on board
with Andres' suggestion of just swapping the two code stanzas so that
the no-op case doesn't error out. As soon as you go beyond that, you
are in wildly unsafe and untested territory.

That doesn't solve the original problem, which is being able to set
reloptions on pg_attribute, because pg_attribute doesn't have a toast
table but would need one according to existing rules.

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.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-08-21 17:04:41 +0200, Peter Eisentraut wrote:

On 20/08/2018 15:34, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2018-08-20 14:38:25 +0200, Peter Eisentraut wrote:

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.

FWIW, I agree with doing as little as possible here. I'd be on board
with Andres' suggestion of just swapping the two code stanzas so that
the no-op case doesn't error out. As soon as you go beyond that, you
are in wildly unsafe and untested territory.

That doesn't solve the original problem, which is being able to set
reloptions on pg_attribute, because pg_attribute doesn't have a toast
table but would need one according to existing rules.

I still think it's wrong to work around this than to actually fix the
issue with pg_attribute not having a toast table.

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).

@@ -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.

Greetings,

Andres Freund

On 2018-Aug-21, Andres Freund wrote:

On 2018-08-21 17:04:41 +0200, Peter Eisentraut wrote:

That doesn't solve the original problem, which is being able to set
reloptions on pg_attribute, because pg_attribute doesn't have a toast
table but would need one according to existing rules.

I still think it's wrong to work around this than to actually fix the
issue with pg_attribute not having a toast table.

FWIW I'm still bothered by the inability to move pg_largeobject to a
different tablespace, per
/messages/by-id/20160502163033.GA15384@alvherre.pgsql
While that needs even more work (preservability across pg_dump for one),
this item here would be one thing to fix.

Also, I don't quite understand what's so horrible about setting
autovacuum options for system catalogs, including those that don't have
toast tables. That seems a pretty general feature that needs fixing,
too.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-09-28 16:06:30 -0300, Alvaro Herrera wrote:

On 2018-Aug-21, Andres Freund wrote:

On 2018-08-21 17:04:41 +0200, Peter Eisentraut wrote:

That doesn't solve the original problem, which is being able to set
reloptions on pg_attribute, because pg_attribute doesn't have a toast
table but would need one according to existing rules.

I still think it's wrong to work around this than to actually fix the
issue with pg_attribute not having a toast table.

FWIW I'm still bothered by the inability to move pg_largeobject to a
different tablespace, per
/messages/by-id/20160502163033.GA15384@alvherre.pgsql
While that needs even more work (preservability across pg_dump for one),
this item here would be one thing to fix.

Also, I don't quite understand what's so horrible about setting
autovacuum options for system catalogs, including those that don't have
toast tables. That seems a pretty general feature that needs fixing,
too.

I'm not sure what that has to do with my point? What I'm saying is that
we shouldn't have some weird "should have a toast table but doesn't"
exception, not that we shouldn't allow any sort of DDL on catalogs.

Greetings,

Andres Freund

On 2018-Sep-28, Andres Freund wrote:

On 2018-09-28 16:06:30 -0300, Alvaro Herrera wrote:

On 2018-Aug-21, Andres Freund wrote:

I still think it's wrong to work around this than to actually fix the
issue with pg_attribute not having a toast table.

FWIW I'm still bothered by the inability to move pg_largeobject to a
different tablespace, per
/messages/by-id/20160502163033.GA15384@alvherre.pgsql
While that needs even more work (preservability across pg_dump for one),
this item here would be one thing to fix.

Also, I don't quite understand what's so horrible about setting
autovacuum options for system catalogs, including those that don't have
toast tables. That seems a pretty general feature that needs fixing,
too.

I'm not sure what that has to do with my point? What I'm saying is that
we shouldn't have some weird "should have a toast table but doesn't"
exception, not that we shouldn't allow any sort of DDL on catalogs.

Well, the subtext in your argument seemed to be "let's just add a
toast table to pg_attribute and then we don't need any of this". I'm
just countering that if we don't have toast tables for some catalogs,
it's because that's something we've already beaten to death; so rather
than continue to beat it, we should discuss alternative ways to attack
the resulting side-effects.

I mean, surely adding a toast table to pg_largeobject would be
completely silly. Yet if we leave this code unchanged, trying to move
it to a different tablespace would "blow up" in a way.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

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