From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Choosing values for multivariate MCV lists
Date: Tue, 2 Jul 2019 12:25:55 +0200
Message-ID: <20190702102555.qp3smvbwas7f6pc2@development> (raw)
In-Reply-To: <CAEZATCWBXQ=P0oTEOauLYKWW_s2di0ZvTZGwZ_6_KaHwOrJgQA@mail.gmail.com>
References: <20190620223548.p22wj7uejbrhkglf@development>
<CAEZATCWqqB3R+Oewq2u_ByS01-12+M4=yRARVvdmx4R0ZO-RvQ@mail.gmail.com>
<20190622141052.5csokbp3sx577mub@development>
<CAEZATCVQYRScftGh-k_An8yo0d=GwNKzEUOR=t4g7Zw9Qd1=kQ@mail.gmail.com>
<20190623202319.4lcqsi5zc53hktda@development>
<20190623234232.xkydftfd7uxexelo@development>
<CAEZATCU5O1y09w0u4BpV5sY=X+KGomuwP5vvjdp8QFWTW+VDTQ@mail.gmail.com>
<20190625091819.uslpopydrkqm3sdf@development>
<20190629130126.53rshkfu2go6atkl@development>
<CAEZATCWBXQ=P0oTEOauLYKWW_s2di0ZvTZGwZ_6_KaHwOrJgQA@mail.gmail.com>
On Mon, Jul 01, 2019 at 12:02:28PM +0100, Dean Rasheed wrote:
>On Sat, 29 Jun 2019 at 14:01, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>>
>> >>>>>However, it looks like the problem is with mcv_list_items()'s use
>> >>>>>of %f to convert to text, which is pretty ugly.
>> >>>>
>> >>>There's one issue with the signature, though - currently the function
>> >>>returns null flags as bool array, but values are returned as simple
>> >>>text value (formatted in array-like way, but still just a text).
>> >>>
>> >>IMO fixing this to return a text array is worth doing, even though it
>> >>means a catversion bump.
>> >>
>> Attached is a cleaned-up version of that patch. The main difference is
>> that instead of using construct_md_array() this uses ArrayBuildState to
>> construct the arrays, which is much easier. The docs don't need any
>> update because those were already using text[] for the parameter, the
>> code was inconsistent with it.
>>
>
>Cool, this looks a lot neater and fixes the issues discussed with both
>floating point values no longer being converted to text, and proper
>text arrays for values.
>
>One minor nitpick -- it doesn't seem to be necessary to build the
>arrays *outfuncs and *fmgrinfo. You may as well just fetch the
>individual column output function information at the point where it's
>used (in the "if (!item->isnull[i])" block) rather than building those
>arrays.
>
OK, thanks for the feedback. I'll clean-up the function lookup.
>
>> This does require catversion bump, but as annoying as it is, I think
>> it's worth it (and there's also the thread discussing the serialization
>> issues). Barring objections, I'll get it committed later next week, once
>> I get back from PostgresLondon.
>>
>> As I mentioned before, if we don't want any additional catversion bumps,
>> it's possible to pass the arrays through output functions - that would
>> allow us keeping the text output (but correct, unlike what we have now).
>>
>
>I think this is a clear bug fix, so I'd vote for fixing it properly
>now, with a catversion bump.
>
I agree.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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, dean.a.rasheed@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Choosing values for multivariate MCV lists
In-Reply-To: <20190702102555.qp3smvbwas7f6pc2@development>
* 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