Hi
2017-08-24 19:24 GMT+02:00 Alexander Kuzmenkov <a.kuzmen...@postgrespro.ru>:
> 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,100000),
> ... [x5 total]) vs select generate_series(1,100000)::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, &typOutput, &typIsVarlena);
+ fmgr_info_cxt(typOutput, &typcache[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(&str);
+ 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(&str, 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, &typOutput, &typIsVarlena);
appendStringInfoString(&str,
- OidOutputFunctionCall(typOutput, value));
+ OutputFunctionCall(&typcache[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
