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: <CAApHDvoh3Q413szd-zsUTCpQPWNdpUYvx-fvsB8DP8zOja+ckg@mail.gmail.com>
Date: Mon, 19 Jan 2026 13:47:47 +0800
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <9A17C43D-7A28-4885-8974-555A40C9523E@gmail.com>
References: <CAApHDvpoFjaj3+w_jD5uPnGazaw41A71tVJokLDJg2zfcigpMQ@mail.gmail.com>
 <CAApHDvrF6DG7=xD8JGo2HoQKN0LRFNF0ysVt6cKSNPiqbdQOSA@mail.gmail.com>
 <CAApHDvoh3Q413szd-zsUTCpQPWNdpUYvx-fvsB8DP8zOja+ckg@mail.gmail.com>
To: David Rowley <dgrowleyml@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/9A17C43D-7A28-4885-8974-555A40C9523E%40gmail.com>
Precedence: bulk


> On Jan 19, 2026, at 06:13, David Rowley <dgrowleyml@gmail.com> wrote:
>=20
> On Fri, 2 Jan 2026 at 18:58, David Rowley <dgrowleyml@gmail.com> =
wrote:
>> Please find attached an updated set of patches. A rebase was needed,
>> plus 0003 had a problem with an Assert not handling the bitmap being =
a
>> NULL pointer.
>=20
> Another rebase and updates to some newly created missing calls to
> TupleDescFinalize().
>=20
> I've also attached another round of benchmarks after dipping into some
> Azure machines to cover my lack of any Intel benchmark results. I
> think these are somewhat noisy as I opted for low core-count instances
> which will have L3 shared with workloads running for other people.
> This is most evident in Xeon_E5-2673 with gcc where the patched run
> was nearly twice as fast as unpatched for test 2 on 20 extra columns.
> If you look at the raw results from that, you can see the times are
> quite unstable between the 3 runs of each test, which makes me believe
> that the machine was busy with other work when that test ran on
> master. The AMD3990x and M2 machines are all sitting next to me and
> were otherwise idle, so they should be much more stable.
>=20
> Quite a few machines have a small regression for the 0 extra column
> tests. There is a small amount of extra work being done in the
> deforming function to check if the attnum < the first attribute
> without an attcacheoff. This mostly only affects the tests that don't
> do any deforming with a cached attcacheoff, e.g due to NULLs or
> varlena types. The only way I've thought about to possibly reduce that
> is to invent a new TupleTableSlotOps and pick the one that applies
> when creating the TupleTableSlot. This doesn't appeal to me very much
> as it requires modifying many callsites. But I do wonder if we should
> try to come up with something here as technically we could use this to
> eliminate alignment padding out of some MinimalTuples in some cases
> where these were not directly derived from pre-formed HeapTuples. That
> could allow a more compact tuple representation for sorting and
> hashing, allowing us to do more with less memory in some cases.
>=20
> The benchmark results also indicated that there wasn't much advantage
> to the 0002+0003 patches, so I've removed those from the set. That
> reduces some complexity around the benchmarks. I did still keep the
> OPTIMIZE_BYVAL loop as separate results. It's not quite clear what's
> best there as machines seem to vary on which they prefer.
>=20
> Benchmark results attached in the bz2 file both in spreadsheet form
> and the raw results pg_dumped.
>=20
> David
> =
<v3-0001-Precalculate-CompactAttribute-s-attcacheoff.patch><deform_results=
2.tar.bz2>

Hi David,

I reviewed the patch and traced some basic workflows. But I haven=E2=80=99=
t 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:

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;
```

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.

So, to avoid future reader=E2=80=99s same confusion, can we add a brief =
comment to explain that no overflow should happen here.

2 - After this patch, nocachegetattr() and nocache_index_getattr() =
strictly rely on tupleDesc->firstNonCachedOffAttr to work:
```
	if (tupleDesc->firstNonCachedOffAttr >=3D 0)
	{
		startAttr =3D Min(tupleDesc->firstNonCachedOffAttr - 1, =
firstnullattr);
		off =3D TupleDescCompactAttr(tupleDesc, =
startAttr)->attcacheoff;
	}
	else
	{
		startAttr =3D 0;
		off =3D 0;
	}
```

And tupleDesc->firstNonCachedOffAttr is only set by TupleDescFinalize(). =
So, assuming some code misses to call TupleDescFinalize(), looking at =
how TupleDesc is created, for example CreateTemplateTupleDesc():
```
	desc =3D (TupleDesc) palloc(offsetof(struct TupleDescData, =
compact_attrs) +
							  natts * =
sizeof(CompactAttribute) +
							  natts * =
sizeof(FormData_pg_attribute));

	/*
	 * Initialize other fields of the tupdesc.
	 */
	desc->natts =3D natts;
	desc->constr =3D NULL;
	desc->tdtypeid =3D RECORDOID;
	desc->tdtypmod =3D -1;
	desc->tdrefcount =3D -1;		/* assume not =
reference-counted */

	return desc;
```

It=E2=80=99s palloc and not palloc0, so desc->firstNonCachedOffAttr will =
initially hold a random value. As long as TupleDescFinalize() is missed, =
then that=E2=80=99s a bug.

=46rom this perspective, I think we can set firstNonCachedOffAttr to -2 =
when in CreateTemplateTupleDesc() as well as other functions that create =
a TupleDesc. Then in nocachegetattr() and nocache_index_getattr(), we =
can Assert(desc->firstNonCachedOffAttr > -2).

3
```
+		firstNonCachedOffAttr =3D i + 1;
```

In TupleDescFinalize(), given firstNonCachedOffAttr =3D i + 1, =
firstNonCachedOffAttr will never be 0.

But in nocachegetattr(), it checks firstNonCachedOffAttr >=3D 0:
```
	if (tupleDesc->firstNonCachedOffAttr >=3D 0)
	{
		startAttr =3D Min(tupleDesc->firstNonCachedOffAttr - 1, =
firstnullattr);
		off =3D TupleDescCompactAttr(tupleDesc, =
startAttr)->attcacheoff;
	}
```

This is kinda inconsistent, and may potentially lead to some confusion =
to code readers.

=46rom the meaning of the variable name =E2=80=9CfirstNonCachedOffAttr=E2=80=
=9D, when there is no cached attribute, firstNonCachedOffAttr feels =
better to be 0 rather than-1. =46rom this perspective, =
TupleDescFinalize() can initialize desc->firstNonCachedOffAttr to 0. And =
for my comment 2, we can use -1 instead of -2, so that -1 indicates =
TupleDescFinalize() is not called, 0 means no cached attribute, >0 means =
some cached attributes.=20

4
```
+		 * possibily could cache the first attlen =3D=3D -2 =
attr.  Worthwhile?
```

Typo: possibily -> possibly

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