From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Petr Jelinek <petr@2ndquadrant.com>
To: Tatsuo Ishii <ishii@postgresql.org>
Cc: jeff.janes@gmail.com
Cc: alvherre@2ndquadrant.com
Cc: pgsql-hackers@postgresql.org
Subject: Re: multivariate statistics v14
Date: Thu, 24 Mar 2016 18:12:58 +0100
Message-ID: <5b18d0a1-4dc7-78c7-784d-e1497e001675@2ndquadrant.com> (raw)
In-Reply-To: <56F2DF2C.6070202@2ndquadrant.com>
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>
<56F2DF2C.6070202@2ndquadrant.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Hi,
attached is v17 of the patch series, with these changes:
* rebase to current master (the AM patch caused some conflicts)
* add alterStatistics to reference.sgml (Alvaro)
* move the sample size discussion to README.stats (Alvaro)
* tweak the inner for loop in CREATE STATISTICS (Alvaro)
* use ObjectAddressSet() to create dependencies in statscmds.c (Petr)
* fix whitespace in alterStatistics.sgml (Petr)
* replace custom MIN/MAX with Min/Max in c.h (Petr)
* fix examples in createStatistics.sgml (Tatsuo)
A few more comments inline:
On 03/23/2016 07:23 PM, Petr Jelinek wrote:
>
> The common.h in backend/utils/mvstat is slightly weird header file
> placement and naming.
>
True. I plan to move this header to
src/include/catalog/pg_mv_statistic_fn.h
which is what the other catalogs do (as pointed by Alvaro). Or do you
think another location/name would be more appropriate?
>
> + values[Anum_pg_mv_statistic_stamcv - 1] = PointerGetDatum(data);
>
> Why the double space (that's actually in several places in several of
> the patches).
To align the whole block like this:
nulls[Anum_pg_mv_statistic_stadeps -1] = true;
nulls[Anum_pg_mv_statistic_stamcv -1] = true;
nulls[Anum_pg_mv_statistic_stahist -1] = true;
nulls[Anum_pg_mv_statistic_standist -1] = true;
But I won't fight for this too hard, if it breaks rules somehow.
>
> 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.
That is mostly because both 0007 and 0008 tweak the GROUP BY estimates,
but 0008 is not really part of this patch (it's discussed separately in
another thread). I admit it may be a bit confusing.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:
[application/x-compressed-tar] multivariate-stats-v17.tgz (144.9K, ../5b18d0a1-4dc7-78c7-784d-e1497e001675@2ndquadrant.com/2-multivariate-stats-v17.tgz)
download
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: tomas.vondra@2ndquadrant.com, petr@2ndquadrant.com, ishii@postgresql.org, jeff.janes@gmail.com, alvherre@2ndquadrant.com
Subject: Re: multivariate statistics v14
In-Reply-To: <5b18d0a1-4dc7-78c7-784d-e1497e001675@2ndquadrant.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