Re: json/jsonb cleanup + FmgrInfo caching

On Thu, Jul 9, 2026 at 11:22 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> The hole in that argument is that fmgr_info_copy() does
>
> dstinfo->fn_extra = NULL;
>
> so that if the function-to-be-called caches anything in fn_extra,
> it will have to populate that cache for each argument. That could be
> quite expensive.

Agreed, in theory.

> It might still not justify complicating the setup
> code, but I don't think it's as clear-cut as you suggest. Perhaps
> some benchmarking is in order.

The thing is, the only thing we are calling here are cast functions
and type output functions. The only such functions I turned up that
use fn_extra are record_out(), array_out(), range_out(), and
multirange_out(), but I believe that the former two are irrelevant
because we do JSON-specific things for arrays and records. So any
possible gain would be limited to a VARIADIC call using a range or
multirange type. Something like this:

CREATE TABLE table_of_range_arrays (some_column int4range[]);
INSERT INTO table_of_range_arrays SELECT array[int4range(g, g+10),
int4range(g+20, g+30)] FROM generate_series(1, 20000000) g;

And then:

SELECT any_value(json_build_array(VARIADIC some_column)) FROM
table_of_range_arrays;

In my testing, this takes 3291 ms on
d007800f02a7b9fe3ca984a1b870405657d990ed and 2687 ms with the patches
applied to that commit, so it's close to 20% faster already. Somebody
might be able to squeeze out a little more by sharing the same
FmgrInfo across all argument positions, but it would be at the cost
not only of code complexity but also of cases that don't involve
VARIADIC or that use anything other than a range or multirange type. I
feel like if somebody thinks that's worthwhile they can propose it as
a followup patch, but I'm inclined to think that it isn't. It's a very
marginal case and I think the effort would be better spent on
optimizing other cases.

For example, composite_to_json() doesn't currently do any caching of
json_categorize_type results because it has nowhere to store the
cache. One of the callers is row_to_json(), which seems like it might
be able to use fn_extra ... but are we guaranteed that all inputs will
have the same rowtype? I'm not sure how that works. It can also get
called recursively from datum_to_json_internal() or
array_dim_to_json() while recursing down through substructure, but I'm
even less certain whether the types are guaranteed to be the same from
row to row in those cases, and there's also the question of where you
would actually store the cache, since they don't have a fn_extra that
they own. If type changes are not possible at these levels or are
cheap to detect, it might be possible to store a cache entry in the
top-level fn_extra that leaves places for substructure to store
fn_extra-like pointers so that we can do caching all the way down
through the structure. I'm not sure how feasible all that is, though.

One related thing that I noticed while researching this is that, as I
currently have it, json_extract_variadic_args() does this:

*categories = palloc_array(JsonTypeCategory, nargs);
*outflinfos = palloc0_array(FmgrInfo, nargs);
if (OidIsValid(jcache->flinfos[0].fn_oid))
{
for (int i = 0; i < nargs; ++i)
{
(*categories)[i] = jcache->categories[0];
fmgr_info_copy(&(*outflinfos)[i], &jcache->flinfos[0],
CurrentMemoryContext);
}
}
else
{
for (int i = 0; i < nargs; ++i)
(*categories)[i] = jcache->categories[0];
}

It could be argued that this is a waste: if we take the OidIsValid
branch, we'll overwrite every byte of outflinfos, so zeroing it was a
waste. If we take the !OidIsValid, zeroing it is also a waste unless
there's a bug, but I zeroed it because I figured if there was a bug,
using random memory as a FmgrInfo was going to be unpleasant for
debuggability. But there's an argument that even allocating the array
is a waste in this case: when datum_to_json(b)_internal is eventually
called with (*categories)[i] and (*outflinfos)[i], it should never
access the FmgrInfo if the category is such that the FmgrInfo is
invalid. So when !OidIsValid, I suppose we could just set *outflinfos
to NULL or some random pointer address and in theory that will never
get dereferenced so I guess it should be fine, and the benefit is we
save a palloc0_array per call. And there is a small but seemingly
measurable performance benefit from doing that. But I'm reluctant to
go that way unless we can find a less hacky way to avoid the palloc,
because I feel like static analyzers (and humans) will find it
confusing.

--
Robert Haas
EDB: http://www.enterprisedb.com