Re: multivariate statistics v14

On 03/21/2016 04:34 AM, Alvaro Herrera wrote:
> Another skim on 0002:
>
> reference.sgml is missing a call to &alterStatistic.
>
> ObjectProperty[] contains a comment that the ACL is "same as relation",
> but is that still correct, given that now stats may be related to more
> than one relation?  Do we even know what the rules for ACLs on
> cross-relation stats are?  One very simple way to get around this is to
> dictate that all the rels must have the same owner.  Perhaps we're not
> considering the multi-relation case yet?

As I wrote in response to Robert's message, I don't think we need ACLs 
for statistics - the user should be able to use them when they can 
access all the underlying relations (in a query). For ALTER STATISTICS 
the (owner || superuser) check should be enough, right?

>
> We have this FIXME comment in do_analyze_rel:
>
> +     * FIXME This sample sizing is mostly OK when computing stats for
> +     *       individual columns, but when computing multi-variate stats
> +     *       for multivariate stats (histograms, mcv, ...) it's rather
> +     *       insufficient. For stats on multiple columns / complex stats
> +     *       we need larger sample sizes, because we need to build more
> +     *       detailed stats (more MCV items / histogram buckets) to get
> +     *       good accuracy. Maybe it'd be appropriate to use samples
> +     *       proportional to the table (say, 0.5% - 1%) instead of a
> +     *       fixed size might be more appropriate. Also, this should be
> +     *       bound to the requested statistics size - e.g. number of MCV
> +     *       items or histogram buckets should require several sample
> +     *       rows per item/bucket (so the sample should be k*size).
>
> Maybe this merits more discussion.  Right now we have an upper bound on
> how much to scan for analyze; if we introduce the idea of scanning a
> percentage of the relation, the time to analyze very large relations
> could increase significantly.  Do we have an idea of what to do for
> this?  For instance, a rule that would make me comfortable would say to
> scan a sample 3x the current size when you have a mvstats on 3 columns;
> then the size of fraction to scan is still bounded.  But does that
> actually work?  From the wording of this comment, I assume you don't
> actually know.

Yeah. I think more discussion is needed, because I myself am not sure 
the FIXME is actually correct. For now I think we're OK with using the 
same logic as statistics on a single column (300 * target).

>
> In this block (CreateStatistics)
> +    /* look for duplicities */
> +    for (i = 0; i < numcols; i++)
> +        for (j = 0; j < numcols; j++)
> +            if ((i != j) && (attnums[i] == attnums[j]))
> +                ereport(ERROR,
> +                        (errcode(ERRCODE_UNDEFINED_COLUMN),
> +                         errmsg("duplicate column name in statistics definition")));
>
> isn't it easier to have the inner loop go from i+1 to numcols?

It probably is.

>
> I wonder if this is sensible with multi-relation statistics:
> +    /*
> +     * Store a dependency too, so that statistics are dropped on DROP TABLE
> +     */
> +    parentobject.classId = RelationRelationId;
> +    parentobject.objectId = ObjectIdGetDatum(RelationGetRelid(rel));
> +    parentobject.objectSubId = 0;
> +    childobject.classId = MvStatisticRelationId;
> +    childobject.objectId = statoid;
> +    childobject.objectSubId = 0;
>
> I suppose the idea is to drop the stats if any of the rels they are for
> is dropped.

What do you mean by sensible? I mean, we don't support multiple tables 
at this point (except for choosing a syntax that should allow that), but 
the code assumes a single relation on a few places (like this one).

>
> Right after that you create a dependency on the schema.  Is that
> necessary?  Since you have the dependency on the relation, the stats
> would be dropped by recursion.

Hmmmm, that's probably right. Also, now that I think about it, it 
probably gets broken after ALTER STATISTICS ... SET SCHEMA, because the 
code does not remove the old dependency (and does not create a new one).

>
> Why are you #include'ing builtins.h everywhere?

Stupidity.

>
> RelationGetMVStatList() needs a comment.

OK.

>
> Please get rid of common.h.  It's totally unlike the way we structure
> our header files.  We don't keep headers in src/backend; they're all in
> src/include.  One reason is that the latter gets installed as a whole in
> include/server, which this file will not be.  This file may be necessary
> to build some extensions in the future, for example.

OK, I'll rework that and move it to src/include/.

>
> In mvstats.h, please mark function prototypes as "extern".
>
> Many files need a pgindent pass.

OK.

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services