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’t done a load test to compare performance
differenceswith 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 = 0xFF & ((((uint8) 1) << (natts & 7)) - 1);
> + byte = (~bits[lastByte]) & mask;
> ```
>
> When I read the code, I got an impression bits[lastByte] might overflow when natts % 8 == 0, so I traced the code,
thenI realized that, this function is only called when a row has null values, so that, when reaching here, natts % 8 !=
0,otherwise it should return earlier within the for loop.
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.
How about if I write the comment as follows?
/*
* Create a mask with all bits beyond natts's bit set to off. The code
* below will generate a zero mask when natts & 7 == 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.
*/
> In TupleDescFinalize(), given firstNonCachedOffAttr = i + 1, firstNonCachedOffAttr will never be 0.
> But in nocachegetattr(), it checks firstNonCachedOffAttr >= 0:
> This is kinda inconsistent, and may potentially lead to some confusion to code readers.
I've changed things around so that firstNonCachedOffAttr == 0 means
there are no attributes with cached offsets. -1 becomes uninitialised.
I've added Asserts to check for firstNonCachedOffAttr >= 0 with a
comment directing anyone who's facing debugging one of those failing
to add the missing call to TupleDescFinalize().
Thanks for reviewing.
I've attached the v4 patch, which also fixes the LLVM compiler warning
that I introduced.
David
