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.vondra@2ndquadrant.com> wrote in <1457906378.27231.10.camel@2ndquadrant.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