public inbox for [email protected]  

help / color / mirror / Atom feed

From: Robert Haas <[email protected]>
To: Andrew Dunstan <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: json/jsonb cleanup + FmgrInfo caching
Date: Thu, 9 Jul 2026 11:15:46 -0400
Message-ID: <CA+TgmoZTQN8oXtB0JiwagG1cAkPOO+gA2+Hnf3w=O5t4ZwOJkA@mail.gmail.com> (raw)
In-Reply-To: <CAD5tBc+LK2V6m-BDDD2KUCn3y9zLhO6FsY5VFTRS-+4L5tht3A@mail.gmail.com>
References: <CA+TgmoaiothgQrw9OtgsMzBUCnqJ2jdaGTbS6o3fkjCd+LfzWw@mail.gmail.com>
	<CAD5tBc+LK2V6m-BDDD2KUCn3y9zLhO6FsY5VFTRS-+4L5tht3A@mail.gmail.com>

On Wed, Jul 8, 2026 at 11:16 AM Andrew Dunstan <[email protected]> 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

view thread (4+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: json/jsonb cleanup + FmgrInfo caching
  In-Reply-To: <CA+TgmoZTQN8oXtB0JiwagG1cAkPOO+gA2+Hnf3w=O5t4ZwOJkA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox