Re: PoC/WIP: Extended statistics on expressions - Mailing list pgsql-hackers

From Tomas Vondra
Subject Re: PoC/WIP: Extended statistics on expressions
Date
Msg-id afcd65d7-7701-a469-b064-9c62d63ef437@enterprisedb.com
Whole thread Raw
In response to Re: PoC/WIP: Extended statistics on expressions  (Dean Rasheed <dean.a.rasheed@gmail.com>)
Responses Re: PoC/WIP: Extended statistics on expressions
List pgsql-hackers
Hi,

Attached is a patch fixing most of the issues. There are a couple 
exceptions:


 > * Looking at the pg_stats_ext view, I think perhaps expressions stats
 > should be omitted entirely from that view, since it doesn't show any
 > useful information about them. So it could remove "e" from the "kinds"
 > array, and exclude rows whose only kind is "e", since such rows have
 > no interesting data in them. Essentially, the new view
 > pg_stats_ext_exprs makes having any expression stats in pg_stats_ext
 > redundant. Hiding this data in pg_stats_ext would also be consistent
 > with making the "expressions" stats kind hidden from the user.

I haven't removed the expressions stats from pg_stats_ext view yet. I'm 
not 100% sure about it yet.


 > * Why does the new code in tcop/utility.c not use
 > RangeVarGetRelidExtended together with RangeVarCallbackOwnsRelation?
 > That seems preferable to doing the ACL check in CreateStatistics().
 > For one thing, as it stands, it allows the lock to be taken even if
 > the user doesn't own the table. Is it intentional that the current
 > code allows extended stats to be created on system catalogs? That
 > would be one thing that using RangeVarCallbackOwnsRelation would
 > change, but I can't see a reason to allow it.

I haven't switched utility.c to RangeVarGetRelidExtended together with 
RangeVarCallbackOwnsRelation, because the current code allows checking 
for object type first. I don't recall why exactly was it done this way, 
but I didn't feel like changing that in this patch.

You're however right it should not be possible to create statistics on 
system catalogs. For regular users that should be rejected thanks to the 
ownership check, but superuser may create it. I've added proper check to 
CreateStatistics() - this is probably worth backpatching.


 > * In src/bin/psql/describe.c, I think the \d output should also
 > exclude the "expressions" stats kind and just list the other kinds (or
 > have no kinds list at all, if there are no other kinds), to make it
 > consistent with the CREATE STATISTICS syntax.

I've done this, but I went one step further - we hide the list of kinds 
using the same rules as pg_dump, i.e. we don't list the kinds if all of 
them are selected. Not sure if that's the right thing, though.


The rest of the issues should be fixed, I think.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachment

pgsql-hackers by date:

Previous
From: Bruce Momjian
Date:
Subject: Re: Key management with tests
Next
From: Thomas Munro
Date:
Subject: Default wal_sync_method on FreeBSD