On Wed, Mar 26, 2025 at 6:45 PM David Rowley <dgrowle...@gmail.com> wrote: > On Wed, 26 Mar 2025 at 19:31, Richard Guo <guofengli...@gmail.com> wrote: > > On Wed, Mar 26, 2025 at 3:06 PM Tender Wang <tndrw...@gmail.com> wrote: > > > The comment about notnullattnums in struct RangeTblEntry says that: > > > * notnullattnums is zero-based set containing attnums of NOT NULL > > > * columns. > > > > > > But in get_relation_notnullatts(): > > > rte->notnullattnums = bms_add_member(rte->notnullattnums, > > > i + > > > 1); > > > > > > The notnullattnums seem to be 1-based.
> > This corresponds to the attribute numbers in Var nodes; you can > > consider zero as representing a whole-row Var. > Yeah, and a negative number is a system attribute, which the Bitmapset > can't represent... The zero-based comment is meant to inform the > reader that they don't need to offset by > FirstLowInvalidHeapAttributeNumber when indexing the Bitmapset. If > there's some confusion about that then maybe the wording could be > improved. I used "zero-based" because I wanted to state what it was > and that was the most brief terminology that I could think of to do > that. The only other way I thought about was to say that "it's not > offset by FirstLowInvalidHeapAttributeNumber", but I thought it was > better to say what it is rather than what it isn't. > > I'm open to suggestions if people are confused about this. I searched the current terminology used in code and can find "offset by FirstLowInvalidHeapAttributeNumber", but not "not offset by FirstLowInvalidHeapAttributeNumber". I think "zero-based" should be sufficient to indicate that this bitmapset is offset by zero, not by FirstLowInvalidHeapAttributeNumber. So I'm fine to go with "zero-based". I'm planning to push this patch soon, barring any objections. Thanks Richard
