Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81.1.4\))
Subject: Re: More speedups for tuple deformation
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <0663AA4F-74FB-48A5-B77B-3C150445FF2B@gmail.com>
Date: Tue, 20 Jan 2026 14:05:58 +0800
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <AC3F5585-E7B1-4F01-835C-19A0D9F17C83@gmail.com>
References: <CAApHDvpoFjaj3+w_jD5uPnGazaw41A71tVJokLDJg2zfcigpMQ@mail.gmail.com>
 <CAApHDvrF6DG7=xD8JGo2HoQKN0LRFNF0ysVt6cKSNPiqbdQOSA@mail.gmail.com>
 <CAApHDvoh3Q413szd-zsUTCpQPWNdpUYvx-fvsB8DP8zOja+ckg@mail.gmail.com>
 <9A17C43D-7A28-4885-8974-555A40C9523E@gmail.com>
 <CAApHDvqhbJU_-yF3Hbf4VhX33qXtpeYv3MsvMPDMcDwGGLr9ZQ@mail.gmail.com>
 <0663AA4F-74FB-48A5-B77B-3C150445FF2B@gmail.com>
To: David Rowley <dgrowleyml@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/AC3F5585-E7B1-4F01-835C-19A0D9F17C83%40gmail.com>
Precedence: bulk


> On Jan 20, 2026, at 12:32, Chao Li <li.evan.chao@gmail.com> wrote:
>=20
>=20
>=20
>> On Jan 20, 2026, at 08:11, David Rowley <dgrowleyml@gmail.com> wrote:
>>=20
>> On Mon, 19 Jan 2026 at 18:48, Chao Li <li.evan.chao@gmail.com> wrote:
>>> I reviewed the patch and traced some basic workflows. But I =
haven=E2=80=99t done a load test to compare performance differences with =
and without this patch, I will do that if I get some bandwidth later. =
Here comes some review comments:
>>>=20
>>> 1 - tupmacs.h
>>> ```
>>> +       /* Create a mask with all bits beyond natts's bit set to off =
*/
>>> +       mask =3D 0xFF & ((((uint8) 1) << (natts & 7)) - 1);
>>> +       byte =3D (~bits[lastByte]) & mask;
>>> ```
>>>=20
>>> When I read the code, I got an impression bits[lastByte] might =
overflow when natts % 8 =3D=3D 0, so I traced the code, then I realized =
that, this function is only called when a row has null values, so that, =
when reaching here, natts % 8 !=3D 0, otherwise it should return earlier =
within the for loop.
>>=20
>> It certainly is possible to get to that part of the code when natts =
is
>> a multiple of 8 and the tuple contains NULLs after that (we may not =
be
>> deforming the entire tuple). The code you quoted that's setting =
"mask"
>> in that case will produce a zero mask, resulting in not finding any
>> NULLs. I don't quite see any risk of overflowing any of the types
>> here.  If natts is 16 then effectively the code does 0xFF & ((1 << 0)
>> - 1); so no overflow. Just left shift by 0 bits and bitwise AND with
>> zero, resulting in the mask becoming zero.
>>=20
>> How about if I write the comment as follows?
>>=20
>> /*
>> * Create a mask with all bits beyond natts's bit set to off.  The =
code
>> * below will generate a zero mask when natts & 7 =3D=3D 0.  When that =
happens
>> * all bytes that need to be checked were done so in the loop above.  =
The
>> * code below will create an empty mask and end up returning natts.  =
This
>> * has been done to avoid having to write a special case to check if =
we've
>> * covered all bytes already.
>> */
>>=20
>=20
> I=E2=80=99m sorry I didn=E2=80=99t express myself clearly, maybe I =
should have used =E2=80=9COOB=E2=80=9D rather than =E2=80=9Coverflow". =
My real concern is about out-of-boundary read of bits[lastByte] when =
natts&7=3D=3D0.
>=20
> Say, natts is 16, then bits is 2 bytes long; lastByte =3D 16>>3 =3D 2, =
so bits[2] is a OOB read.
>=20
> If first_null_attr() is only called when hasnulls=3D=3Dtrue, then it =
will never hit the OOB point, because it will return early from the =
=E2=80=9Cfor=E2=80=9D loop. In the current patch, which is true, so the =
OOB should never happen.
>=20
> However, I don=E2=80=99t see any comment mentions something like =
=E2=80=9Cfirst_null_attr() should only be called when hasnulls is true. =
If in future one calls first_null_attr() in a situation where hasnulls =
=3D=3D false, then the OOB will be triggered.
>=20
> The comment you added explains that even if OOB happens, no matter =
what value is hold by bits[lastByte], because mask is 0, the final =
result is still correct, which is true, but OOB is still a concern. If =
the bits array happens to end exactly at the edge of a memory page, the =
OOB read bits[lastByte] may trigger a segment fault; and valgrind may =
detect the OOB and complain about it.
>=20
> So, my original comment was that, we should at least add something to =
the header comment to mention =E2=80=9Cfirst_null_attr() should only be =
called when hasnulls is true. If we can add an Assert to ensure hasnulls =
is true, that would be even better.
>=20
> But if we want first_null_attr() to be safe no matter hasnulls is true =
or false, I think we should avoid the OOB.
>=20

I also noticed one thing that, with running an arbitrary SQL statement, =
first_null_attr() might be called with natts=3D0, so maybe it can have a =
fast path to return 0 directly if natts=3D=3D0.

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