Re: [Patch] Invalid permission check in pg_stats for functional indexes

On 2019-Sep-03, Pierre Ducroquet wrote:

> > IIUC, the patch introduces an additional privilege check for the
> > underlying objects involved in the expression/functional index. If the
> > user has 'select' privileges on all of the columns/objects included in
> > the expression/functional index, then it should be visible in pg_stats
> > view. I've applied the patch manually and tested the feature. It works
> > as expected.
>
> Indeed, you understood correctly. I have not digged around to find out the
> origin of the current situation, but it does not look like an intentional
> behaviour, more like a small oversight.

Hmm. This seems to create a large performance drop. I created your
view as pg_stats2 alongside pg_stats, and ran EXPLAIN on both for the
query you posted. Look at the plan with the original query:

55432 13devel 10881=# explain select tablename, attname from pg_stats where tablename like 'tbl1%';
QUERY PLAN
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Subquery Scan on pg_stats (cost=129.79..156.46 rows=1 width=128)
Filter: (pg_stats.tablename ~~ 'tbl1%'::text)
-> Hash Join (cost=129.79..156.39 rows=5 width=401)
Hash Cond: ((s.starelid = c.oid) AND (s.staattnum = a.attnum))
-> Index Only Scan using pg_statistic_relid_att_inh_index on pg_statistic s (cost=0.27..22.60 rows=422 width=6)
-> Hash (cost=114.88..114.88 rows=976 width=138)
-> Hash Join (cost=22.90..114.88 rows=976 width=138)
Hash Cond: (a.attrelid = c.oid)
Join Filter: has_column_privilege(c.oid, a.attnum, 'select'::text)
-> Seq Scan on pg_attribute a (cost=0.00..84.27 rows=2927 width=70)
Filter: (NOT attisdropped)
-> Hash (cost=17.95..17.95 rows=396 width=72)
-> Seq Scan on pg_class c (cost=0.00..17.95 rows=396 width=72)
Filter: ((NOT relrowsecurity) OR (NOT row_security_active(oid)))
(14 filas)

and here's the plan with your modified view:

55432 13devel 10881=# explain select tablename, attname from pg_stats2 where tablename like 'tbl1%';
QUERY PLAN
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Subquery Scan on pg_stats2 (cost=128.72..6861.85 rows=1 width=128)
Filter: (pg_stats2.tablename ~~ 'tbl1%'::text)
-> Nested Loop (cost=128.72..6861.80 rows=4 width=401)
Join Filter: (s.starelid = c.oid)
-> Hash Join (cost=128.45..152.99 rows=16 width=74)
Hash Cond: ((s.starelid = a.attrelid) AND (s.staattnum = a.attnum))
-> Index Only Scan using pg_statistic_relid_att_inh_index on pg_statistic s (cost=0.27..22.60 rows=422 width=6)
-> Hash (cost=84.27..84.27 rows=2927 width=70)
-> Seq Scan on pg_attribute a (cost=0.00..84.27 rows=2927 width=70)
Filter: (NOT attisdropped)
-> Index Scan using pg_class_oid_index on pg_class c (cost=0.27..419.29 rows=1 width=73)
Index Cond: (oid = a.attrelid)
Filter: (((NOT relrowsecurity) OR (NOT row_security_active(oid))) AND ((relkind = 'r'::"char") OR ((relkind = 'i'::"char") AND (NOT (alternatives: SubPlan 1 or hashed SubPlan 2)))) AND (((relkind = 'r'::"char") AND has_column_privilege(oid, a.attnum, 'select'::text)) OR ((relkind = 'i'::"char") AND (NOT (alternatives: SubPlan 1 or hashed SubPlan 2)))))
SubPlan 1
-> Seq Scan on pg_depend (cost=0.00..209.48 rows=1 width=0)
Filter: ((refobjsubid > 0) AND (objid = c.oid) AND (NOT has_column_privilege(refobjid, (refobjsubid)::smallint, 'select'::text)))
SubPlan 2
-> Seq Scan on pg_depend pg_depend_1 (cost=0.00..190.42 rows=176 width=4)
Filter: ((refobjsubid > 0) AND (NOT has_column_privilege(refobjid, (refobjsubid)::smallint, 'select'::text)))
(19 filas)

You forgot to add a condition `pg_depend.classid =
'pg_catalog.pg_class'::pg_catalog.regclass` in your subquery (fixing
that probably improves the plan a lot); but more generally I'm not sure
that querying pg_depend is an acceptable way to go about this. I have
to admit I don't see any other way to get a list of columns involved in
an expression, though. Maybe we need to introduce a function that
returns the set of columns involved in an index (which should include
the column in a WHERE clause if any, I suppose.)

What about relkind='m'?

I'm not sure about a writing a test for this. Do we have any tests for
privileges here?

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services