Re: missing toast table for pg_policy
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
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
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
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 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 ... > 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. regards, tom lane
Re: missing toast table for pg_policy
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
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
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
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); +DECLARE_TOAST(pg_user_
Re: missing toast table for pg_policy
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
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
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
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
On 7/12/18, Peter Eisentraut wrote: > To get things rolling, I have committed the regression test addition. > > I'll look through the other stuff soon. Hi Peter, 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. -John Naylor From e414effdcf03b3005390ab1671f4425c9e3036f2 Mon Sep 17 00:00:00 2001 From: John Naylor Date: Sat, 14 Jul 2018 15:09:44 +0700 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. 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, which will never have reason for a toast table anyway. --- src/backend/catalog/catalog.c | 18 +-- src/include/catalog/toasting.h| 40 ++-- src/test/regress/expected/misc_sanity.out | 79 --- src/test/regress/sql/misc_sanity.sql | 11 +++-- 4 files changed, 80 insertions(+), 68 deletions(-) diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c index a42155e..6061428 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 3db39b8..f259890 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); +DECLARE_TOAST(pg_user_mapping, 4173, 4174); /* shared catalogs */ -DECLARE_TOAST(pg_shdescription, 2846, 2847); -#define PgShdescriptionToastTable 2846 -#define PgShdescriptionToastIndex 2847 +DECLARE_TOAST(pg_authid, 4175, 4176); +#define PgAuthidToastTable 4175 +#define PgAuthidToastIndex 4176 +DECLARE_TOAST(pg_database, 4177, 4178); +#define PgDatabaseToastTable 4177 +#define PgDatabaseToastIndex 4178 DECLARE_TOAST(pg_db_role_setting, 2966, 2967); #define PgDbRoleSettingToastTable 2966 #define PgDbRoleSettingToastIndex 2967 +DECLARE_TOAST(pg_pltemplate, 4179, 4180); +#define PgPlTemplateToastTable 4179 +#define PgPlTemplateToastIndex 4180 +DECLARE_TOAST(pg_replication_origin, 4181, 4182); +#define PgReplicationOriginToastTable 4181 +#define PgReplicationOriginToastIndex 4182 +DECLARE_TOAST(pg_shdescription, 2846, 2847); +#define PgShdescriptionToastTable 2846 +#define PgShdescriptionToastIndex
Re: missing toast table for pg_policy
On 10.07.18 03:29, Michael Paquier wrote: > 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. To get things rolling, I have committed the regression test addition. I'll look through the other stuff soon. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: missing toast table for pg_policy
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
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
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
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
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
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
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
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 pg_largeobject_metada
Re: missing toast table for pg_policy
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
On Mon, Feb 19, 2018 at 11:33:30AM -0500, 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? Thanks Joe for working on this. +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' This is really a good idea! (Saw the suggestion upthread as well, did not comment on it previously.) 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 Just a thought: introducing a system function which returns FirstNormalObjectId. I have myself faced a couple of times case where this OID is hardcoded in some tests. I'll spawn a new thread about that. -- Michael signature.asc Description: PGP signature
Re: missing toast table for pg_policy
On 02/18/2018 01:33 PM, Joe Conway wrote: > On 02/18/2018 11:18 AM, Tom Lane wrote: >> 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. 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? Joe -- 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/catversion.h b/src/include/catalog/catversion.h index 433d6db4f6..202072c3c7 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -53,6 +53,6 @@ */ /* mmddN */ -#define CATALOG_VERSION_NO 201802061 +#define CATALOG_VERSION_NO 201802191 #endif diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h index f6387ae143..4586ac93b2 100644 --- a/src/include/catalog/toasting.h +++ b/src/include/catalog/toasting.h @@ -46,25 +46,61 @@ extern void BootstrapToastTable(char *relName, */ /* normal catalogs */ +DECLARE_TOAST(pg_aggregate, 4139, 4140); DECLARE_TOAST(pg_attrdef, 2830, 2831); +DECLARE_TOAST(pg_collation, 4141, 4142); 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_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); 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); +DECLARE_TOAST(pg_user_mapping, 4173, 4174); /* shared catalogs */ -DECLARE_TOAST(pg_shdescription, 2846, 2847); -#define PgShdescriptionToastTable 2846 -#define PgShdescriptionToastIndex 2847 +DECLARE_TOAST(pg_authid, 4175, 4176); +#define PgAuthidToastTable 4175 +#define PgAuthidToastIndex 4176 +DECLARE_TOAST(pg_database, 4177, 4178); +#define PgDatabaseToastTable 4177 +#define PgDatabaseToastIndex 4178 DECLARE_TOAST(pg_db_role_setting, 2966, 2967); #define PgDbRoleSettingToastTable 2966 #define PgDbRoleSettingToastIndex 2967 +DECLARE_TOAST(pg_pltemplate, 4179, 4180); +#define PgPlTemplateToastTable 4179 +#define PgPlTemplateToastIndex 4180 +DECLARE_TOAST(pg_replication_origin, 4181, 4182); +#define PgReplicationOriginToastTable 4181 +#define PgReplicationOriginToastIndex 4182 +DECLARE_TOAST(pg_shdescription, 2846, 2847); +#define PgShdescriptionToastTable 2846 +#define PgShdescriptionToastIndex 2847 DECLARE_TOAST(pg_shseclabel, 4060, 4061); #define PgShseclabelToastTable 4060 #define PgShseclabelToastIndex 4061 +DECLARE_TOAST(pg_subscription, 4183, 4184); +#define PgSubscriptionToastTable 4183 +#define PgSubscriptionToastIndex 4184 +DECLARE_TOAST(pg_tablespace, 4185, 4186);
Re: missing toast table for pg_policy
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
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
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 PgDbRoleSettingToastInde
Re: missing toast table for pg_policy
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
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
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
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
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
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
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
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
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
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