Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: Making jsonb_agg() faster
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <163563.1762117009@sss.pgh.pa.us>
Date: Mon, 3 Nov 2025 21:38:46 +0800
Cc: jian he <jian.universality@gmail.com>,
 pgsql-hackers@lists.postgresql.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <AAFBCC62-1295-4084-8CBC-2026C9075A3C@gmail.com>
References: <1060917.1753202222@sss.pgh.pa.us>
 <CACJufxHuE-P0s1PxiRja26ZMjKUZiCAgWa1jedswo94SFzjXMg@mail.gmail.com>
 <617666.1755277022@sss.pgh.pa.us>
 <CACJufxGHLYtvOAB7uA59C9Lz+rDPta5Egiii-B5opsZbw6eDDQ@mail.gmail.com>
 <654628.1755297629@sss.pgh.pa.us> <2129790.1755889894@sss.pgh.pa.us>
 <68A1061C-81A5-46AA-AFA9-48294908254C@gmail.com>
 <2613D418-67E0-4DD8-BDA6-AB1BB04DB1A2@gmail.com>
 <2466173.1756498180@sss.pgh.pa.us> <163563.1762117009@sss.pgh.pa.us>
To: Tom Lane <tgl@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/AAFBCC62-1295-4084-8CBC-2026C9075A3C%40gmail.com>
Precedence: bulk


> On Nov 3, 2025, at 04:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>=20
> PFA v3, rebased over 8a27d418f, no substantive changes whatever.
>=20
> regards, tom lane
>=20

I am on vacation this week, I only find a hour in the evening and did an =
eyeball review without actually tracing and testing this patch set. =
Overall, the changes are solid and look good to me. Only a few small =
comments on 0001:

1 -  jsonpath_exec.c
```
@@ -2874,8 +2874,7 @@ executeKeyValueMethod(JsonPathExecContext *cxt, =
JsonPathItem *jsp,
 	{
 		JsonBaseObjectInfo baseObject;
 		JsonbValue	obj;
-		JsonbParseState *ps;
-		JsonbValue *keyval;
+		JsonbInState ps;
 		Jsonb	   *jsonb;
=20
 		if (tok !=3D WJB_KEY)
@@ -2889,7 +2888,8 @@ executeKeyValueMethod(JsonPathExecContext *cxt, =
JsonPathItem *jsp,
 		tok =3D JsonbIteratorNext(&it, &val, true);
 		Assert(tok =3D=3D WJB_VALUE);
=20
-		ps =3D NULL;
+		memset(&ps, 0, sizeof(ps));
+
```

In elsewhere of the patch, you all use =E2=80=9CJsonbInState ps =3D =
{0}=E2=80=9D to do the initialization, only this place uses memset(). =
Can we keep consistent and use {0} here also. I see there is a =
=E2=80=9Ccontinue=E2=80=9D and a =E2=80=9Cbreak=E2=80=9D before the =
memset(), maybe you want to avoid unnecessary initialization. I guess =
that is a tiny saving, but if you think that saving is worthwhile, I=E2=80=
=99m fine with keeping the current code.

2 - jsonb_util.c
```
+				case TIMETZOID:
+					/* pass-by-reference */
+					oldcontext =3D =
MemoryContextSwitchTo(outcontext);
+					v->val.datetime.value =3D =
datumCopy(v->val.datetime.value,
+										=
			  false, 12);
```

Instead of hard-coding 12, can we use "sizeof(TimeTzADT)=E2=80=9D for =
better readability?

3 - jsonb_plperl.c
```
+	{
+		/* XXX Ugly hack */
+		jsonb_state->result =3D palloc(sizeof(JsonbValue));
+		memcpy(jsonb_state->result, &out, sizeof(JsonbValue));
+	}
```
And
```
+	else
+	{
+		/* XXX Ugly hack */
+		jsonb_state->result =3D out;
+	}
```

You left two =E2=80=9Cugly hack=E2=80=9D comments. Maybe add a short =
description for why they are hack and what can be improved in the =
future.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/