Subject: Re: multivariate statistics v14
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 Tatsuo Ishii <ishii@postgresql.org>
References: <20160323.110125.1808526964660429411.t-ishii@sraoss.co.jp>
 <20160323.115758.938003466026607400.t-ishii@sraoss.co.jp>
 <20160323.120101.207302357952079425.t-ishii@sraoss.co.jp>
 <20160323.142004.624106180171245380.t-ishii@sraoss.co.jp>
 <eb8522d2-f1ee-7401-2118-e7d6b8990483@2ndquadrant.com>
Cc: jeff.janes@gmail.com, alvherre@2ndquadrant.com,
 pgsql-hackers@postgresql.org
From: Petr Jelinek <petr@2ndquadrant.com>
Message-ID: <56F2DF2C.6070202@2ndquadrant.com>
Date: Wed, 23 Mar 2016 19:23:40 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <eb8522d2-f1ee-7401-2118-e7d6b8990483@2ndquadrant.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

Hi,

I'll add couple of code comments from my first cursory read through 
(this is huge):

0002:
there is some whitespace noise between the varlistentries in 
alter_statistics.sgml

+	parentobject.classId = RelationRelationId;
+	parentobject.objectId = ObjectIdGetDatum(RelationGetRelid(rel));
+	parentobject.objectSubId = 0;
+	childobject.classId = MvStatisticRelationId;
+	childobject.objectId = statoid;
+	childobject.objectSubId = 0;

I wonder if this (several places similar code) would be simpler done 
using ObjectAddressSet()

The common.h in backend/utils/mvstat is slightly weird header file 
placement and naming.


0004:
+/* used for merging bitmaps - AND (min), OR (max) */
+#define MAX(x, y) (((x) > (y)) ? (x) : (y))
+#define MIN(x, y) (((x) < (y)) ? (x) : (y))

Huh? We have Max and Min macros defined in c.h

+		values[Anum_pg_mv_statistic_stamcv  - 1] = PointerGetDatum(data);

Why the double space (that's actually in several places in several of 
the patches).

I don't really understand why 0008 and 0009 are separate patches and 
aren't part of one of the other patches. But otherwise good job on 
splitting the functionality into patchset.

-- 
   Petr Jelinek                  http://www.2ndQuadrant.com/
   PostgreSQL Development, 24x7 Support, 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