Re: [HACKERS] PATCH: multivariate histograms and MCV lists

> On Nov 18, 2017, at 12:28 PM, Tomas Vondra <tomas.vondra@2ndquadrant.com> 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