Re: [Patch] Invalid permission check in pg_stats for functionalindexes

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
hashedSubPlan 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