Thread: missing toast table for pg_policy
Currently if you try to create a too large policy, it fails with: ERROR: row is too big: size XXXXX, maximum size 8160 An example for reproducing this is attached. 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. Thoughts? -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Attachment
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
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
Attachment
Joe Conway <mail@joeconway.com> 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
On 02/16/2018 05:24 PM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> 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
Attachment
Joe Conway <mail@joeconway.com> 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
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
Andres Freund <andres@anarazel.de> 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
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
Attachment
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
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
Attachment
On 02/17/2018 11:39 AM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> 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: <snip> > 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 real 0m16.295s user 0m3.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 real 0m16.462s user 0m3.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
Attachment
Joe Conway <mail@joeconway.com> 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
On 02/18/2018 11:18 AM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> 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
Attachment
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
Attachment
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
Attachment
On Sun, Feb 18, 2018 at 10:43 AM, Joe Conway <mail@joeconway.com> 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
On 2/19/18, Joe Conway <mail@joeconway.com> 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 >
Attachment
On 06/15/2018 02:40 PM, John Naylor wrote: > On 2/19/18, Joe Conway <mail@joeconway.com> 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
On 2/20/18, Michael Paquier <michael@paquier.xyz> 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 >
Attachment
On 2018-02-18 11:18:49 -0500, Tom Lane wrote: > Joe Conway <mail@joeconway.com> 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
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
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
Attachment
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
Attachment
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
Attachment
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
On 7/12/18, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
Attachment
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
Attachment
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
Attachment
On 7/17/18, Michael Paquier <michael@paquier.xyz> 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 <jcnaylor@gmail.com> wrote: > On 2/20/18, Michael Paquier <michael@paquier.xyz> 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
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
Attachment
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
Attachment
On 7/17/18, Michael Paquier <michael@paquier.xyz> 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
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
Attachment
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
Andres Freund <andres@anarazel.de> 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
On Thu, Jul 19, 2018 at 07:18:32PM -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> 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
Attachment
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 <andres@anarazel.de> 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
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
Attachment
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