MIME-Version: 1.0
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>
 <rbskhk7scqbxqnaw4o6nh6na2ffcclg3cxn4d4cn5jfr2z7vv3@kadtz65meesb>
 <CAApHDvpDxDFatUskuOfuM7A3VESrx8U7MtYnU_HiB0QLAg94zg@mail.gmail.com>
 <pmik622adey6fnddivkt4uvkulvnc6rasmq3tcbrzeglx4hsn7@f3x6e2eph3w5>
 <rvlc7pb6zn4kydqovcqh72lf2qfcgs3qkj2seq7tcpvxyqwtqt@nrvv6lpehwwa> <CAApHDvo1i-ycAcWnK3L7ZASTuM8mW46kvRqMaUHD46HSuJmx7A@mail.gmail.com>
In-Reply-To: <CAApHDvo1i-ycAcWnK3L7ZASTuM8mW46kvRqMaUHD46HSuJmx7A@mail.gmail.com>
From: John Naylor <johncnaylorls@gmail.com>
Date: Sat, 31 Jan 2026 09:47:49 +0700
Message-ID: <CANWCAZYUQ-TXp5Tq6QYWktcK0ub9s73dYiprQpm341UcNOds=A@mail.gmail.com>
Subject: Re: More speedups for tuple deformation
To: David Rowley <dgrowleyml@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Chao Li <li.evan.chao@gmail.com>, 
	PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CANWCAZYUQ-TXp5Tq6QYWktcK0ub9s73dYiprQpm341UcNOds%3DA%40mail.gmail.com>
Precedence: bulk

On Tue, Jan 27, 2026 at 8:34=E2=80=AFPM David Rowley <dgrowleyml@gmail.com>=
 wrote:
> I've also included a slightly revised patch. I made a small change to
> the first_null_attr() to get rid of the masking of higher attnums and
> also now making use of __builtin_ctz to find the first NULL attnum in
> the byte. For compilers that don't support that, I've included a
> pg_rightmost_*zero*_pos table. I didn't want to use the pg_bitutils
> table for the rightmost *one* pos as it meant having to special-case
> what happens when using index 255, as that would return 0, and I want
> 8. I'll make the MSVC version use _BitScanForward() in the next patch.

I don't get why we'd need to special-case 255 in only one place.

+ /* Process all bytes up to just before the byte for the natts index */
+ for (bytenum =3D 0; bytenum < lastByte; bytenum++)
+ {
+   /* break if there's any NULL attrs (a 0 bit) */
+   if (bits[bytenum] !=3D 0xFF)
+   break;
+ }
+
+ res =3D bytenum << 3;
+
+#ifdef HAVE__BUILTIN_CTZ
+   res +=3D __builtin_ctz(~bits[bytenum]);
+#else
+   res +=3D pg_rightmost_zero_pos[bits[bytenum]];
+#endif

If bits[bytenum] is 255, then __builtin_ctz(0) is undefined. The top
of the function says

+ * We expect that 'bits' contains at least one 0 bit somewhere in the mask=
,
+ * not necessarily < natts.

...in which case it should be well defined everywhere. Am I missing
something? If we need to handle the 255 case, this should work:

pg_rightmost_one_pos32(~((uint32) bits[bytenum]))

--
John Naylor
Amazon Web Services