Content-Type: text/plain; charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\))
Subject: Re: [HACKERS] PATCH: multivariate histograms and MCV lists
From: Mark Dilger <hornschnorter@gmail.com>
In-Reply-To: <cf43d6de-617f-83c0-8c14-740c3fa23795@2ndquadrant.com>
Date: Sat, 25 Nov 2017 13:01:06 -0800
Cc: Adrien Nayrat <adrien.nayrat@dalibo.com>,
 Pg Hackers <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <B3AF05D4-A312-4988-BC5F-256E756C43CB@gmail.com>
References: <dfdac334-9cf2-2597-fb27-f0fb3753f435@2ndquadrant.com>
 <31ee8686-6d62-7f10-4129-7580ec74a710@dalibo.com>
 <b3688203-7c5d-2d7e-58b6-a6bb04f29b5c@2ndquadrant.com>
 <340ffd44-2d67-d39d-e54f-5a97a842cd3a@2ndquadrant.com>
 <b3e23e18-afbb-2a39-8e15-fe0a6c39a128@2ndquadrant.com>
 <67EAB5B3-F9FB-4753-8102-4D77DCFFFCD4@gmail.com>
 <cf43d6de-617f-83c0-8c14-740c3fa23795@2ndquadrant.com>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Precedence: bulk


> On Nov 18, 2017, at 12:28 PM, Tomas Vondra =
<tomas.vondra@2ndquadrant.com> wrote:
>=20
> Hi,
>=20
> Attached is an updated version of the patch, adopting the psql =
describe
> changes introduced by 471d55859c11b.

Hi Tomas,

In src/backend/statistics/dependencies.c, you have introduced a comment:

+       /*
+        * build an array of SortItem(s) sorted using the multi-sort =
support
+        *
+        * XXX This relies on all stats entries pointing to the same =
tuple
+        * descriptor. Not sure if that might not be the case.
+        */

Would you mind explaining that a bit more for me?  I don't understand =
exactly what
you mean here, but it sounds like the sort of thing that needs to be =
clarified/fixed
before it can be committed.  Am I misunderstanding this?


In src/backend/statistics/mcv.c, you have comments:

+ * FIXME: Single-dimensional MCV is sorted by frequency (descending). =
We
+ * should do that too, because when walking through the list we want to
+ * check the most frequent items first.
+ *
+ * TODO: We're using Datum (8B), even for data types (e.g. int4 or =
float4).
+ * Maybe we could save some space here, but the bytea compression =
should
+ * handle it just fine.
+ *
+ * TODO: This probably should not use the ndistinct directly (as =
computed from
+ * the table, but rather estimate the number of distinct values in the
+ * table), no?

Do you intend these to be fixed/implemented prior to committing this =
patch?


Further down in function statext_mcv_build, you have two loops, the =
first allocating
memory and the second initializing the memory.  There is no clear reason =
why this
must be done in two loops.  I tried combining the two loops into one, =
and it worked
just fine, but did not look any cleaner to me.  Feel free to disregard =
this paragraph
if you like it better the way you currently have it organized.


Further down in statext_mcv_deserialize, you have some elogs which might =
need to be
ereports.  It is unclear to me whether you consider these deserialize =
error cases to be
"can't happen" type errors.  If so, you might add that fact to the =
comments rather than
changing the elogs to ereports.


mark