json/jsonb cleanup + FmgrInfo caching

On Thu, Jul 2, 2026 at 12:25 PM Robert Haas <robertmhaas@gmail.com> wrote:

Hi,

While working on my "sandboxing untrusted code" project, I found
myself investigating how the JSON and JSONB code calls type output and
cast functions. I feel like this needs some cleanup in order to avoid
blocking that project, and it turns out that there are also
significant opportunities to improve performance, so here are some
patches. One caveat: one of these patches causes a small backward
compatibility break, because the current behavior is wrong. See below
for full details.

First, a quick performance demonstration:

CREATE TABLE hstores AS SELECT hstore('k', g::text) x FROM
generate_series(1,4000000) g;
SELECT any_value(json_build_object('a', x)) FROM hstores;

Unpatched, median-of-three runs was 398.571 ms. Patched, 193.714 ms.
That's a 2.05x speedup, which is the highest I observed in my test
queries. What I found is that this technique shows the largest gains
with user-defined types like hstore, smaller gains with built-in types
like int, and the smallest gains of all with container types such as a
user-defined record type. Generally, jsonb benefited more than json.
The aggregates - json_agg and jsonb_agg - showed very little benefit
or even small regressions, but I'm pretty confident that the
regressions are simply noise that goes away with a sufficiently large
number of sufficiently-careful test runs. Everything else gets faster,
often by 50%+. Leaving aside the aggregates which are expected to show
little or no benefit, here's one of the less-sympathetic cases:

CREATE TABLE ints AS SELECT x FROM generate_series(1,4000000) x;
SELECT any_value(to_json(x)) FROM ints;

The speedup is smaller here because it's json rather than jsonb and
because it's int rather than hstore, but it's still 117.461 ms
unpatched vs. 89.895 patched, a 30% speedup.

OK, now let's go through the patches:

0001 refactors the json_categorize_type() function to initialize an
FmgrInfo instead of returning a base function OID. All of the built-in
JSON aggregates are updated to cache this FmgrInfo across calls, but
it really doesn't save much. In the process of working on this
refactoring it came to light that the current behavior of
json_check_mutability() is incorrect: it erroneously treats record,
anyarray, and anycompatiblearray as immutable when in fact they should
be treated as stable. This refactoring preserves that incorrect
behavior.

0002 fixes the bug discovered during the development of 0001 by
removing the special case. AFAICT, the anyarray and anycompatiblearray
cases are unreachable, but the record case is reachable, and the
included test case shows how this could hypothetically matter. It
seems unlikely we'll inconvenience any significant number of users by
changing this, but in theory somebody's upgrade could fail.

0003 moves some code around to avoid problems with circular header
dependencies, creating new files jsontypes.c/h.

0004 refactors the SQL-level JSON constructors -- JSON_OBJECT,
JSON_ARRAY, and JSON_SCALAR -- to make use of the new type caching
infrastructure.

0005 refactors the SQL-callable functions similarly. This means
to_json(b), json(b)_build_object, and json(b)_build_array.

Looks pretty good.

The old `add_json()`/`add_jsonb()` helpers (removed in 0004/0005) checked
`val_type == InvalidOid` and raised a clean
`ERRCODE_INVALID_PARAMETER_VALUE` / "could not determine input data type"
error. Now I think we'd get something like "cache lookup failed for type
0", which is rather more opaque. Not sure if that matters.

For VARIADIC arrays,

for (int i = 0; i < nargs; ++i)
fmgr_info_copy(&(*outflinfos)[i], &jcache->flinfos[0],
CurrentMemoryContext);

is going to copy the same flinfo for every array element for every row, The
comment says that it's done that way to avoid complicating the code, and
that seems reasonable. I don't know how often these functions are used with
explicit VARIADIC array arguments, so it might be a niche case.

cheers

andrew

On Wed, Jul 8, 2026 at 11:16 AM Andrew Dunstan <andrew@dunslane.net> wrote:

Looks pretty good.

Thanks for looking.

The old `add_json()`/`add_jsonb()` helpers (removed in 0004/0005) checked `val_type == InvalidOid` and raised a clean `ERRCODE_INVALID_PARAMETER_VALUE` / "could not determine input data type" error. Now I think we'd get something like "cache lookup failed for type 0", which is rather more opaque. Not sure if that matters.

I definitely don't want to show "cache lookup failed" errors, but I
don't think that's reachable. If I'm wrong and it is, we should add a
guard. Do you have a test case, by any chance?

For VARIADIC arrays,

for (int i = 0; i < nargs; ++i)
fmgr_info_copy(&(*outflinfos)[i], &jcache->flinfos[0], CurrentMemoryContext);

is going to copy the same flinfo for every array element for every row, The comment says that it's done that way to avoid complicating the code, and that seems reasonable. I don't know how often these functions are used with explicit VARIADIC array arguments, so it might be a niche case.

Right. I think it is a niche case, but I also don't think this is
really costing us anything. As far as I can tell from my benchmarking
to date, fmgr_info() is kind of expensive, but fmgr_info_copy() is
pretty cheap. So, we could arrange to share a single flinfo for every
argument of the VARIADIC list, but the savings are pretty limited
because we wouldn't save fmgr_info() calls, only fmgr_info_copy()
calls. And on the downside json{,b}_build_{array,object}_worker would
all need to support two modes, one where there is an fcinfo per Datum
and a second where there is one fcinfo for all Datums. There's a
code-complexity argument against that, but it would also have some
cost in CPU cycles, because every call would have to branch into one
path or the other. We'd be slowing down both variadic and non-variadic
calls slightly by adding those branches, in the hopes that saving
fmgr_info_copy() calls in the non-variadic case would be valuable
enough to make it worthwhile. Somebody can certainly try that, but my
guess is that there won't be any performance benefit worth caring
about and the code will be uglier.

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

Robert Haas <robertmhaas@gmail.com> writes:

Right. I think it is a niche case, but I also don't think this is
really costing us anything. As far as I can tell from my benchmarking
to date, fmgr_info() is kind of expensive, but fmgr_info_copy() is
pretty cheap. So, we could arrange to share a single flinfo for every
argument of the VARIADIC list, but the savings are pretty limited
because we wouldn't save fmgr_info() calls, only fmgr_info_copy()
calls.

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. 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.

regards, tom lane

On Thu, Jul 9, 2026 at 11:22 AM Tom Lane <tgl@sss.pgh.pa.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