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

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

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

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

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

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

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

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

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

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

Show quoted text

Joe

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

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

Show quoted text

Michael