From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Tatsuo Ishii <ishii@postgresql.org>
Cc: horiguchi.kyotaro@lab.ntt.co.jp
Cc: jeff.janes@gmail.com
Cc: pgsql-hackers@postgresql.org
Subject: Re: multivariate statistics v14
Date: Mon, 21 Mar 2016 00:34:40 -0300
Message-ID: <20160321033440.GA350186@alvherre.pgsql> (raw)
In-Reply-To: <1bc2bd16-8e33-9e22-cdb1-9ddc38fedc4c@2ndquadrant.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
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?
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.
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?
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.
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.
Why are you #include'ing builtins.h everywhere?
RelationGetMVStatList() needs a comment.
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.
In mvstats.h, please mark function prototypes as "extern".
Many files need a pgindent pass.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@2ndquadrant.com, tomas.vondra@2ndquadrant.com, ishii@postgresql.org, horiguchi.kyotaro@lab.ntt.co.jp, jeff.janes@gmail.com
Subject: Re: multivariate statistics v14
In-Reply-To: <20160321033440.GA350186@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox