> On Nov 18, 2017, at 12:28 PM, Tomas Vondra <[email protected]>
> wrote:
>
> Hi,
>
> Attached is an updated version of the patch, adopting the psql describe
> changes introduced by 471d55859c11b.
>
> regards
>
> --
> Tomas Vondra http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> <0001-multivariate-MCV-lists.patch.gz><0002-multivariate-histograms.patch.gz>
Hello Tomas,
After applying both your patches, I get a warning:
histogram.c:1284:10: warning: taking the absolute value of unsigned type
'uint32' (aka 'unsigned int') has no effect [-Wabsolute-value]
delta = fabs(data->numrows);
^
histogram.c:1284:10: note: remove the call to 'fabs' since unsigned values
cannot be negative
delta = fabs(data->numrows);
^~~~
1 warning generated.
Looking closer at this section, there is some odd integer vs. floating point
arithmetic happening
that is not necessarily wrong, but might be needlessly inefficient:
delta = fabs(data->numrows);
split_value = values[0].value;
for (i = 1; i < data->numrows; i++)
{
if (values[i].value != values[i - 1].value)
{
/* are we closer to splitting the bucket in half? */
if (fabs(i - data->numrows / 2.0) < delta)
{
/* let's assume we'll use this value for the split */
split_value = values[i].value;
delta = fabs(i - data->numrows / 2.0);
nrows = i;
}
}
}
I'm not sure the compiler will be able to optimize out the recomputation of
data->numrows / 2.0
each time through the loop, since the compiler might not be able to prove to
itself that data->numrows
does not get changed. Perhaps you should compute it just once prior to
entering the outer loop,
store it in a variable of integer type, round 'delta' off and store in an
integer, and do integer comparisons
within the loop? Just a thought....
mark
