Re: multivariate statistics v14

On 03/16/2016 09:31 AM, Kyotaro HORIGUCHI wrote:
> Hello, I returned to this.
>
> At Sun, 13 Mar 2016 22:59:38 +0100, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote in <1457906378(dot)27231(dot)10(dot)camel(at)2ndquadrant(dot)com>
>> Oh, yeah. There was an extra pfree().
>>
>> Attached is v15 of the patch series, fixing this and also doing quite a
>> few additional improvements:
>>
>> * added some basic examples into the SGML documentation
>>
>> * addressing the objectaddress omissions, as pointed out by Alvaro
>>
>> * support for ALTER STATISTICS ... OWNER TO / RENAME / SET SCHEMA
>>
>> * significant refactoring of MCV and histogram code, particularly
>> serialization, deserialization and building
>>
>> * reworking the functional dependencies to support more complex
>> dependencies, with multiple columns as 'conditions'
>>
>> * the reduction using functional dependencies is also significantly
>> simplified (I decided to get rid of computing the transitive closure
>> for now - it got too complex after the multi-condition dependencies,
>> so I'll leave that for the future
>
> Many trailing white spaces found.

Sorry, haven't noticed that after one of the rebases. Fixed in the
attached v15 of the patch.

>
> 0002
>
> + * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
>
> 2014 should be 2016?

Yes, the copyright info will need some tweaks. There's a few other files
with 2015, and I think the start should be the current year (and not 1996).

>
>
> This patch defines many "magic"s for many structs, but
> magic(number)s seems to be used to identify file or buffer page
> in PostgreSQL. They wouldn't be needed if you don't intend to
> dig out or identify the orphan memory blocks of mvstats.
>
> + MVDependency deps[1]; /* XXX why not a pointer? */
>
> MVDependency seems to be a pointer type.

Right, but we need an array of the structures here, so one way is to use
a pointer and the other one is using variable-length field. Will remove
the comment, I think the structure is fine as is.

>
> + if (numcols >= MVSTATS_MAX_DIMENSIONS)
> + ereport(ERROR,
> and
> + Assert((attrs->dim1 >= 2) && (attrs->dim1 <= MVSTATS_MAX_DIMENSIONS));
>
> seem to be contradicting.

Nope, because the first check is in a loop where 'numcols' is used as an
index into an array with MVSTATS_MAX_DIMENSIONS elements.

>
> .. Sorry, time is up..

Thanks for the comments!

Attached is v15 of the patch, that also fixes one mistake - after
reworking the functional dependencies to support multiple columns on the
left side (as conditions), I failed to move it to the proper place in
the patch series. So 0002 built the dependencies in the old way and 0003
changed it to the new one. That was pointless and added another 20kB to
the patch, so v15 moves the new code to 0002.

regards

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