Date: Mon, 24 Jun 2019 01:56:36 +0200
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
Message-ID: <20190623235636.s2wlndxdhdpjhowc@development>
References: <20190618205920.qtlzcu73whfpfqne@development>
 <CAEZATCWMDpQUmnSXtRCq_CxLXUoLETra=7SoQ3P5ydiFcse7wQ@mail.gmail.com>
 <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>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="e3n5prx3tb4vrauv"
Content-Disposition: inline
In-Reply-To: <20190623234232.xkydftfd7uxexelo@development>
User-Agent: NeoMutt/20180716-1444-295967
Precedence: bulk


--e3n5prx3tb4vrauv
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline

On Mon, Jun 24, 2019 at 01:42:32AM +0200, Tomas Vondra wrote:
>On Sun, Jun 23, 2019 at 10:23:19PM +0200, Tomas Vondra wrote:
>>On Sun, Jun 23, 2019 at 08:48:26PM +0100, Dean Rasheed wrote:
>>>On Sat, 22 Jun 2019 at 15:10, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
>>>>One annoying thing I noticed is that the base_frequency tends to end up
>>>>being 0, most likely due to getting too small. It's a bit strange, though,
>>>>because with statistic target set to 10k the smallest frequency for a
>>>>single column is 1/3e6, so for 2 columns it'd be ~1/9e12 (which I think is
>>>>something the float8 can represent).
>>>>
>>>
>>>Yeah, it should be impossible for the base frequency to underflow to
>>>0. However, it looks like the problem is with mcv_list_items()'s use
>>>of %f to convert to text, which is pretty ugly.
>>>
>>
>>Yeah, I realized that too, eventually. One way to fix that would be
>>adding %.15f to the sprintf() call, but that just adds ugliness. It's
>>probably time to rewrite the function to build the tuple from datums,
>>instead of relying on BuildTupleFromCStrings.
>>
>
>OK, attached is a patch doing this. It's pretty simple, and it does
>resolve the issue with frequency precision.
>
>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).
>
>In the attached patch I've reworked both to proper arrays, but obviously
>that'd require a CATVERSION bump - and there's not much apetite for that
>past beta2, I suppose. So I'll just undo this bit.
>

Meh, I forgot to attach the patch, of couse ...


-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services 

--e3n5prx3tb4vrauv
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-rework-pg_mcv_list_items.patch"

From 9aa3ce9bfdcab50a6d5dcf289bcf715e4a8aed9c Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
Date: Mon, 24 Jun 2019 00:50:53 +0200
Subject: [PATCH 1/3] rework pg_mcv_list_items

---
 src/backend/statistics/mcv.c    | 106 ++++++++++++--------------------
 src/include/catalog/pg_proc.dat |   2 +-
 2 files changed, 42 insertions(+), 66 deletions(-)

diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index 5fe61ea0a4..1b1a5c9846 100644
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -1134,9 +1134,6 @@ Datum
 pg_stats_ext_mcvlist_items(PG_FUNCTION_ARGS)
 {
 	FuncCallContext *funcctx;
-	int			call_cntr;
-	int			max_calls;
-	AttInMetadata *attinmeta;
 
 	/* stuff done only on the first call of the function */
 	if (SRF_IS_FIRSTCALL())
@@ -1166,13 +1163,13 @@ pg_stats_ext_mcvlist_items(PG_FUNCTION_ARGS)
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 					 errmsg("function returning record called in context "
 							"that cannot accept type record")));
+		tupdesc = BlessTupleDesc(tupdesc);
 
 		/*
 		 * generate attribute metadata needed later to produce tuples from raw
 		 * C strings
 		 */
-		attinmeta = TupleDescGetAttInMetadata(tupdesc);
-		funcctx->attinmeta = attinmeta;
+		funcctx->attinmeta = TupleDescGetAttInMetadata(tupdesc);
 
 		MemoryContextSwitchTo(oldcontext);
 	}
@@ -1180,18 +1177,17 @@ pg_stats_ext_mcvlist_items(PG_FUNCTION_ARGS)
 	/* stuff done on every call of the function */
 	funcctx = SRF_PERCALL_SETUP();
 
-	call_cntr = funcctx->call_cntr;
-	max_calls = funcctx->max_calls;
-	attinmeta = funcctx->attinmeta;
-
-	if (call_cntr < max_calls)	/* do when there is more left to send */
+	if (funcctx->call_cntr < funcctx->max_calls)	/* do when there is more left to send */
 	{
-		char	  **values;
+		Datum		values[5];
+		bool		nulls[5];
 		HeapTuple	tuple;
 		Datum		result;
+		int			dims[1];
+		int			lbs[1];
 
-		StringInfoData itemValues;
-		StringInfoData itemNulls;
+		Datum		  *itemNulls;
+		Datum		  *itemValues;
 
 		int			i;
 
@@ -1203,24 +1199,16 @@ pg_stats_ext_mcvlist_items(PG_FUNCTION_ARGS)
 
 		mcvlist = (MCVList *) funcctx->user_fctx;
 
-		Assert(call_cntr < mcvlist->nitems);
-
-		item = &mcvlist->items[call_cntr];
-
-		/*
-		 * Prepare a values array for building the returned tuple. This should
-		 * be an array of C strings which will be processed later by the type
-		 * input functions.
-		 */
-		values = (char **) palloc0(5 * sizeof(char *));
+		Assert(funcctx->call_cntr < mcvlist->nitems);
 
-		values[0] = (char *) palloc(64 * sizeof(char)); /* item index */
-		values[3] = (char *) palloc(64 * sizeof(char)); /* frequency */
-		values[4] = (char *) palloc(64 * sizeof(char)); /* base frequency */
+		item = &mcvlist->items[funcctx->call_cntr];
 
 		outfuncs = (Oid *) palloc0(sizeof(Oid) * mcvlist->ndimensions);
 		fmgrinfo = (FmgrInfo *) palloc0(sizeof(FmgrInfo) * mcvlist->ndimensions);
 
+		itemValues = (Datum *) palloc(sizeof(Datum) * mcvlist->ndimensions);
+		itemNulls = (Datum *) palloc(sizeof(Datum) * mcvlist->ndimensions);
+
 		for (i = 0; i < mcvlist->ndimensions; i++)
 		{
 			bool		isvarlena;
@@ -1230,61 +1218,49 @@ pg_stats_ext_mcvlist_items(PG_FUNCTION_ARGS)
 			fmgr_info(outfuncs[i], &fmgrinfo[i]);
 		}
 
-		/* build the arrays of values / nulls */
-		initStringInfo(&itemValues);
-		initStringInfo(&itemNulls);
-
-		appendStringInfoChar(&itemValues, '{');
-		appendStringInfoChar(&itemNulls, '{');
-
 		for (i = 0; i < mcvlist->ndimensions; i++)
 		{
-			Datum		val,
-						valout;
-
-			if (i > 0)
-			{
-				appendStringInfoString(&itemValues, ", ");
-				appendStringInfoString(&itemNulls, ", ");
-			}
+			Datum		valout;
 
 			if (item->isnull[i])
-				valout = CStringGetDatum("NULL");
+				valout = (Datum) 0;
 			else
-			{
-				val = item->values[i];
-				valout = FunctionCall1(&fmgrinfo[i], val);
-			}
+				valout = FunctionCall1(&fmgrinfo[i], item->values[i]);
 
-			appendStringInfoString(&itemValues, DatumGetCString(valout));
-			appendStringInfoString(&itemNulls, item->isnull[i] ? "t" : "f");
+			itemNulls[i] = BoolGetDatum(item->isnull[i]);
+			itemValues[i] = valout;
 		}
 
-		appendStringInfoChar(&itemValues, '}');
-		appendStringInfoChar(&itemNulls, '}');
+		dims[0] = mcvlist->ndimensions;
+		lbs[0] = 1;
+
+		values[0] = Int32GetDatum(funcctx->call_cntr);
 
-		snprintf(values[0], 64, "%d", call_cntr);
-		snprintf(values[3], 64, "%f", item->frequency);
-		snprintf(values[4], 64, "%f", item->base_frequency);
+		values[1] = PointerGetDatum(construct_md_array(itemValues,
+													   item->isnull,
+													   1,
+													   dims,
+													   lbs,
+													   CSTRINGOID, -2,
+													   false, 'c'));
 
-		values[1] = itemValues.data;
-		values[2] = itemNulls.data;
+		values[2] = PointerGetDatum(construct_array(itemNulls,
+													mcvlist->ndimensions,
+													BOOLOID, sizeof(bool),
+													true, 'c'));
+
+		values[3] = Float8GetDatum(item->frequency);
+		values[4] = Float8GetDatum(item->base_frequency);
+
+		/* no NULLs in the tuple */
+		memset(nulls, 0, sizeof(nulls));
 
 		/* build a tuple */
-		tuple = BuildTupleFromCStrings(attinmeta, values);
+		tuple = heap_form_tuple(funcctx->attinmeta->tupdesc, values, nulls);
 
 		/* make the tuple into a datum */
 		result = HeapTupleGetDatum(tuple);
 
-		/* clean up (this is not really necessary) */
-		pfree(itemValues.data);
-		pfree(itemNulls.data);
-
-		pfree(values[0]);
-		pfree(values[3]);
-		pfree(values[4]);
-		pfree(values);
-
 		SRF_RETURN_NEXT(funcctx, result);
 	}
 	else						/* do when there is no more left */
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 87335248a0..36ae2ac9d8 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5019,7 +5019,7 @@
 { oid => '3427', descr => 'details about MCV list items',
   proname => 'pg_mcv_list_items', prorows => '1000', proretset => 't',
   provolatile => 's', prorettype => 'record', proargtypes => 'pg_mcv_list',
-  proallargtypes => '{pg_mcv_list,int4,text,_bool,float8,float8}',
+  proallargtypes => '{pg_mcv_list,int4,_text,_bool,float8,float8}',
   proargmodes => '{i,o,o,o,o,o}',
   proargnames => '{mcv_list,index,values,nulls,frequency,base_frequency}',
   prosrc => 'pg_stats_ext_mcvlist_items' },
-- 
2.20.1


--e3n5prx3tb4vrauv--