From: | Petr Jelinek <petr(at)2ndquadrant(dot)com> |
---|---|
To: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Tatsuo Ishii <ishii(at)postgresql(dot)org> |
Cc: | jeff(dot)janes(at)gmail(dot)com, alvherre(at)2ndquadrant(dot)com, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: multivariate statistics v14 |
Date: | 2016-03-23 18:23:40 |
Message-ID: | 56F2DF2C.6070202@2ndquadrant.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | David G. Johnston | 2016-03-23 18:27:23 | Re: PostgreSQL 9.6 behavior change with set returning (funct).* |
Previous Message | Vladimir Sitnikov | 2016-03-23 18:15:32 | Re: NOT EXIST for PREPARE |