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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Hmm. This seems to create a large performance drop.

Yeah. It's also flat out wrong for indexes that depend on whole-row
variables. For that case, we really need to insist that the user
have select privilege on all the table columns, but this won't
accomplish that. It ignores pg_depend entries with refobjsubid = 0,
and there's no good way to expand those even if it didn't.

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

pg_depend isn't ideal for this, I agree. It serves other masters.

> 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.)

I agree that some C function that inspects the index definition is
probably needed here. Not sure exactly what it should look like.

We might be well advised to bury the whole business in a function
like "has_index_column_privilege(index_oid, col, priv_type)" rather
than implementing that partly in SQL and partly in C. The performance
would be better, and we'd have more flexibility to fix issues without
forcing new initdb's.

On the other hand, a SQL function that just parses the index definition
and returns relevant column number(s) might be useful for other
purposes, so maybe we should write that alongside this.

> What about relkind='m'?

As coded, this certainly breaks pg_stat for those, and for foreign tables
as well. Likely better to write something like
"case when relkind = 'i' then do-something-for-indexes else old-code end".

Actually ... maybe we don't need to change the view definition at all,
but instead just make has_column_privilege() do something different
for indexes than it does for other relation types. It's dubious that
applying that function to an index yields anything meaningful today,
so we could redefine what it returns without (probably) breaking
anything. That would at least give us an option to back-patch, too,
though the end result might be complex enough that we don't care to
risk it.

I wonder which of the other has_xxx_privilege tests are likewise
in need of rethink for indexes.

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

I don't think we have any meaningful tests for the info-schema views
as such. However, if we redefine the problem as "has_column_privilege
on an index does the wrong thing", then privileges.sql is a natural
home for testing that because it already has test cases for that
function.

regards, tom lane