On Wed, Mar 26, 2025 at 6:45 PM David Rowley <dgrowleyml@gmail.com> wrote:
> On Wed, 26 Mar 2025 at 19:31, Richard Guo <guofenglinux@gmail.com> wrote:
> > On Wed, Mar 26, 2025 at 3:06 PM Tender Wang <tndrwang@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
