Re: [HACKERS] type cache for concat functions

[Pavel Stehule]

2017-09-19 21:11 GMT+02:00 Tom Lane :

> Pavel Stehule  writes:
> > [ faster-concat-2.patch ]
>
> Pushed with some cosmetic adjustments (mostly better comments).
>

Thank you very much

Pavel


> regards, tom lane
>

Re: [HACKERS] type cache for concat functions

[Tom Lane]

Pavel Stehule  writes:
> [ faster-concat-2.patch ]

Pushed with some cosmetic adjustments (mostly better comments).

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] type cache for concat functions

[Pavel Stehule]

2017-09-19 12:18 GMT+02:00 Alexander Kuzmenkov :

> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   tested, passed
> Documentation:tested, passed
>
> Looks good to me.
>
> The new status of this patch is: Ready for Committer
>

Thank you very much

Pavel


> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: [HACKERS] type cache for concat functions

[Alexander Kuzmenkov]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

Looks good to me.

The new status of this patch is: Ready for Committer

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] type cache for concat functions

[Pavel Stehule]

Hi

2017-08-24 19:24 GMT+02:00 Alexander Kuzmenkov :

> Hi Pavel,
>
> I tried applying your patch, it applies and compiles fine, check and
> checkworld pass.
>
> I ran a simple performance test, select concat(generate_series(1,10),
> ... [x5 total]) vs select generate_series(1,10)::text || ... .
> Operator || runs in 60 ms, while unpatched concat takes 90 and patched --
> 55 ms.
>
> About the code:
> * There seems to be an extra tab here:
> FmgrInfo*typcache;
> It's not aligned with the previous declaration with tab size 4.
>
> * You could allocate the cache and store it into the context inside
> rebuildConcatCache. It would return return the cache pointer, and the
> calling code would look cleaner -- just one line. This is a matter of taste
> though.
>
> * The nearby functions use snake_case, so it should be
> rebuild_concat_cache too.
>

all should be fixed.

Regards

Pavel


>
> Overall the patch looks good to me.
>
> --
> Alexander Kuzmenkov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index ebfb823fb8..24d7512320 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -4734,6 +4734,38 @@ string_agg_finalfn(PG_FUNCTION_ARGS)
 }
 
 /*
+ * Prepare cache with typeOutput fmgr info for any argument of
+ * concat like function.
+ */
+static FmgrInfo *
+build_concat_typcache(FunctionCallInfo fcinfo, int argidx)
+{
+	FmgrInfo *typcache;
+	int		i;
+
+	typcache = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+		PG_NARGS() * sizeof(FmgrInfo));
+
+	for (i = argidx; i < PG_NARGS(); i++)
+	{
+		Oid			valtype;
+		Oid			typOutput;
+		bool		typIsVarlena;
+
+		valtype = get_fn_expr_argtype(fcinfo->flinfo, i);
+		if (!OidIsValid(valtype))
+			elog(ERROR, "could not determine data type of concat() input");
+
+		getTypeOutputInfo(valtype, , );
+		fmgr_info_cxt(typOutput, [i-argidx], fcinfo->flinfo->fn_mcxt);
+	}
+
+	fcinfo->flinfo->fn_extra = typcache;
+
+	return typcache;
+}
+
+/*
  * Implementation of both concat() and concat_ws().
  *
  * sepstr is the separator string to place between values.
@@ -4748,6 +4780,7 @@ concat_internal(const char *sepstr, int argidx,
 	StringInfoData str;
 	bool		first_arg = true;
 	int			i;
+	FmgrInfo   *typcache;
 
 	/*
 	 * concat(VARIADIC some-array) is essentially equivalent to
@@ -4787,14 +4820,15 @@ concat_internal(const char *sepstr, int argidx,
 	/* Normal case without explicit VARIADIC marker */
 	initStringInfo();
 
+	typcache = (FmgrInfo *) fcinfo->flinfo->fn_extra;
+	if (typcache == NULL)
+		typcache = build_concat_typcache(fcinfo, argidx);
+
 	for (i = argidx; i < PG_NARGS(); i++)
 	{
 		if (!PG_ARGISNULL(i))
 		{
 			Datum		value = PG_GETARG_DATUM(i);
-			Oid			valtype;
-			Oid			typOutput;
-			bool		typIsVarlena;
 
 			/* add separator if appropriate */
 			if (first_arg)
@@ -4802,13 +4836,8 @@ concat_internal(const char *sepstr, int argidx,
 			else
 appendStringInfoString(, sepstr);
 
-			/* call the appropriate type output function, append the result */
-			valtype = get_fn_expr_argtype(fcinfo->flinfo, i);
-			if (!OidIsValid(valtype))
-elog(ERROR, "could not determine data type of concat() input");
-			getTypeOutputInfo(valtype, , );
 			appendStringInfoString(,
-   OidOutputFunctionCall(typOutput, value));
+	OutputFunctionCall([i-argidx], value));
 		}
 	}
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] type cache for concat functions

[Alexander Kuzmenkov]

Hi Pavel,

I tried applying your patch, it applies and compiles fine, check and 
checkworld pass.


I ran a simple performance test, select 
concat(generate_series(1,10), ... [x5 total]) vs select 
generate_series(1,10)::text || ... .
Operator || runs in 60 ms, while unpatched concat takes 90 and patched 
-- 55 ms.


About the code:
* There seems to be an extra tab here:
FmgrInfo*typcache;
It's not aligned with the previous declaration with tab size 4.

* You could allocate the cache and store it into the context inside 
rebuildConcatCache. It would return return the cache pointer, and the 
calling code would look cleaner -- just one line. This is a matter of 
taste though.


* The nearby functions use snake_case, so it should be 
rebuild_concat_cache too.


Overall the patch looks good to me.

--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] type cache for concat functions

[Pavel Stehule]

2017-06-18 0:50 GMT+02:00 Dmitry Dolgov <9erthali...@gmail.com>:

> > On 20 May 2017 at 10:03, Pavel Stehule  wrote:
> >
> > Now concat is 2x times slower than || operator. With cached FmgrInfo for
> > output function it will be only 5%.
>
> Looks nice and does what's expected (what else one may need from a Saturday
> evening). I just can mention that from what I see the previous version of
> `concat` is slower the more arguments are getting involved, so looks like
> it
> can be more than 2x.
>
> Also, it was a little bit confusing to see that the function `concat`
> behaves
> differently from operator `||` in terms of performance. When you're
> looking at
> the code it's becoming obvious, but I couldn't find any mention about that
> in
> the documentation.
>

There was few steps how to evaluate expressions more faster, but no step
how to evaluate faster polymorphic "any" functions. A detecting of used
argument type and looking to type cache are expensive. These functions was
designed for string processing in error messages, and the performance was
not too important. Now the concat function can be used often as replacement
of || operator when you do migration from Oracle due similar behave. And
the performance is more interesting now.

I think so can be interesting to allow some activity in analyzing/planning
time and prexec stage to user function developers. Some is possible via
planning time hooks, but for controlling few functions it is not user
friendly - and there are no space for data from this stages.

Regards

Pavel

Re: [HACKERS] type cache for concat functions

[Dmitry Dolgov]

> On 20 May 2017 at 10:03, Pavel Stehule  wrote:
>
> Now concat is 2x times slower than || operator. With cached FmgrInfo for
> output function it will be only 5%.

Looks nice and does what's expected (what else one may need from a Saturday
evening). I just can mention that from what I see the previous version of
`concat` is slower the more arguments are getting involved, so looks like it
can be more than 2x.

Also, it was a little bit confusing to see that the function `concat`
behaves
differently from operator `||` in terms of performance. When you're looking
at
the code it's becoming obvious, but I couldn't find any mention about that
in
the documentation.

[HACKERS] type cache for concat functions

[Pavel Stehule]

Hi

Now concat is 2x times slower than || operator. With cached FmgrInfo for
output function it will be only 5%.

Regards

Pavel
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index be399f4..d3f04f8 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -4718,6 +4718,28 @@ string_agg_finalfn(PG_FUNCTION_ARGS)
 		PG_RETURN_NULL();
 }
 
+static void
+rebuildConcatCache(FmgrInfo *typcache, FunctionCallInfo fcinfo, int argidx)
+{
+	int		i;
+
+	Assert(typcache != NULL);
+
+	for (i = argidx; i < PG_NARGS(); i++)
+	{
+		Oid			valtype;
+		Oid			typOutput;
+		bool		typIsVarlena;
+
+		valtype = get_fn_expr_argtype(fcinfo->flinfo, i);
+		if (!OidIsValid(valtype))
+			elog(ERROR, "could not determine data type of concat() input");
+
+		getTypeOutputInfo(valtype, , );
+		fmgr_info_cxt(typOutput, [i-argidx], fcinfo->flinfo->fn_mcxt);
+	}
+}
+
 /*
  * Implementation of both concat() and concat_ws().
  *
@@ -4733,6 +4755,7 @@ concat_internal(const char *sepstr, int argidx,
 	StringInfoData str;
 	bool		first_arg = true;
 	int			i;
+	FmgrInfo		*typcache;
 
 	/*
 	 * concat(VARIADIC some-array) is essentially equivalent to
@@ -4772,14 +4795,20 @@ concat_internal(const char *sepstr, int argidx,
 	/* Normal case without explicit VARIADIC marker */
 	initStringInfo();
 
+	typcache = (FmgrInfo *) fcinfo->flinfo->fn_extra;
+	if (typcache == NULL)
+	{
+		fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+		PG_NARGS() * sizeof(FmgrInfo));
+		typcache = (FmgrInfo *) fcinfo->flinfo->fn_extra;
+		rebuildConcatCache(typcache, fcinfo, argidx);
+	}
+
 	for (i = argidx; i < PG_NARGS(); i++)
 	{
 		if (!PG_ARGISNULL(i))
 		{
 			Datum		value = PG_GETARG_DATUM(i);
-			Oid			valtype;
-			Oid			typOutput;
-			bool		typIsVarlena;
 
 			/* add separator if appropriate */
 			if (first_arg)
@@ -4787,13 +4816,8 @@ concat_internal(const char *sepstr, int argidx,
 			else
 appendStringInfoString(, sepstr);
 
-			/* call the appropriate type output function, append the result */
-			valtype = get_fn_expr_argtype(fcinfo->flinfo, i);
-			if (!OidIsValid(valtype))
-elog(ERROR, "could not determine data type of concat() input");
-			getTypeOutputInfo(valtype, , );
 			appendStringInfoString(,
-   OidOutputFunctionCall(typOutput, value));
+	OutputFunctionCall([i-argidx], value));
 		}
 	}
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers