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
Attachments:
0001-Add-test-for-system-catalog-TOAST-tables.patchtext/plain; charset=UTF-8; name=0001-Add-test-for-system-catalog-TOAST-tables.patch; x-mac-creator=0; x-mac-type=0Download
From fa75f6573a37f4065f0ba16b1ef8a55e8b933af6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Fri, 18 May 2018 19:07:53 -0400
Subject: [PATCH 1/5] Add test for system catalog TOAST tables
This records which system catalogs are supposed to have a TOAST table.
---
src/test/regress/expected/misc_sanity.out | 22 ++++++++++++++++++++++
src/test/regress/sql/misc_sanity.sql | 10 ++++++++++
2 files changed, 32 insertions(+)
diff --git a/src/test/regress/expected/misc_sanity.out b/src/test/regress/expected/misc_sanity.out
index 5aaae6c39f..a1ea485a15 100644
--- a/src/test/regress/expected/misc_sanity.out
+++ b/src/test/regress/expected/misc_sanity.out
@@ -76,3 +76,25 @@ NOTICE: pg_database contains unpinned initdb-created object(s)
NOTICE: pg_extension contains unpinned initdb-created object(s)
NOTICE: pg_rewrite contains unpinned initdb-created object(s)
NOTICE: pg_tablespace contains unpinned initdb-created object(s)
+-- **************** general ****************
+-- check which system catalogs have toast tables
+SELECT relname
+FROM pg_class
+WHERE relnamespace = 'pg_catalog'::regnamespace AND reltoastrelid <> 0
+ORDER BY 1;
+ relname
+--------------------
+ pg_attrdef
+ pg_constraint
+ pg_db_role_setting
+ pg_description
+ pg_proc
+ pg_rewrite
+ pg_seclabel
+ pg_shdescription
+ pg_shseclabel
+ pg_statistic
+ pg_statistic_ext
+ pg_trigger
+(12 rows)
+
diff --git a/src/test/regress/sql/misc_sanity.sql b/src/test/regress/sql/misc_sanity.sql
index b921117fa5..b9adcbe4e0 100644
--- a/src/test/regress/sql/misc_sanity.sql
+++ b/src/test/regress/sql/misc_sanity.sql
@@ -72,3 +72,13 @@
end if;
end loop;
end$$;
+
+
+-- **************** general ****************
+
+-- check which system catalogs have toast tables
+
+SELECT relname
+FROM pg_class
+WHERE relnamespace = 'pg_catalog'::regnamespace AND reltoastrelid <> 0
+ORDER BY 1;
base-commit: 2e61c50785a0dca6ed30a1a5522457b18bbb5498
--
2.18.0
0002-Ignore-attempts-to-add-TOAST-table-to-shared-or-cata.patchtext/plain; charset=UTF-8; name=0002-Ignore-attempts-to-add-TOAST-table-to-shared-or-cata.patch; x-mac-creator=0; x-mac-type=0Download
From 029fa7e97c8219f13b12ebd5026beb5ffd5ba684 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Fri, 18 May 2018 17:05:27 -0400
Subject: [PATCH 2/5] Ignore attempts to add TOAST table to shared or catalog
tables
Running ALTER TABLE on any table will check if a TOAST table needs to be
added. On shared tables, this would previously fail, thus effectively
disabling ALTER TABLE for those tables. On (non-shared) system
catalogs, on the other hand, it would add a TOAST table, even though we
don't really want TOAST tables on some system catalogs. In some cases,
it would also fail with an error "AccessExclusiveLock required to add
toast table.", depending on what locks the ALTER TABLE actions had
already taken.
So instead, just ignore attempts to add TOAST tables to such tables,
outside of bootstrap mode, pretending they don't need one.
This allows running ALTER TABLE on such tables without messing up the
TOAST situation. (All this still requires allow_system_table_mods,
which is independent of this.)
---
src/backend/catalog/toasting.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 3baaa08238..97a215d2c1 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -17,6 +17,7 @@
#include "access/tuptoaster.h"
#include "access/xact.h"
#include "catalog/binary_upgrade.h"
+#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/heap.h"
#include "catalog/index.h"
@@ -153,9 +154,10 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
*/
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")));
+ return false;
+
+ if (IsCatalogRelation(rel) && !IsBootstrapProcessingMode())
+ return false;
/* It's mapped if and only if its parent is, too */
mapped_relation = RelationIsMapped(rel);
--
2.18.0
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
Attachments:
v2-0001-Ignore-attempts-to-add-TOAST-table-to-shared-or-c.patchtext/plain; charset=UTF-8; name=v2-0001-Ignore-attempts-to-add-TOAST-table-to-shared-or-c.patch; x-mac-creator=0; x-mac-type=0Download
From c83d2f64195d93991c129812d91dfc6118bae44b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Fri, 18 May 2018 17:05:27 -0400
Subject: [PATCH v2 1/2] Ignore attempts to add TOAST table to shared or
catalog tables
Running ALTER TABLE on any table will check if a TOAST table needs to be
added. On shared tables, this would previously fail, thus effectively
disabling ALTER TABLE for those tables. On (non-shared) system
catalogs, on the other hand, it would add a TOAST table, even though we
don't really want TOAST tables on some system catalogs. In some cases,
it would also fail with an error "AccessExclusiveLock required to add
toast table.", depending on what locks the ALTER TABLE actions had
already taken.
So instead, just ignore attempts to add TOAST tables to such tables,
outside of bootstrap mode, pretending they don't need one.
This allows running ALTER TABLE on such tables without messing up the
TOAST situation. (All this still requires allow_system_table_mods,
which is independent of this.)
---
src/backend/catalog/toasting.c | 42 +++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 3baaa08238..c94d7ef9a5 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -17,6 +17,7 @@
#include "access/tuptoaster.h"
#include "access/xact.h"
#include "catalog/binary_upgrade.h"
+#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/heap.h"
#include "catalog/index.h"
@@ -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")));
-
- /* It's mapped if and only if its parent is, too */
- mapped_relation = RelationIsMapped(rel);
-
/*
* Is it already toasted?
*/
@@ -259,6 +245,12 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
binary_upgrade_next_toast_pg_type_oid = InvalidOid;
}
+ /* Toast table is shared if and only if its parent is. */
+ shared_relation = rel->rd_rel->relisshared;
+
+ /* It's mapped if and only if its parent is, too */
+ mapped_relation = RelationIsMapped(rel);
+
toast_relid = heap_create_with_catalog(toast_relname,
namespaceid,
rel->rd_rel->reltablespace,
@@ -398,7 +390,6 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
* (1) there are any toastable attributes, and (2) the maximum length
* of a tuple could exceed TOAST_TUPLE_THRESHOLD. (We don't want to
* create a toast table for something like "f1 varchar(20)".)
- * No need to create a TOAST table for partitioned tables.
*/
static bool
needs_toast_table(Relation rel)
@@ -410,9 +401,28 @@ needs_toast_table(Relation rel)
int32 tuple_length;
int i;
+ /*
+ * No need to create a TOAST table for partitioned tables.
+ */
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
return false;
+ /*
+ * We cannot allow toasting a shared relation after initdb (because
+ * there's no way to mark it toasted in other databases' pg_class).
+ */
+ if (rel->rd_rel->relisshared && !IsBootstrapProcessingMode())
+ return false;
+
+ /*
+ * Ignore attempts to create toast tables on catalog tables after initdb.
+ * Which catalogs get toast tables is explicitly chosen in
+ * catalog/toasting.h. (We could get here via some ALTER TABLE command if
+ * the catalog doesn't have a toast table.)
+ */
+ if (IsCatalogRelation(rel) && !IsBootstrapProcessingMode())
+ return false;
+
tupdesc = rel->rd_att;
for (i = 0; i < tupdesc->natts; i++)
--
2.18.0
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
Attachments:
0001-Explicitly-mark-some-attributes-in-catalog-as-no-nee.patchtext/x-patch; charset=us-asciiDownload
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
Attachments:
v2-0001-Explicitly-mark-some-attributes-in-catalog-as-no-nee.patchtext/x-patch; charset=us-asciiDownload
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
v2-0002-Let-ALTER-TABLE-accept-new-storage-class-compress.patchtext/x-patch; charset=us-asciiDownload
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
Import Notes
Reply to msg id not found: 20190208.120331.167280496.horiguchi.kyotaro@lab.ntt.co.jp
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
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]: /messages/by-id/20180928190630.crt43sk5zd5p555h@alvherre.pgsql
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]: /messages/by-id/20180928190630.crt43sk5zd5p555h@alvherre.pgsql
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
allow-alter-table-on-pg_class-pg_attribute.patchtext/x-patch; charset=US-ASCII; name=allow-alter-table-on-pg_class-pg_attribute.patchDownload
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index 3bf308fe3b..885df79f92 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -124,6 +124,7 @@ sub ParseHeader
$catalog{rowtype_oid_macro} = '';
}
$catalog{schema_macro} = /BKI_SCHEMA_MACRO/ ? 1 : 0;
+ $catalog{no_toast} = /BKI_NO_TOAST/ ? 1 : 0;
$declaring_attributes = 1;
}
elsif ($is_client_code)
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 4935e00fb2..b4777e5e5a 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -658,7 +658,7 @@ sub gen_pg_attribute
$row{attnum} = $attnum;
$row{attrelid} = $table->{relation_oid};
- morph_row_for_pgattr(\%row, $schema, $attr, $priornotnull);
+ morph_row_for_pgattr($table, \%row, $schema, $attr, $priornotnull);
$priornotnull &= ($row{attnotnull} eq 't');
# If it's bootstrapped, put an entry in postgres.bki.
@@ -691,7 +691,7 @@ sub gen_pg_attribute
$row{attrelid} = $table->{relation_oid};
$row{attstattarget} = '0';
- morph_row_for_pgattr(\%row, $schema, $attr, 1);
+ morph_row_for_pgattr($table, \%row, $schema, $attr, 1);
print_bki_insert(\%row, $schema);
}
}
@@ -707,7 +707,7 @@ sub gen_pg_attribute
# Any value not handled here must be supplied by caller.
sub morph_row_for_pgattr
{
- my ($row, $pgattr_schema, $attr, $priornotnull) = @_;
+ my ($table, $row, $pgattr_schema, $attr, $priornotnull) = @_;
my $attname = $attr->{name};
my $atttype = $attr->{type};
@@ -719,9 +719,17 @@ sub morph_row_for_pgattr
$row->{atttypid} = $type->{oid};
$row->{attlen} = $type->{typlen};
$row->{attbyval} = $type->{typbyval};
- $row->{attstorage} = $type->{typstorage};
$row->{attalign} = $type->{typalign};
+ if ($table->{no_toast} && $attr->{is_varlen})
+ {
+ $row->{attstorage} = 'm';
+ }
+ else
+ {
+ $row->{attstorage} = $type->{typstorage};
+ }
+
# set attndims if it's an array type
$row->{attndims} = $type->{typcategory} eq 'A' ? '1' : '0';
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 77be19175a..37e494b205 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -18,6 +18,7 @@
#include "access/tuptoaster.h"
#include "access/xact.h"
#include "catalog/binary_upgrade.h"
+#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/heap.h"
#include "catalog/index.h"
@@ -435,7 +436,8 @@ needs_toast_table(Relation rel)
maxlength_unknown = true;
else
data_length += maxlen;
- if (att->attstorage != 'p')
+ if (att->attstorage != 'p' &&
+ !(att->attstorage == 'm' && IsCatalogRelation(rel)))
has_toastable_attrs = true;
}
}
diff --git a/src/include/catalog/genbki.h b/src/include/catalog/genbki.h
index 1b8e4e9e19..1984bbc426 100644
--- a/src/include/catalog/genbki.h
+++ b/src/include/catalog/genbki.h
@@ -27,6 +27,7 @@
#define BKI_SHARED_RELATION
#define BKI_ROWTYPE_OID(oid,oidmacro)
#define BKI_SCHEMA_MACRO
+#define BKI_NO_TOAST
/* Options that may appear after an attribute (on the same line) */
#define BKI_FORCE_NULL
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index a6ec122389..66120244f5 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -34,7 +34,7 @@
* You may need to change catalog/genbki.pl as well.
* ----------------
*/
-CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,AttributeRelation_Rowtype_Id) BKI_SCHEMA_MACRO
+CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,AttributeRelation_Rowtype_Id) BKI_SCHEMA_MACRO BKI_NO_TOAST
{
Oid attrelid; /* OID of relation containing this attribute */
NameData attname; /* name of attribute */
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 5d82ce09a6..151b195f02 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -26,7 +26,7 @@
* typedef struct FormData_pg_class
* ----------------
*/
-CATALOG(pg_class,1259,RelationRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83,RelationRelation_Rowtype_Id) BKI_SCHEMA_MACRO
+CATALOG(pg_class,1259,RelationRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83,RelationRelation_Rowtype_Id) BKI_SCHEMA_MACRO BKI_NO_TOAST
{
Oid oid; /* oid */
NameData relname; /* class name */
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