Thread: ALTER TABLE on system catalogs
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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 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
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
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
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
From 0849c0f1c5cbbba2bedd0dc841b564f67a32b612 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@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
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
From 14312f2b53edb053281b80cf3df16851ef474525 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 8 Feb 2019 11:22:57 +0900
Subject: [PATCH 1/2] 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/backend/commands/tablecmds.c | 2 ++
src/include/catalog/genbki.h | 2 ++
src/include/catalog/pg_attribute.h | 8 ++++----
src/include/catalog/pg_class.h | 6 +++---
8 files changed, 24 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/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 35a9ade059..a7b37d6e2b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1854,6 +1854,8 @@ storage_name(char c)
return "EXTENDED";
case 'e':
return "EXTERNAL";
+ case 'c':
+ return "COMPRESS";
default:
return "???";
}
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
From f3535de79039407511f815eecdba920b9b56408a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 8 Feb 2019 12:28:02 +0900
Subject: [PATCH 2/2] Let ALTER TABLE accept new storage class "compress"
This patch adds new choice of storage class in ALTER TABLE,
"compress", which means compress in-line, but don't make it
out-of-line. Likewise plain, toast relations once created won't be
removed even when no attribute that needs toast relation remains.
---
src/backend/commands/tablecmds.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a7b37d6e2b..bd49a6f259 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6613,6 +6613,8 @@ ATExecSetStorage(Relation rel, const char *colName, Node *newValue, LOCKMODE loc
newstorage = 'x';
else if (pg_strcasecmp(storagemode, "main") == 0)
newstorage = 'm';
+ else if (pg_strcasecmp(storagemode, "compress") == 0)
+ newstorage = 'c';
else
{
ereport(ERROR,
--
2.16.3
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
Saarbrücker Straße 37a, 10405 Berlin
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
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
Attachment
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
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