Horton

Thread

Commits

  1. Ignore attempts to add TOAST table to shared or catalog tables

  1. ALTER TABLE on system catalogs

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-06-27T20:31:30Z

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

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

  2. Re: ALTER TABLE on system catalogs

    Andres Freund <andres@anarazel.de> — 2018-06-27T20:37:33Z

    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

  3. Re: ALTER TABLE on system catalogs

    Michael Paquier <michael@paquier.xyz> — 2018-06-27T23:10:28Z

    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

  4. Re: ALTER TABLE on system catalogs

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-06-28T08:14:57Z

    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

  5. Re: ALTER TABLE on system catalogs

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-07-13T09:05:10Z

    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

  6. Re: ALTER TABLE on system catalogs

    Michael Paquier <michael@paquier.xyz> — 2018-08-20T02:37:49Z

    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

  7. Re: ALTER TABLE on system catalogs

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-08-20T10:43:20Z

    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

  8. Re: ALTER TABLE on system catalogs

    Andres Freund <andres@anarazel.de> — 2018-08-20T10:48:25Z

    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

  9. Re: ALTER TABLE on system catalogs

    Michael Paquier <michael@paquier.xyz> — 2018-08-20T12:11:36Z

    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

  10. Re: ALTER TABLE on system catalogs

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-08-20T12:38:25Z

    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

  11. Re: ALTER TABLE on system catalogs

    Andres Freund <andres@anarazel.de> — 2018-08-20T12:50:22Z

    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

  12. Re: ALTER TABLE on system catalogs

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-20T13:34:50Z

    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

  13. ALTER TABLE on system catalogs

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-08-21T15:04:41Z

    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

  14. Re: ALTER TABLE on system catalogs

    Andres Freund <andres@anarazel.de> — 2018-08-21T15:24:44Z

    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

  15. Re: ALTER TABLE on system catalogs

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-09-28T19:06:30Z

    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
https://postgr.es/m/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

  16. Re: ALTER TABLE on system catalogs

    Andres Freund <andres@anarazel.de> — 2018-09-28T19:15:03Z

    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
> https://postgr.es/m/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

  17. Re: ALTER TABLE on system catalogs

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-09-28T19:19:43Z

    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
> > https://postgr.es/m/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

  18. Re: ALTER TABLE on system catalogs

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-09-28T20:58:36Z

    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

  19. Re: ALTER TABLE on system catalogs

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2019-02-08T03:04:44Z

    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

  20. Re: ALTER TABLE on system catalogs

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2019-02-08T03:34:33Z

    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

  21. Re: ALTER TABLE on system catalogs

    Chris Travers <chris.travers@adjust.com> — 2019-02-14T12:42:29Z

    I have a couple of thoughts here.

On Fri, Feb 8, 2019 at 4:35 AM Kyotaro HORIGUCHI <
horiguchi.kyotaro@lab.ntt.co.jp> wrote:

> 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 guess there is a serious question why this is for internal use only.
Are there any particular cases where this would be problematic for those
who know what they are doing to set?  If not, maybe it should be included
in the docs?

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

I agree that we need to support setting options on tables in system
catalogs.  We have some cases where we want to disable autovacuum on some
table spaces, but set it aggressively on a couple system catalogs.
Currently this is not really doable in any sane way.

I also think that if the current catalogs violate expectations regarding
precondition checks this needs to be corrected rather than special-cased
and this seems to be the best way forward.

>
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>


-- 
Best Regards,
Chris Travers
Head of Database

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin

  22. Re: ALTER TABLE on system catalogs

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-02-14T15:35:45Z

    On 08/02/2019 04:04, Kyotaro HORIGUCHI wrote:
> 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.

That already exists: 'm': Value can be stored compressed inline

I agree that it seems we should be using that for those tables that
don't have a toast table.  Maybe the genbki stuff could do it
automatically for the appropriate catalogs.

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

  23. Re: ALTER TABLE on system catalogs

    John Naylor <john.naylor@2ndquadrant.com> — 2019-02-16T15:04:59Z

    On 2/8/19, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>  [v2 patch]

I poked this around a bit and found that this mechanism only works for
bootstrapped tables, as those are the only ones where we can scribble
on pg_attribute entries directly during bootstrap. As such, with this
patch we cannot perform ALTER TABLE for pg_index or pg_largeobject*
[1]. IMHO, it's not worth it to introduce new notation unless it
offers complete coverage. If we're willing to only solve the problem
for pg_class and pg_attribute, I'd rather mark the table rather than
the columns, because we already have visibility into CATALOG_VARLEN.
(rough example attached)

On 2/14/19, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> That already exists: 'm': Value can be stored compressed inline
>
> I agree that it seems we should be using that for those tables that
> don't have a toast table.  Maybe the genbki stuff could do it
> automatically for the appropriate catalogs.

The docs say:
(Actually, out-of-line storage will still be performed for such
columns, but only as a last resort when there is no other way to make
the row small enough to fit on a page.)

If we allow 'm' as an exception, would that interfere with this? My
demo patch has this just in case:

-            if (att->attstorage != 'p')
+            if (att->attstorage != 'p' &&
+                !(att->attstorage == 'm' && IsCatalogRelation(rel)))
                 has_toastable_attrs = true;

Here's another idea:  During initdb, do "ALTER TABLE ALTER COLUMN xyz
SET STORAGE MAIN;"
In initdb, we already pass "-O" to allow system table mods, so I think
we would have to just make sure this one statement doesn't try to add
a toast table. We could have genbki.pl emit a file with SQL statements
to cover all necessary tables/columns.


[1] https://www.postgresql.org/message-id/20180928190630.crt43sk5zd5p555h%40alvherre.pgsql

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

  24. Re: ALTER TABLE on system catalogs

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-03-19T10:31:43Z

    On 2019-02-08 04:04, Kyotaro HORIGUCHI wrote:
> Hi, I got redirected here by a kind suggestion by Tom.

I have committed my patch, which also addresses the issue you had in
your other thread.

I rest of these discussions have merit but are not really dependent on
my patch.

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

  25. Re: ALTER TABLE on system catalogs

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2019-03-19T11:00:01Z

    Sorry overlooked this.

At Thu, 14 Feb 2019 16:35:45 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in <84c6bcc4-026f-a44f-5726-e8035f4d197a@2ndquadrant.com>
> On 08/02/2019 04:04, Kyotaro HORIGUCHI wrote:
> > 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.
> 
> That already exists: 'm': Value can be stored compressed inline

It works differently. 'm' doesn't prevent toast table from
creation, and 'c' does. But I agree that your patch fixes
that. My point was just seeming consistency in narrow area.

> I agree that it seems we should be using that for those tables that
> don't have a toast table.  Maybe the genbki stuff could do it
> automatically for the appropriate catalogs.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center