On Wed, 25 Feb 2026 at 23:54, Zsolt Parragi <[email protected]> wrote:
> +/*
> + * first_null_attr
> + * Inspect a NULL bitmask from a tuple and return the 0-based attnum of the
> + * first NULL attribute. Returns natts if no NULLs were found.
> + */
> +static inline int
> +first_null_attr(const bits8 *bits, int natts)
>
> The previous version of this function had an explanation that there
> has to be at least one NULL in bits - now it's not part of the
> description.
> Is it a requirement or not? I think it is currently true for all call
> sites, but the comment now allows all fields to be not null.
I mistakenly removed that comment thinking that it was no longer a
requirement. I've now put a more comprehensive comment there so that I
don't forget the reason for this again.
> + /* use attrmiss to set the missing values */
> + for (int attnum = startAttNum; attnum < lastAttNum; attnum++)
> {
> - slot->tts_values[missattnum] = attrmiss[missattnum].am_value;
> - slot->tts_isnull[missattnum] = !attrmiss[missattnum].am_present;
> + slot->tts_values[attnum] = attrmiss[attnum].am_value;
> + slot->tts_isnull[attnum] = !attrmiss[attnum].am_present;
> }
> }
> +
> + if (unlikely(lastAttNum > slot->tts_tupleDescriptor->natts))
> + elog(ERROR, "invalid attribute number %d", lastAttNum);
>
>
> Is it okay to do this error check at the end? If we hit that unlikely
> error condition, we already performed a write past the end of the
> arrays in the loop before (and also a read from attrmiss).
It's not. I've moved it to the start of that function.
> (in nocachegetattr)
>
> - off += att->attlen;
> + off = att_pointer_alignby(off, cattr->attalignby, cattr->attlen,
> + tp + off);
> + off += cattr->attlen;
>
> Shouldn't this be att_nominal_alignby, like above in the previous loop?
>
> There's one more typo in "This loops only differs"
Fixed both. I'll send an updated set of patches shortly.
Many thanks for reviewing.
David
