message: ThisPR {
  optional bool lgtm = 1 [(gogoproto.nullable) = false];
}

return &ThisPR{lgtm: true}

Reviewed 24 of 24 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @postamar)

Very nicely done! Let's get someone on SQL Schema to stamp this before merging.

Reviewed 24 of 24 files at r1, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @postamar and @wenyihu6)

-- commits line 2 at r1:
nit: we prefer to use the imperative mood for commit titles, like sql: add NotVisible to index descriptor

I think we typically use the present tense in the commit body (e.g. "This PR adds ..." rather than "This PR added ..."), but I don't see that as an official guideline in the doc linked above, so that's up to you if you want to change it.

-- commits line 4 at r1:
super nit: PR => commit

pkg/sql/catalog/catformat/index.go line 179 at r1 (raw file):

	if index.NotVisible {
		f.WriteString(" NOT VISIBLE")

Reminder to add tests for this with SHOW CREATE TABLE once not visible indexes can be added.

pkg/sql/opt/indexrec/hypothetical_index.go line 52 at r1 (raw file):

	// notVisible indicates if an index is not visible to the optimizer.
	notVisible bool

I don't think we need this field, we can just always return false in IsNotVisible below. I don't see a case where we'd make a recommendation to the user to create an invisible index.

You could add a TODO here to handle cases where an invisible index exists that should be made non-visible to improve the query plan.

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_index.go line 46 at r1 (raw file):

		IsInverted:     n.Inverted,
		IsConcurrently: n.Concurrently,
		IsNotVisible:   false, // TODO(wenyihu6): populate hidden property after CREATE

nit: hidden => IsNotVisible

LGTM