Re: missing toast table for pg_policy

[Andres Freund]

On 2018-07-20 08:56:32 +0900, Michael Paquier wrote:
> On Thu, Jul 19, 2018 at 04:50:06PM -0700, Andres Freund wrote:
> > On 2018-07-20 08:46:50 +0900, Michael Paquier wrote:
> >> On Thu, Jul 19, 2018 at 07:18:32PM -0400, Tom Lane wrote:
> >> I have found the argument about circular dependencies rather sensible
> >> FWIW.  So at the end it seems to me that we would not want to add toast
> >> tables for those catalogs.
> > 
> > As argued a fair bit ago, I think that isn't actually an issue: As long
> > as we keep the boostrap relevant fields from being toasted, there's no
> > issue with circularlity. Given the initial contents are defined to be
> > static or live in relmapper there's no danger of that accidentally
> > happening.
> 
> I still have some doubts about issues hidden behind our backs with a
> knife ready to hit...  The patch committed is already a good cut I
> think, and addresses the original complaints from Joe and me.

I disagree fairly strongly.  I think that's a half-assed way to address
the concerns raised in this thread.  All but guarantees that we'll have
this discussion again.


> >> That could be nice, but separate from the fact of adding a toast table
> >> to it?
> > 
> > Yea, that seems mostly independent.
> 
> Please don't tell me that I forgot to bump CATALOG_VERSION_NO, and that
> it needs to be bumped..

You mean I shouldn't say that it needs to because you already know?
Because obviously, yes, that's required and appears to be missing?

Greetings,

Andres Freund

Re: missing toast table for pg_policy

[Michael Paquier]

On Thu, Jul 19, 2018 at 04:50:06PM -0700, Andres Freund wrote:
> On 2018-07-20 08:46:50 +0900, Michael Paquier wrote:
>> On Thu, Jul 19, 2018 at 07:18:32PM -0400, Tom Lane wrote:
>> I have found the argument about circular dependencies rather sensible
>> FWIW.  So at the end it seems to me that we would not want to add toast
>> tables for those catalogs.
> 
> As argued a fair bit ago, I think that isn't actually an issue: As long
> as we keep the boostrap relevant fields from being toasted, there's no
> issue with circularlity. Given the initial contents are defined to be
> static or live in relmapper there's no danger of that accidentally
> happening.

I still have some doubts about issues hidden behind our backs with a
knife ready to hit...  The patch committed is already a good cut I
think, and addresses the original complaints from Joe and me.

>> That could be nice, but separate from the fact of adding a toast table
>> to it?
> 
> Yea, that seems mostly independent.

Please don't tell me that I forgot to bump CATALOG_VERSION_NO, and that
it needs to be bumped..
--
Michael


signature.asc
Description: PGP signature

Re: missing toast table for pg_policy

[Andres Freund]

On 2018-07-20 08:46:50 +0900, Michael Paquier wrote:
> On Thu, Jul 19, 2018 at 07:18:32PM -0400, Tom Lane wrote:
> > Andres Freund  writes:
> >> FWIW, I was off the last few days. I personally think the reasoning to
> >> leave out pg_class, pg_index etc. is bad.  We should just make them work
> >> and create toast tables as well.
> > 
> > If it's easy to make those work and keep them working, then sure, but
> > I have my doubts.  I remain afraid of circular accesses occurring only
> > in strange corner cases ...
> 
> I have found the argument about circular dependencies rather sensible
> FWIW.  So at the end it seems to me that we would not want to add toast
> tables for those catalogs.

As argued a fair bit ago, I think that isn't actually an issue: As long
as we keep the boostrap relevant fields from being toasted, there's no
issue with circularlity. Given the initial contents are defined to be
static or live in relmapper there's no danger of that accidentally
happening.


> >> It's definitely not right that "those
> >> relations have no reason to use a toast table anyway." as the commit
> >> message states, given relacl, reloptions and relpartbound.
> > 
> > I wonder whether we shouldn't have handled ACLs through something more
> > like the pg_description solution, ie keep them all in one catalog with
> > a (classoid, objoid) primary key.
> 
> That could be nice, but separate from the fact of adding a toast table
> to it?

Yea, that seems mostly independent.

Greetings,

Andres Freund

Re: missing toast table for pg_policy

[Michael Paquier]

On Thu, Jul 19, 2018 at 07:18:32PM -0400, Tom Lane wrote:
> Andres Freund  writes:
>> FWIW, I was off the last few days. I personally think the reasoning to
>> leave out pg_class, pg_index etc. is bad.  We should just make them work
>> and create toast tables as well.
> 
> If it's easy to make those work and keep them working, then sure, but
> I have my doubts.  I remain afraid of circular accesses occurring only
> in strange corner cases ...

I have found the argument about circular dependencies rather sensible
FWIW.  So at the end it seems to me that we would not want to add toast
tables for those catalogs.

>> It's definitely not right that "those
>> relations have no reason to use a toast table anyway." as the commit
>> message states, given relacl, reloptions and relpartbound.
> 
> I wonder whether we shouldn't have handled ACLs through something more
> like the pg_description solution, ie keep them all in one catalog with
> a (classoid, objoid) primary key.

That could be nice, but separate from the fact of adding a toast table
to it?
--
Michael


signature.asc
Description: PGP signature

Re: missing toast table for pg_policy

[Andres Freund]

Hi,

On 2018-07-20 07:49:11 +0900, Michael Paquier wrote:
> On Wed, Jul 18, 2018 at 01:02:35PM +0900, Michael Paquier wrote:
> > So, I have been playing with the previous patch and tortured it through
> > a couple of upgrade scenarios, where it proves to work.  Attached is an
> > updated patch which adds toast tables except for pg_class, pg_attribute,
> > pg_index and pg_largeobject* with a proposal of commit message.  Any
> > objections for the commit of this stuff?
> 
> Committed.

FWIW, I was off the last few days. I personally think the reasoning to
leave out pg_class, pg_index etc. is bad.  We should just make them work
and create toast tables as well.  It's definitely not right that "those
relations have no reason to use a toast table anyway." as the commit
message states, given relacl, reloptions and relpartbound.

Greetings,

Andres Freund

Re: missing toast table for pg_policy

[Michael Paquier]

On Wed, Jul 18, 2018 at 01:02:35PM +0900, Michael Paquier wrote:
> So, I have been playing with the previous patch and tortured it through
> a couple of upgrade scenarios, where it proves to work.  Attached is an
> updated patch which adds toast tables except for pg_class, pg_attribute,
> pg_index and pg_largeobject* with a proposal of commit message.  Any
> objections for the commit of this stuff?

Committed.
--
Michael


signature.asc
Description: PGP signature

Re: missing toast table for pg_policy

[John Naylor]

On 7/17/18, Michael Paquier  wrote:
> [... digging ...]
> This comes from get_rel_infos where large objects are treated as user
> data.  Rather than the comment you added, I would rather do the
> following:
> "Large object catalogs and toast tables are mutually exclusive and large
> object data is handled as user data by pg_upgrade, which would cause
> failures."

Sounds good to me.

-John Naylor

Re: missing toast table for pg_policy

[Michael Paquier]

On Tue, Jul 17, 2018 at 06:03:26PM +0900, Michael Paquier wrote:
> On Tue, Jul 17, 2018 at 02:55:07PM +0700, John Naylor wrote:
>> I didn't dig deeper, since TOAST and the large object facility are
>> mutually exclusive so there shouldn't be a toast table here anyway.
>> Hope this helps.
> 
> [... digging ...]
> This comes from get_rel_infos where large objects are treated as user
> data.  Rather than the comment you added, I would rather do the
> following:
> "Large object catalogs and toast tables are mutually exclusive and large
> object data is handled as user data by pg_upgrade, which would cause
> failures."

So, I have been playing with the previous patch and tortured it through
a couple of upgrade scenarios, where it proves to work.  Attached is an
updated patch which adds toast tables except for pg_class, pg_attribute,
pg_index and pg_largeobject* with a proposal of commit message.  Any
objections for the commit of this stuff?
--
Michael
From 816dc770bc6bd2914b00ded5f1523265cf6c6d48 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Wed, 18 Jul 2018 12:56:21 +0900
Subject: [PATCH] Add toast tables to most system catalogs

It has been project policy to create toast tables only for those catalogs
that might reasonably need one.  Since this judgment call can change over
time, just create one for every catalog, as this can be useful when
creating rather-long entries in catalogs, with recent examples being in
the shape of policy expressions or customly-formatted SCRAM verifiers.

To prevent circular dependencies and to avoid adding complexity to VACUUM
FULL logic, exclude pg_class, pg_attribute, and pg_index.  Also, to
prevent pg_upgrade from seeing a non-empty new cluster, exclude
pg_largeobject and pg_largeobject_metadata from the set as large object
data is handled as user data.  Those relations have no reason to use a
toast table anyway.

Author: Joe Conway, John Naylor
Reviewed-by: Michael Paquier, Tom Lane
Discussion: https://postgr.es/m/84ddff04-f122-784b-b6c5-353680449...@joeconway.com
---
 src/backend/catalog/catalog.c | 18 -
 src/include/catalog/toasting.h| 40 +++-
 src/test/regress/expected/misc_sanity.out | 80 +++
 src/test/regress/sql/misc_sanity.sql  | 12 ++--
 4 files changed, 82 insertions(+), 68 deletions(-)

diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index a42155eeea..6061428bcc 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -253,12 +253,24 @@ IsSharedRelation(Oid relationId)
 		relationId == SubscriptionNameIndexId)
 		return true;
 	/* These are their toast tables and toast indexes (see toasting.h) */
-	if (relationId == PgShdescriptionToastTable ||
-		relationId == PgShdescriptionToastIndex ||
+	if (relationId == PgAuthidToastTable ||
+		relationId == PgAuthidToastIndex ||
+		relationId == PgDatabaseToastTable ||
+		relationId == PgDatabaseToastIndex ||
 		relationId == PgDbRoleSettingToastTable ||
 		relationId == PgDbRoleSettingToastIndex ||
+		relationId == PgPlTemplateToastTable ||
+		relationId == PgPlTemplateToastIndex ||
+		relationId == PgReplicationOriginToastTable ||
+		relationId == PgReplicationOriginToastIndex ||
+		relationId == PgShdescriptionToastTable ||
+		relationId == PgShdescriptionToastIndex ||
 		relationId == PgShseclabelToastTable ||
-		relationId == PgShseclabelToastIndex)
+		relationId == PgShseclabelToastIndex ||
+		relationId == PgSubscriptionToastTable ||
+		relationId == PgSubscriptionToastIndex ||
+		relationId == PgTablespaceToastTable ||
+		relationId == PgTablespaceToastIndex)
 		return true;
 	return false;
 }
diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h
index 3db39b8f86..f259890e43 100644
--- a/src/include/catalog/toasting.h
+++ b/src/include/catalog/toasting.h
@@ -46,25 +46,59 @@ extern void BootstrapToastTable(char *relName,
  */
 
 /* normal catalogs */
+DECLARE_TOAST(pg_aggregate, 4159, 4160);
 DECLARE_TOAST(pg_attrdef, 2830, 2831);
+DECLARE_TOAST(pg_collation, 4161, 4162);
 DECLARE_TOAST(pg_constraint, 2832, 2833);
+DECLARE_TOAST(pg_default_acl, 4143, 4144);
 DECLARE_TOAST(pg_description, 2834, 2835);
+DECLARE_TOAST(pg_event_trigger, 4145, 4146);
+DECLARE_TOAST(pg_extension, 4147, 4148);
+DECLARE_TOAST(pg_foreign_data_wrapper, 4149, 4150);
+DECLARE_TOAST(pg_foreign_server, 4151, 4152);
+DECLARE_TOAST(pg_foreign_table, 4153, 4154);
+DECLARE_TOAST(pg_init_privs, 4155, 4156);
+DECLARE_TOAST(pg_language, 4157, 4158);
+DECLARE_TOAST(pg_namespace, 4163, 4164);
+DECLARE_TOAST(pg_partitioned_table, 4165, 4166);
+DECLARE_TOAST(pg_policy, 4167, 4168);
 DECLARE_TOAST(pg_proc, 2836, 2837);
 DECLARE_TOAST(pg_rewrite, 2838, 2839);
 DECLARE_TOAST(pg_seclabel, 3598, 3599);
 DECLARE_TOAST(pg_statistic, 2840, 2841);
 DECLARE_TOAST(pg_statistic_ext, 3439, 3440);
 DECLARE_TOAST(pg_trigger, 2336, 2337);
+DECLARE_TOAST(pg_ts_dict, 4169, 4170);
+DECLARE_TOAST(pg_type, 4171, 4172);

Re: missing toast table for pg_policy

[Michael Paquier]

On Tue, Jul 17, 2018 at 02:55:07PM +0700, John Naylor wrote:
> I didn't dig deeper, since TOAST and the large object facility are
> mutually exclusive so there shouldn't be a toast table here anyway.
> Hope this helps.

[... digging ...]
This comes from get_rel_infos where large objects are treated as user
data.  Rather than the comment you added, I would rather do the
following:
"Large object catalogs and toast tables are mutually exclusive and large
object data is handled as user data by pg_upgrade, which would cause
failures."
--
Michael


signature.asc
Description: PGP signature

Re: missing toast table for pg_policy

[John Naylor]

On 7/17/18, Michael Paquier  wrote:
> I was just having a second look at this patch, and did a bit more tests
> with pg_upgrade which passed.
>
> +-- 2. pg_largeobject and pg_largeobject_metadata, to avoid problems
> +-- with pg_upgrade
> John, what's actually the failure that was seen here?  It would be nice
> to see this patch committed but the reason here should be more
> explicit about why this cannot happen.

I'll copy what I wrote upthread last month:

On 6/19/18, John Naylor  wrote:
> On 2/20/18, Michael Paquier  wrote:
>> Regression tests of pg_upgrade are failing as follows:
>> New cluster database "postgres" is not empty
>> Failure, exiting
>> + rm -rf /tmp/pg_upgrade_check-Xn0ZLe
>
> I looked into this briefly. The error comes from
> check_new_cluster_is_empty() in src/bin/pg_upgrade/check.c, which
> contains the comment
>
> /* pg_largeobject and its index should be skipped */

I didn't dig deeper, since TOAST and the large object facility are
mutually exclusive so there shouldn't be a toast table here anyway.
Hope this helps.

-John Naylor

Re: missing toast table for pg_policy

[Michael Paquier]

On Sat, Jul 14, 2018 at 03:47:38PM +0700, John Naylor wrote:
> I hope you don't mind, but since it might be tedious to piece together
> the addenda I left behind in this thread, I thought it might be useful
> to update Joe's patch. The attached was rebased over the new
> regression test, passes the pg_upgrade test, and has a draft commit
> message.

I was just having a second look at this patch, and did a bit more tests
with pg_upgrade which passed.

+-- 2. pg_largeobject and pg_largeobject_metadata, to avoid problems
+-- with pg_upgrade
John, what's actually the failure that was seen here?  It would be nice
to see this patch committed but the reason here should be more
explicit about why this cannot happen.
--
Michael


signature.asc
Description: PGP signature

Re: missing toast table for pg_policy

[Michael Paquier]

On Sat, Jul 14, 2018 at 03:47:38PM +0700, John Naylor wrote:
> I hope you don't mind, but since it might be tedious to piece together
> the addenda I left behind in this thread, I thought it might be useful
> to update Joe's patch. The attached was rebased over the new
> regression test, passes the pg_upgrade test, and has a draft commit
> message.

The test added by ecd6b434 is very helpful.  Looking at the patch, this
seems sane to me.  pg_upgrade run with different past versions is
proving to pass properly, and we don't want to have toast tables for
large object catalogs.  It is nice that you took care of putting things
in a consistent order in IsSharedRelation().
--
Michael


signature.asc
Description: PGP signature

Re: missing toast table for pg_policy

[Michael Paquier]

On Mon, Jul 09, 2018 at 09:19:35PM -0400, Joe Conway wrote:
> If you can wait for the next commitfest (the original one I put the
> patch into, i.e. September) I am committed to making it happen. But if
> you are anxious to getting this into the current commitfest, go for it.
> I am in the middle of a move from California to Florida and don't think
> I will realistically have time this month.

Oh, OK, thanks for your reply.  Peter, would you like to accelerate
things here?  I recall that you worked lately on another item dependent
on the one discussed here.  I can personally wait a couple of CFs, v12
development has just begun. 
--
Michael


signature.asc
Description: PGP signature

Re: missing toast table for pg_policy

[Joe Conway]

On 07/09/2018 09:16 PM, Michael Paquier wrote:
> On Mon, Jul 09, 2018 at 02:45:26PM +0200, Peter Eisentraut wrote:
>> On 15.06.18 21:15, Joe Conway wrote:
>>> Not surprising -- thanks for the update.
>>>
 It occurred to be that we could go further and create most toast
 tables automatically by taking advantage of the fact that the toast
 creation function is a no-op if there are no varlena attributes. The
 second patch (applies on top of the first) demonstrates a setup where
 only shared and bootstrap catalogs need to have toast declarations
 specified manually with fixed OIDs. It's possible this way is more
 fragile, though.
>>>
>>> Hmmm, I'll have a look.
>>
>> Are you going to provide a new patch soon?  This commit fest item is
>> otherwise not moving forward.
> 
> I am a fan of this patch, so I'd like to help make things move on.  Joe,
> if you cannot provide a patch, do you mind if I begin to hack my way
> around?

If you can wait for the next commitfest (the original one I put the
patch into, i.e. September) I am committed to making it happen. But if
you are anxious to getting this into the current commitfest, go for it.
I am in the middle of a move from California to Florida and don't think
I will realistically have time this month.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: missing toast table for pg_policy

[Michael Paquier]

On Mon, Jul 09, 2018 at 02:45:26PM +0200, Peter Eisentraut wrote:
> On 15.06.18 21:15, Joe Conway wrote:
>> Not surprising -- thanks for the update.
>> 
>>> It occurred to be that we could go further and create most toast
>>> tables automatically by taking advantage of the fact that the toast
>>> creation function is a no-op if there are no varlena attributes. The
>>> second patch (applies on top of the first) demonstrates a setup where
>>> only shared and bootstrap catalogs need to have toast declarations
>>> specified manually with fixed OIDs. It's possible this way is more
>>> fragile, though.
>> 
>> Hmmm, I'll have a look.
> 
> Are you going to provide a new patch soon?  This commit fest item is
> otherwise not moving forward.

I am a fan of this patch, so I'd like to help make things move on.  Joe,
if you cannot provide a patch, do you mind if I begin to hack my way
around?
--
Michael


signature.asc
Description: PGP signature

Re: missing toast table for pg_policy

[Peter Eisentraut]

On 15.06.18 21:15, Joe Conway wrote:
> Not surprising -- thanks for the update.
> 
>> It occurred to be that we could go further and create most toast
>> tables automatically by taking advantage of the fact that the toast
>> creation function is a no-op if there are no varlena attributes. The
>> second patch (applies on top of the first) demonstrates a setup where
>> only shared and bootstrap catalogs need to have toast declarations
>> specified manually with fixed OIDs. It's possible this way is more
>> fragile, though.
> 
> Hmmm, I'll have a look.

Are you going to provide a new patch soon?  This commit fest item is
otherwise not moving forward.

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

Re: missing toast table for pg_policy

[Andres Freund]

On 2018-02-18 11:18:49 -0500, Tom Lane wrote:
> Joe Conway  writes:
> > Is there really a compelling reason to not just create toast tables for
> > all system catalogs as in the attached?
> 
> What happens when you VACUUM FULL pg_class?  (The associated toast table
> would have to be nonempty for the test to prove much.)
> 
> I'm fairly suspicious of toasting anything that the toast mechanism itself
> depends on, actually, and that would include at least pg_attribute and
> pg_index as well as pg_class.  Maybe we could get away with it because
> there would never be any actual recursion only potential recursion ...
> but it seems scary.
> 
> On the whole, I'm dubious that the risk/reward ratio is attractive here.

I think we should just review the code to make sure bootstrapping works
for pg_class and fix if necessary. We've discussed this repeatedly, and
this concern has been the blocker every time - at this point I'm fairly
sure we could have easily fixed the issues.

At least the simpler case, where the bootstrapped contents themselves
aren't toasted, shouldn't be hard to support. And that restriction seems
fairly easy to support, by dint of the population mechanism for
bootstrapped catalogs not supporting toasting.

What I'm not sure will work correctly without fixes is the relation
swapping logic for VACUUM FULL / CLUSTER. There's some pg_class specific
logic in there, that might need to be adapted for pg_class's toast
table.

Greetings,

Andres Freund

Re: missing toast table for pg_policy

[John Naylor]

On 2/20/18, Michael Paquier  wrote:
> Regression tests of pg_upgrade are failing as follows:
> New cluster database "postgres" is not empty
> Failure, exiting
> + rm -rf /tmp/pg_upgrade_check-Xn0ZLe

I looked into this briefly. The error comes from
check_new_cluster_is_empty() in src/bin/pg_upgrade/check.c, which
contains the comment

/* pg_largeobject and its index should be skipped */

If you don't create a toast table for pg_largeobject and
pg_largeobject_metadata, the pg_upgrade regression test passes.
Revised addendum attached. Adding two more exceptions to my alternate
implementation starts to make it ugly, so I haven't updated it for
now.

-John Naylor

> Michael
>
diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h
index ee54487..f259890 100644
--- a/src/include/catalog/toasting.h
+++ b/src/include/catalog/toasting.h
@@ -46,9 +46,9 @@ extern void BootstrapToastTable(char *relName,
  */
 
 /* normal catalogs */
-DECLARE_TOAST(pg_aggregate, 4139, 4140);
+DECLARE_TOAST(pg_aggregate, 4159, 4160);
 DECLARE_TOAST(pg_attrdef, 2830, 2831);
-DECLARE_TOAST(pg_collation, 4141, 4142);
+DECLARE_TOAST(pg_collation, 4161, 4162);
 DECLARE_TOAST(pg_constraint, 2832, 2833);
 DECLARE_TOAST(pg_default_acl, 4143, 4144);
 DECLARE_TOAST(pg_description, 2834, 2835);
@@ -59,8 +59,6 @@ DECLARE_TOAST(pg_foreign_server, 4151, 4152);
 DECLARE_TOAST(pg_foreign_table, 4153, 4154);
 DECLARE_TOAST(pg_init_privs, 4155, 4156);
 DECLARE_TOAST(pg_language, 4157, 4158);
-DECLARE_TOAST(pg_largeobject, 4159, 4160);
-DECLARE_TOAST(pg_largeobject_metadata, 4161, 4162);
 DECLARE_TOAST(pg_namespace, 4163, 4164);
 DECLARE_TOAST(pg_partitioned_table, 4165, 4166);
 DECLARE_TOAST(pg_policy, 4167, 4168);
diff --git a/src/test/regress/expected/misc_sanity.out 
b/src/test/regress/expected/misc_sanity.out
index 0be74d2..a6dacd4 100644
--- a/src/test/regress/expected/misc_sanity.out
+++ b/src/test/regress/expected/misc_sanity.out
@@ -88,15 +88,18 @@ WHERE c.oid < 16384 AND
   relkind = 'r' AND
   attstorage != 'p'
 ORDER BY 1,2;
-   relname|attname|   atttypid   
---+---+--
- pg_attribute | attacl| aclitem[]
- pg_attribute | attfdwoptions | text[]
- pg_attribute | attoptions| text[]
- pg_class | relacl| aclitem[]
- pg_class | reloptions| text[]
- pg_class | relpartbound  | pg_node_tree
- pg_index | indexprs  | pg_node_tree
- pg_index | indpred   | pg_node_tree
-(8 rows)
+ relname |attname|   atttypid   
+-+---+--
+ pg_attribute| attacl| aclitem[]
+ pg_attribute| attfdwoptions | text[]
+ pg_attribute| attmissingval | anyarray
+ pg_attribute| attoptions| text[]
+ pg_class| relacl| aclitem[]
+ pg_class| reloptions| text[]
+ pg_class| relpartbound  | pg_node_tree
+ pg_index| indexprs  | pg_node_tree
+ pg_index| indpred   | pg_node_tree
+ pg_largeobject  | data  | bytea
+ pg_largeobject_metadata | lomacl| aclitem[]
+(11 rows)
 

Re: missing toast table for pg_policy

[Joe Conway]

On 06/15/2018 02:40 PM, John Naylor wrote:
> On 2/19/18, Joe Conway  wrote:
>> The attached does exactly this. Gives all system tables toast tables
>> except pg_class, pg_attribute, and pg_index, and includes cat version
>> bump and regression test in misc_sanity.
>>
>> Any further discussion, comments, complaints?
> 
> Hi Joe,
> There's been a little bit-rot with duplicate OIDs and the regression
> test. The first attached patch fixes that (applies on top of yours).


Not surprising -- thanks for the update.


> It occurred to be that we could go further and create most toast
> tables automatically by taking advantage of the fact that the toast
> creation function is a no-op if there are no varlena attributes. The
> second patch (applies on top of the first) demonstrates a setup where
> only shared and bootstrap catalogs need to have toast declarations
> specified manually with fixed OIDs. It's possible this way is more
> fragile, though.

Hmmm, I'll have a look.


Thanks!

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: missing toast table for pg_policy

[John Naylor]

On 2/19/18, Joe Conway  wrote:
> The attached does exactly this. Gives all system tables toast tables
> except pg_class, pg_attribute, and pg_index, and includes cat version
> bump and regression test in misc_sanity.
>
> Any further discussion, comments, complaints?

Hi Joe,
There's been a little bit-rot with duplicate OIDs and the regression
test. The first attached patch fixes that (applies on top of yours).

It occurred to be that we could go further and create most toast
tables automatically by taking advantage of the fact that the toast
creation function is a no-op if there are no varlena attributes. The
second patch (applies on top of the first) demonstrates a setup where
only shared and bootstrap catalogs need to have toast declarations
specified manually with fixed OIDs. It's possible this way is more
fragile, though.

I also did some non-scientific performance testing. On my machine,
initdb takes at least:
HEAD ~1040 ms
patch ~1070 ms
with my addenda ~1075 ms

A little slower, but within the noise of variation.

-John Naylor

> Joe
>
> --
> Crunchy Data - http://crunchydata.com
> PostgreSQL Support for Secure Enterprises
> Consulting, Training, & Open Source Development
>
diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h
index ee54487..1387569 100644
--- a/src/include/catalog/toasting.h
+++ b/src/include/catalog/toasting.h
@@ -46,9 +46,9 @@ extern void BootstrapToastTable(char *relName,
  */
 
 /* normal catalogs */
-DECLARE_TOAST(pg_aggregate, 4139, 4140);
+DECLARE_TOAST(pg_aggregate, 4187, 4188);
 DECLARE_TOAST(pg_attrdef, 2830, 2831);
-DECLARE_TOAST(pg_collation, 4141, 4142);
+DECLARE_TOAST(pg_collation, 4189, 4190);
 DECLARE_TOAST(pg_constraint, 2832, 2833);
 DECLARE_TOAST(pg_default_acl, 4143, 4144);
 DECLARE_TOAST(pg_description, 2834, 2835);
diff --git a/src/test/regress/expected/misc_sanity.out b/src/test/regress/expected/misc_sanity.out
index 0be74d2..ad10a7e 100644
--- a/src/test/regress/expected/misc_sanity.out
+++ b/src/test/regress/expected/misc_sanity.out
@@ -92,11 +92,12 @@ ORDER BY 1,2;
 --+---+--
  pg_attribute | attacl| aclitem[]
  pg_attribute | attfdwoptions | text[]
+ pg_attribute | attmissingval | anyarray
  pg_attribute | attoptions| text[]
  pg_class | relacl| aclitem[]
  pg_class | reloptions| text[]
  pg_class | relpartbound  | pg_node_tree
  pg_index | indexprs  | pg_node_tree
  pg_index | indpred   | pg_node_tree
-(8 rows)
+(9 rows)
 
diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y
index 4c72989..9f5a3b9 100644
--- a/src/backend/bootstrap/bootparse.y
+++ b/src/backend/bootstrap/bootparse.y
@@ -27,10 +27,13 @@
 #include "catalog/heap.h"
 #include "catalog/namespace.h"
 #include "catalog/pg_am.h"
+#include "catalog/pg_amproc.h"
 #include "catalog/pg_attribute.h"
 #include "catalog/pg_authid.h"
 #include "catalog/pg_class.h"
+#include "catalog/pg_index.h"
 #include "catalog/pg_namespace.h"
+#include "catalog/pg_opclass.h"
 #include "catalog/pg_tablespace.h"
 #include "catalog/toasting.h"
 #include "commands/defrem.h"
@@ -255,6 +258,23 @@ Boot_CreateStmt:
 	  InvalidOid,
 	  NULL);
 		elog(DEBUG4, "relation created with OID %u", id);
+
+		/*
+		 * Create a toast table for all non-shared catalogs
+		 * that have any varlena attributes, except for pg_index.
+		 * We also exclude catalogs that need to be populated
+		 * in order for toast tables to be created. These don't
+		 * currently have varlenas, so this is just future-proofing.
+		 */
+		if (!shared_relation
+			&& id != IndexRelationId
+			&& id != AccessMethodRelationId
+			&& id != OperatorClassRelationId
+			&& id != AccessMethodProcedureRelationId)
+		{
+			NewRelationCreateToastTable(id, (Datum) 0);
+		}
+		elog(DEBUG4, "creating toast table for table \"%s\"", $2);
 	}
 	do_end();
 }
diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index 0865240..48a871a 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -27,12 +27,13 @@ include $(top_srcdir)/src/backend/common.mk
 
 # Note: the order of this list determines the order in which the catalog
 # header files are assembled into postgres.bki.  BKI_BOOTSTRAP catalogs
-# must appear first, and there are reputedly other, undocumented ordering
-# dependencies.
+# must appear first, followed by catalogs needed for creating toast tables.
+# There are reputedly other, undocumented ordering dependencies.
 CATALOG_HEADERS := \
 	pg_proc.h pg_type.h pg_attribute.h pg_class.h \
-	pg_attrdef.h pg_constraint.h pg_inherits.h pg_index.h pg_operator.h \
-	pg_opfamily.h pg_opclass.h pg_am.h pg_amop.h pg_amproc.h \
+	pg_index.h pg_am.h pg_opclass.h pg_amproc.h \
+	pg_operator.h pg_opfamily.h pg_amop.h \
+	pg_attrdef.h pg_constraint.h pg_inherits.h \
 	pg_language.h 

Re: missing toast table for pg_policy

[Robert Haas]

On Sun, Feb 18, 2018 at 10:43 AM, Joe Conway  wrote:
> Is there really a compelling reason to not just create toast tables for
> all system catalogs as in the attached? Then we could just check for 0
> rows in misc_sanity.sql.

+1.  I don't have a huge problem with excluding a few key catalogs for
which we think it might be unsafe, but in general it seems like a good
idea to settle on a policy of including them everywhere else.
Omitting one or even half a dozen TOAST tables on system catalogs
doesn't save anything material for users, but does succeed in annoying
some user who is trying to do something a little off the beaten path.
It also doesn't save anything for developers; indeed, the cognitive
load comes mostly from having to argue about which things should get
TOAST tables.  If we just add them everywhere, we can stop arguing
about this; no other policy will have that effect.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: missing toast table for pg_policy

[Joe Conway]

On 02/18/2018 11:18 AM, Tom Lane wrote:
> Joe Conway  writes:
>> Is there really a compelling reason to not just create toast tables for
>> all system catalogs as in the attached?
> 
> What happens when you VACUUM FULL pg_class?  (The associated toast table
> would have to be nonempty for the test to prove much.)

I tried this:
create table foo (id int);
do $$declare i int; begin for i in 1..1000 loop execute 'create user u'
|| i; end loop; end;$$;
do $$declare i int; begin for i in 1..1000 loop execute 'grant all on
foo to u' || i; end loop; end;$$;
vacuum full pg_class;

Worked without issue FWIW.

> I'm fairly suspicious of toasting anything that the toast mechanism itself
> depends on, actually, and that would include at least pg_attribute and
> pg_index as well as pg_class.  Maybe we could get away with it because
> there would never be any actual recursion only potential recursion ...
> but it seems scary.

Well that is the other approach we could pursue -- instead of justifying
which system catalogs need toast tables we could create an exclusion
list of which ones should not have toast tables, with the current
candidates being pg_class, pg_attribute, and pg_index.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: missing toast table for pg_policy

[Tom Lane]

Joe Conway  writes:
> Is there really a compelling reason to not just create toast tables for
> all system catalogs as in the attached?

What happens when you VACUUM FULL pg_class?  (The associated toast table
would have to be nonempty for the test to prove much.)

I'm fairly suspicious of toasting anything that the toast mechanism itself
depends on, actually, and that would include at least pg_attribute and
pg_index as well as pg_class.  Maybe we could get away with it because
there would never be any actual recursion only potential recursion ...
but it seems scary.

On the whole, I'm dubious that the risk/reward ratio is attractive here.

regards, tom lane

Re: missing toast table for pg_policy

[Joe Conway]

On 02/17/2018 11:39 AM, Tom Lane wrote:
> Joe Conway  writes:
>> Yes, exactly. I'm fine with not backpatching, just wanted to raise the
>> possibility. I will push later today to HEAD (with a catalog version bump).
> 
> BTW, I was wondering if it'd be a good idea to try to forestall future
> oversights of this sort by adding a test query in, say, misc_sanity.sql.
> Something like
> 
> select relname, attname, atttypid::regtype
> from pg_class c join pg_attribute a on c.oid = attrelid
> where c.oid < 16384 and reltoastrelid=0 and relkind = 'r' and attstorage != 
> 'p'
> order by 1,2;
> 
> If you try that you'll see the list is quite long:



> I think in most of these cases we've consciously decided not to toast-ify,
> but maybe some of them need a second look.

Is there really a compelling reason to not just create toast tables for
all system catalogs as in the attached? Then we could just check for 0
rows in misc_sanity.sql.

For what its worth:

HEAD

# du -h --max-depth=1 $PGDATA
[...]
22M /usr/local/pgsql-head/data/base
[...]
584K/usr/local/pgsql-head/data/global
[...]
38M /usr/local/pgsql-head/data

time make check
real0m16.295s
user0m3.597s
sys 0m1.465s


with patch

# du -h --max-depth=1 $PGDATA
[...]
23M /usr/local/pgsql-head/data/base
[...]
632K/usr/local/pgsql-head/data/global
[...]
39M /usr/local/pgsql-head/data

time make check
real0m16.462s
user0m3.521s
sys 0m1.531s

select relname, attname, atttypid::regtype
from pg_class c join pg_attribute a on c.oid = attrelid
where c.oid < 16384 and reltoastrelid=0 and relkind = 'r' and attstorage
!= 'p'
order by 1,2;
 relname | attname | atttypid
-+-+--
(0 rows)


-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 809749add9..813b3b87cc 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -258,7 +258,19 @@ IsSharedRelation(Oid relationId)
 		relationId == PgDbRoleSettingToastTable ||
 		relationId == PgDbRoleSettingToastIndex ||
 		relationId == PgShseclabelToastTable ||
-		relationId == PgShseclabelToastIndex)
+		relationId == PgShseclabelToastIndex ||
+		relationId == PgAuthidToastTable ||
+		relationId == PgAuthidToastIndex ||
+		relationId == PgDatabaseToastTable ||
+		relationId == PgDatabaseToastIndex ||
+		relationId == PgPlTemplateToastTable ||
+		relationId == PgPlTemplateToastIndex ||
+		relationId == PgReplicationOriginToastTable ||
+		relationId == PgReplicationOriginToastIndex ||
+		relationId == PgSubscriptionToastTable ||
+		relationId == PgSubscriptionToastIndex ||
+		relationId == PgTablespaceToastTable ||
+		relationId == PgTablespaceToastIndex)
 		return true;
 	return false;
 }
diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h
index f6387ae143..3ef3990ea3 100644
--- a/src/include/catalog/toasting.h
+++ b/src/include/catalog/toasting.h
@@ -46,25 +46,64 @@ extern void BootstrapToastTable(char *relName,
  */
 
 /* normal catalogs */
+DECLARE_TOAST(pg_aggregate, 4139, 4140);
 DECLARE_TOAST(pg_attrdef, 2830, 2831);
+DECLARE_TOAST(pg_attribute, 4141, 4142);
+DECLARE_TOAST(pg_class, 4143, 4144);
+DECLARE_TOAST(pg_collation, 4145, 4146);
 DECLARE_TOAST(pg_constraint, 2832, 2833);
+DECLARE_TOAST(pg_default_acl, 4147, 4148);
 DECLARE_TOAST(pg_description, 2834, 2835);
+DECLARE_TOAST(pg_event_trigger, 4149, 4150);
+DECLARE_TOAST(pg_extension, 4151, 4152);
+DECLARE_TOAST(pg_foreign_data_wrapper, 4153, 4154);
+DECLARE_TOAST(pg_foreign_server, 4155, 4156);
+DECLARE_TOAST(pg_foreign_table, 4157, 4158);
+DECLARE_TOAST(pg_index, 4159, 4160);
+DECLARE_TOAST(pg_init_privs, 4161, 4162);
+DECLARE_TOAST(pg_language, 4163, 4164);
+DECLARE_TOAST(pg_largeobject, 4165, 4166);
+DECLARE_TOAST(pg_largeobject_metadata, 4167, 4168);
+DECLARE_TOAST(pg_namespace, 4169, 4170);
+DECLARE_TOAST(pg_partitioned_table, 4171, 4172);
+DECLARE_TOAST(pg_policy, 4173, 4174);
 DECLARE_TOAST(pg_proc, 2836, 2837);
 DECLARE_TOAST(pg_rewrite, 2838, 2839);
 DECLARE_TOAST(pg_seclabel, 3598, 3599);
 DECLARE_TOAST(pg_statistic, 2840, 2841);
 DECLARE_TOAST(pg_statistic_ext, 3439, 3440);
 DECLARE_TOAST(pg_trigger, 2336, 2337);
+DECLARE_TOAST(pg_ts_dict, 4175, 4176);
+DECLARE_TOAST(pg_type, 4177, 4178);
+DECLARE_TOAST(pg_user_mapping, 4179, 4180);
 
 /* shared catalogs */
-DECLARE_TOAST(pg_shdescription, 2846, 2847);
-#define PgShdescriptionToastTable 2846
-#define PgShdescriptionToastIndex 2847
+DECLARE_TOAST(pg_authid, 4181, 4182);
+#define PgAuthidToastTable 4181
+#define PgAuthidToastIndex 4182
+DECLARE_TOAST(pg_database, 4183, 4184);	
+#define PgDatabaseToastTable 4183
+#define PgDatabaseToastIndex 4184
 DECLARE_TOAST(pg_db_role_setting, 2966, 2967);
 #define PgDbRoleSettingToastTable 2966
 #define 

Re: missing toast table for pg_policy

[Michael Paquier]

On Sat, Feb 17, 2018 at 03:52:33PM -0800, Andres Freund wrote:
> I've a hard hard hard time believing this is something useful to do. I
> mean by that argument you can just cause trouble everywhere by just
> storing arbitrarily large stuff via sql.

Did you read my last email until the end?  Particularly this quote:

> Longer salts make it for harder to reproduce connection proofs, so some
> users may want to privilege that than the number of iterations, and
> those are perfectly valid per the SCRAM exchange protocol.

The argument here is not about storing large blobs, it is about the
flexibility that the SCRAM protocol allows that PostgreSQL does not
because of this restriction in row size.  Postgres should have in the
future a set of GUC parameters to allow users to control the interation
number and the salt length when generating the SCRAM verifier depending
on their security requirements.  And I see no point in restraining
things on the backend as we do now.
--
Michael


signature.asc
Description: PGP signature

Re: missing toast table for pg_policy

[Andres Freund]

Hi,

On 2018-02-18 08:48:37 +0900, Michael Paquier wrote:
> On Sat, Feb 17, 2018 at 08:52:11AM -0800, Andres Freund wrote:
> > On 2018-02-17 11:39:57 -0500, Tom Lane wrote:
> > >  pg_authid   | rolpassword | text
> > 
> > that seems not not to require one.
> 
> You can craft SCRAM verifiers that make it fail, which can be easily
> done using this module:
> https://github.com/michaelpq/pg_plugins/tree/master/scram_utils
> 
> =# create extension scram_utils ;
> CREATE EXTENSION
> =# select scram_utils_verifier('your_role_name', 'foo', 100, 9000);
> ERROR:  54000: row is too big: size 12224, maximum size 8160
> 
> The third argument counts for the number of iterations to generate the
> proof and the fourth controls the salt length.

I've a hard hard hard time believing this is something useful to do. I
mean by that argument you can just cause trouble everywhere by just
storing arbitrarily large stuff via sql.

Greetings,

Andres Freund

Re: missing toast table for pg_policy

[Michael Paquier]

On Sat, Feb 17, 2018 at 08:52:11AM -0800, Andres Freund wrote:
> On 2018-02-17 11:39:57 -0500, Tom Lane wrote:
> >  pg_authid   | rolpassword | text
> 
> that seems not not to require one.

You can craft SCRAM verifiers that make it fail, which can be easily
done using this module:
https://github.com/michaelpq/pg_plugins/tree/master/scram_utils

=# create extension scram_utils ;
CREATE EXTENSION
=# select scram_utils_verifier('your_role_name', 'foo', 100, 9000);
ERROR:  54000: row is too big: size 12224, maximum size 8160

The third argument counts for the number of iterations to generate the
proof and the fourth controls the salt length.

Longer salts make it for harder to reproduce connection proofs, so some
users may want to privilege that than the number of iterations, and
those are perfectly valid per the SCRAM exchange protocol.

There is another restriction which limits the size of authentication
messages to 2000 in libpq, which we may actually want to relax in the
future if we allow configurable in-core salt lengths to be created with
a GUC.  And other clients like jdbc don't have this restriction if I
recall correctly.

In short, removing this restriction at least on HEAD for the backend
gives more flexibility.
--
Michael


signature.asc
Description: PGP signature

Re: missing toast table for pg_policy

[Tom Lane]

Andres Freund  writes:
> On 2018-02-17 11:39:57 -0500, Tom Lane wrote:
>> pg_aggregate| agginitval  | text
>> pg_aggregate| aggminitval | text

> Seems like it should have a toast table, it's not too hard to imagine
> some form of aggregate to have a large initial condition.

Really?  I should think the initial condition would almost always be some
spelling of "zero".  Certainly nobody's ever complained of this in the
past.

>> pg_attribute| attacl  | aclitem[]
>> pg_attribute| attfdwoptions   | text[]
>> pg_attribute| attoptions  | text[]

> Seems like it should have a toast table, but I think other people
> differed.

I think there was fear of circularity if we tried to toast pg_class
or pg_attribute.  (In particular, VACUUM FULL already has its hands
full handling pg_class correctly --- dealing with a toast table too
would probably be really, uh, interesting.)  Also, given the fact
that tupdescs can only store the fixed-width part of a pg_attribute
entry, having var-width fields in there at all is a pretty dubious
decision; it's way too easy to forget about that and try to fetch
them out of a tupdesc entry.  I think the right approach for
potentially-long per-attribute properties is exemplified by pg_attrdef.

In any case, I don't see any of the "options" columns as things that
are likely to get long enough to be a problem.  ACLs maybe could get
long, but I can only recall perhaps one thread complaining about that,
so I don't feel that there's field demand to justify toasting all the
catalogs with ACLs in them.

>> pg_largeobject  | data| bytea

> We deal with this in other ways.

Right, this is one that definitely should not have a toast table.

>> pg_partitioned_table| partexprs   | pg_node_tree

> Probably makes sense.

Dunno, what is a sane partitioning expression?

I don't feel that we need to insist on having a toast table for every
theoretically toastable column.  The point here is to make a conscious
decision for each such column that we don't expect it to get long.
I think most of these columns are probably fine.  Unsure about the
partitioning-related ones.

regards, tom lane

Re: missing toast table for pg_policy

[Andres Freund]

On 2018-02-17 11:39:57 -0500, Tom Lane wrote:
> BTW, I was wondering if it'd be a good idea to try to forestall future
> oversights of this sort by adding a test query in, say, misc_sanity.sql.
> Something like

+many


>  relname | attname |   atttypid   
> -+-+--
>  pg_aggregate| agginitval  | text
>  pg_aggregate| aggminitval | text

Seems like it should have a toast table, it's not too hard to imagine
some form of aggregate to have a large initial condition.


>  pg_attribute| attacl  | aclitem[]
>  pg_attribute| attfdwoptions   | text[]
>  pg_attribute| attoptions  | text[]

Seems like it should have a toast table, but I think other people
differed.


>  pg_authid   | rolpassword | text

that seems not not to require one.


>  pg_class| relacl  | aclitem[]
>  pg_class| reloptions  | text[]
>  pg_class| relpartbound| pg_node_tree

I still think this should have one, but we disagreed. Only argument
against that I can see is complexity around rewrites.


>  pg_collation| collversion | text

unnecessary.


>  pg_database | datacl  | aclitem[]
>  pg_default_acl  | defaclacl   | aclitem[]

hm.

>  pg_event_trigger| evttags | text[]

unnecessary?


>  pg_extension| extcondition| text[]
>  pg_extension| extconfig   | oid[]
>  pg_extension| extversion  | text

Possibly add?


>  pg_foreign_data_wrapper | fdwacl  | aclitem[]
>  pg_foreign_data_wrapper | fdwoptions  | text[]

Possibly add?


>  pg_foreign_server   | srvacl  | aclitem[]
>  pg_foreign_server   | srvoptions  | text[]
>  pg_foreign_server   | srvtype | text
>  pg_foreign_server   | srvversion  | text
>  pg_foreign_table| ftoptions   | text[]

Add? That's a fair number of potentially longer fields.


>  pg_index| indexprs| pg_node_tree
>  pg_index| indpred | pg_node_tree

Imo we should add one here, but honestly I can recall only one or two
complaints.


>  pg_init_privs   | initprivs   | aclitem[]

Only if we decide to make other aclitem containing fields toastable.


>  pg_largeobject  | data| bytea

We deal with this in other ways.


>  pg_partitioned_table| partexprs   | pg_node_tree

Probably makes sense.


>  pg_pltemplate   | tmplacl | aclitem[]
>  pg_pltemplate   | tmplhandler | text
>  pg_pltemplate   | tmplinline  | text
>  pg_pltemplate   | tmpllibrary | text
>  pg_pltemplate   | tmplvalidator   | text

Hard to imagine this being required, unless we just want to make
aclitem[] toastable as a rule.


>  pg_policy   | polqual | pg_node_tree
>  pg_policy   | polroles| oid[]
>  pg_policy   | polwithcheck| pg_node_tree

Yes.


>  pg_replication_origin   | roname  | text

unnecessary.


>  pg_subscription | subconninfo | text
>  pg_subscription | subpublications | text[]
>  pg_subscription | subsynccommit   | text

I'd say yes, with a few alternative hosts connection info can become
quite long.


>  pg_tablespace   | spcacl  | aclitem[]
>  pg_tablespace   | spcoptions  | text[]

Hm?


>  pg_ts_dict  | dictinitoption  | text

probably not.


> I think in most of these cases we've consciously decided not to toast-ify,
> but maybe some of them need a second look.

Greetings,

Andres Freund

Re: missing toast table for pg_policy

[Tom Lane]

Joe Conway  writes:
> Yes, exactly. I'm fine with not backpatching, just wanted to raise the
> possibility. I will push later today to HEAD (with a catalog version bump).

BTW, I was wondering if it'd be a good idea to try to forestall future
oversights of this sort by adding a test query in, say, misc_sanity.sql.
Something like

select relname, attname, atttypid::regtype
from pg_class c join pg_attribute a on c.oid = attrelid
where c.oid < 16384 and reltoastrelid=0 and relkind = 'r' and attstorage != 'p'
order by 1,2;

If you try that you'll see the list is quite long:

 relname | attname |   atttypid   
-+-+--
 pg_aggregate| agginitval  | text
 pg_aggregate| aggminitval | text
 pg_attribute| attacl  | aclitem[]
 pg_attribute| attfdwoptions   | text[]
 pg_attribute| attoptions  | text[]
 pg_authid   | rolpassword | text
 pg_class| relacl  | aclitem[]
 pg_class| reloptions  | text[]
 pg_class| relpartbound| pg_node_tree
 pg_collation| collversion | text
 pg_database | datacl  | aclitem[]
 pg_default_acl  | defaclacl   | aclitem[]
 pg_event_trigger| evttags | text[]
 pg_extension| extcondition| text[]
 pg_extension| extconfig   | oid[]
 pg_extension| extversion  | text
 pg_foreign_data_wrapper | fdwacl  | aclitem[]
 pg_foreign_data_wrapper | fdwoptions  | text[]
 pg_foreign_server   | srvacl  | aclitem[]
 pg_foreign_server   | srvoptions  | text[]
 pg_foreign_server   | srvtype | text
 pg_foreign_server   | srvversion  | text
 pg_foreign_table| ftoptions   | text[]
 pg_index| indexprs| pg_node_tree
 pg_index| indpred | pg_node_tree
 pg_init_privs   | initprivs   | aclitem[]
 pg_language | lanacl  | aclitem[]
 pg_largeobject  | data| bytea
 pg_largeobject_metadata | lomacl  | aclitem[]
 pg_namespace| nspacl  | aclitem[]
 pg_partitioned_table| partexprs   | pg_node_tree
 pg_pltemplate   | tmplacl | aclitem[]
 pg_pltemplate   | tmplhandler | text
 pg_pltemplate   | tmplinline  | text
 pg_pltemplate   | tmpllibrary | text
 pg_pltemplate   | tmplvalidator   | text
 pg_policy   | polqual | pg_node_tree
 pg_policy   | polroles| oid[]
 pg_policy   | polwithcheck| pg_node_tree
 pg_replication_origin   | roname  | text
 pg_subscription | subconninfo | text
 pg_subscription | subpublications | text[]
 pg_subscription | subsynccommit   | text
 pg_tablespace   | spcacl  | aclitem[]
 pg_tablespace   | spcoptions  | text[]
 pg_ts_dict  | dictinitoption  | text
 pg_type | typacl  | aclitem[]
 pg_type | typdefault  | text
 pg_type | typdefaultbin   | pg_node_tree
 pg_user_mapping | umoptions   | text[]
(50 rows)

I think in most of these cases we've consciously decided not to toast-ify,
but maybe some of them need a second look.

regards, tom lane

Re: missing toast table for pg_policy

[Joe Conway]

On 02/16/2018 05:24 PM, Tom Lane wrote:
> Joe Conway  writes:
>> On 02/16/2018 05:07 PM, Andres Freund wrote:
>>> If problematic for < master users I think you'll have to restart cluster
>>> with allow_system_table_mods, manually create/drop toasted column. IIRC
>>> that should add a toast table even after dropping.
> 
>> I wasn't sure if we would want to backpatch and put the manual procedure
>> steps into the release notes.
> 
> The example you give seems like pretty bad practice to me.  I don't think
> we should back-patch unless it's possible to trigger the problem with a
> more realistic policy expression.

Fair enough, but the origin of this was a real life-based complaint.

> (Also, one can always work around it by putting the complicated condition
> into a function, which would likely be a better idea anyway from a
> maintenance standpoint.)

Yes, exactly. I'm fine with not backpatching, just wanted to raise the
possibility. I will push later today to HEAD (with a catalog version bump).

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: missing toast table for pg_policy

[Tom Lane]

Joe Conway  writes:
> On 02/16/2018 05:07 PM, Andres Freund wrote:
>> On 2018-02-16 16:56:15 -0500, Joe Conway wrote:
>>> Looking at the issue, the problem seems to be missing toast table for
>>> pg_policy. Also attached is a one line patch. It isn't clear to me
>>> whether this is a candidate for backpatching.

>> Don't think it is - it'd not take effect on already initdb'ed clusters.

> Yep, knew that, but...

>> If problematic for < master users I think you'll have to restart cluster
>> with allow_system_table_mods, manually create/drop toasted column. IIRC
>> that should add a toast table even after dropping.

> I wasn't sure if we would want to backpatch and put the manual procedure
> steps into the release notes.

The example you give seems like pretty bad practice to me.  I don't think
we should back-patch unless it's possible to trigger the problem with a
more realistic policy expression.

(Also, one can always work around it by putting the complicated condition
into a function, which would likely be a better idea anyway from a
maintenance standpoint.)

regards, tom lane

Re: missing toast table for pg_policy

[Joe Conway]

On 02/16/2018 05:07 PM, Andres Freund wrote:
> Hi,
> 
> On 2018-02-16 16:56:15 -0500, Joe Conway wrote:
>> Looking at the issue, the problem seems to be missing toast table for
>> pg_policy. Also attached is a one line patch. It isn't clear to me
>> whether this is a candidate for backpatching.
> 
> Don't think it is - it'd not take effect on already initdb'ed clusters.

Yep, knew that, but...

> If problematic for < master users I think you'll have to restart cluster
> with allow_system_table_mods, manually create/drop toasted column. IIRC
> that should add a toast table even after dropping.

I wasn't sure if we would want to backpatch and put the manual procedure
steps into the release notes.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: missing toast table for pg_policy

[Andres Freund]

Hi,

On 2018-02-16 16:56:15 -0500, Joe Conway wrote:
> Looking at the issue, the problem seems to be missing toast table for
> pg_policy. Also attached is a one line patch. It isn't clear to me
> whether this is a candidate for backpatching.

Don't think it is - it'd not take effect on already initdb'ed clusters.

If problematic for < master users I think you'll have to restart cluster
with allow_system_table_mods, manually create/drop toasted column. IIRC
that should add a toast table even after dropping.

Greetings,

Andres Freund