From: Mark Dilger <hornschnorter@gmail.com>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Adrien Nayrat <adrien.nayrat@dalibo.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] PATCH: multivariate histograms and MCV lists
Date: Sat, 25 Nov 2017 13:01:06 -0800
Message-ID: <B3AF05D4-A312-4988-BC5F-256E756C43CB@gmail.com> (raw)
In-Reply-To: <cf43d6de-617f-83c0-8c14-740c3fa23795@2ndquadrant.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>
> 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.
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: hornschnorter@gmail.com, tomas.vondra@2ndquadrant.com, adrien.nayrat@dalibo.com
Subject: Re: [HACKERS] PATCH: multivariate histograms and MCV lists
In-Reply-To: <B3AF05D4-A312-4988-BC5F-256E756C43CB@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox