MIME-Version: 1.0
References: 
 <CA+TgmoaiothgQrw9OtgsMzBUCnqJ2jdaGTbS6o3fkjCd+LfzWw@mail.gmail.com>
 <CAD5tBc+LK2V6m-BDDD2KUCn3y9zLhO6FsY5VFTRS-+4L5tht3A@mail.gmail.com>
In-Reply-To: 
 <CAD5tBc+LK2V6m-BDDD2KUCn3y9zLhO6FsY5VFTRS-+4L5tht3A@mail.gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Thu, 9 Jul 2026 11:15:46 -0400
Message-ID: 
 <CA+TgmoZTQN8oXtB0JiwagG1cAkPOO+gA2+Hnf3w=O5t4ZwOJkA@mail.gmail.com>
Subject: Re: json/jsonb cleanup + FmgrInfo caching
To: Andrew Dunstan <andrew@dunslane.net>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BTgmoZTQN8oXtB0JiwagG1cAkPOO%2BgA2%2BHnf3w%3DO5t4ZwOJkA%40mail.gmail.com>
Precedence: bulk

On Wed, Jul 8, 2026 at 11:16=E2=80=AFAM 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 =3D=3D 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 opaq=
ue. 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 =3D 0; i < nargs; ++i)
>        fmgr_info_copy(&(*outflinfos)[i], &jcache->flinfos[0], CurrentMemo=
ryContext);
>
> is going to copy the same flinfo for every array element for every row, T=
he 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 wit=
h 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.

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