public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: Chao Li <[email protected]>
Cc: jian he <[email protected]>
Cc: [email protected]
Subject: Re: Making jsonb_agg() faster
Date: Mon, 03 Nov 2025 13:40:30 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<CACJufxHuE-P0s1PxiRja26ZMjKUZiCAgWa1jedswo94SFzjXMg@mail.gmail.com>
	<[email protected]>
	<CACJufxGHLYtvOAB7uA59C9Lz+rDPta5Egiii-B5opsZbw6eDDQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

Chao Li <[email protected]> writes:
>> On Nov 3, 2025, at 04:56, Tom Lane <[email protected]> wrote:
>> PFA v3, rebased over 8a27d418f, no substantive changes whatever.

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

Thanks for looking at it!

> In elsewhere of the patch, you all use “JsonbInState ps = {0}” to do the initialization, only this place uses memset(). Can we keep consistent and use {0} here also. I see there is a “continue” and a “break” 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’m fine with keeping the current code.

I intentionally made the 0001 patch as mechanical as possible,
and in particular did not second-guess existing decisions about
whether to initialize a variable in the declaration or later.
So I'm not excited about doing it differently in this one place.
There are some other memsets in the same area that could also
be replaced by "= {0}" if we cared to, but that seems like
material for a separate cleanup patch.

> Instead of hard-coding 12, can we use "sizeof(TimeTzADT)” for better readability?

No, because that's not the same :-(.  sizeof() would produce 16,
on most machines anyway, thanks to alignment.  But 12 is the
type's size according to pg_type.

> You left two “ugly hack” comments. Maybe add a short description for why they are hack and what can be improved in the future.

It's the same ugly hack as before, I just formatted the code more
legibly ... I didn't stop to look at whether there's a better way to
do it.  Again, that seems like material for a different patch.

			regards, tom lane

view thread (25+ 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], [email protected], [email protected]
  Subject: Re: Making jsonb_agg() faster
  In-Reply-To: <[email protected]>

* 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