> I think this works ok in v9, but in v10 I've added a cast so that line becomes:
Maybe I missed something in my logic there, I didn't try to actually
break the function, but the new one with the additional comment is
definitely clearer about this.
+/*
+ * 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.
Because if it is valid for bits to not have any NULLs, we might do an
out of bounds read if natts == tupnatts, and bits is exactly allocated
to the proper size. MAXALIGN usually hides this situation, but the
possibility seems to be there.
+ /* 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).
(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"
