Re: support virtual generated column not null constraint

On 2025-Mar-13, jian he wrote:

> hi.
> 
> new patch attached.
> 
> 0001 for virtual generated columns not null.
> minor change to fix the compiler warning.

I gave a look at 0001, and I propose some fixes.  0001 is just a typo in
an error message.  0002 is pgindent.  Then 0003 contain some more
interesting bits:

- in ExecRelCheckGenVirtualNotNull() it seems more natural to an
  AttrNumber rather than int, and use the InvalidAttrNumber value rather
  than -1 to indicate that all columns are ok.  Also, use
  foreach_current_index instead of manually counting the loop.

- ATRewriteTable can be simplified if we no longer add the virtual
  generated columns with not nulls to the notnulls_attrs list, but instead 
  we store the attnums in a separate list.  That way, the machinery
  around ExecRelCheckGenVirtualNotNull made less convoluted.

- in ExecConstraints, you removed a comment (which ended up in
  NotNullViolationErrorReport), which was OK as far as that went; but
  there was another comment a few lines below which said "See the
  comment above", which no longer made sense.  To fix it, I duplicated
  the original comment in the place where "See the..." comment was.

- renamed NotNullViolationErrorReport to ReportNotNullViolationError.
  Perhaps a better name is still possible -- something in line with
  ExecPartitionCheckEmitError?  (However, that function is exported,
  while the new one is not. So we probably don't need an "Exec" prefix
  for it.  I don't particularly like that name very much anyway.)

- Reduce scope of variables all over the place.

- Added a bunch more comments.


Other comments:

* The block in ATRewriteTable that creates a resultRelInfo for
  ExecRelCheckGenVirtualNotNull needs an explanation.

* I suspect the locations for the new functions were selected with
  the help of a dartboard.  ExecRelCheckGenVirtualNotNull() in
  particular looks like it could use a better location.  Maybe it's
  better right after ExecConstraints, and ReportNotNullViolationError
  (or whatever we name it) can go after it.

* I don't find the name all_virtual_nns particularly appropriate.  Maybe
  virtual_notnull_attrs?

* There are some funny rules around nulls on rowtypes.  I think allowing
  this tuple is wrong (and I left an XXX comment near where the 'argisrow'
  flag is set):

create type twoints as (a int, b int);
create table foo (a int, b int, c twoints generated always as (row(a,b)::twoints) not null);
insert into foo values (null, null);

I don't remember exactly what the rules are though so I may be wrong.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Having your biases confirmed independently is how scientific progress is
made, and hence made our great society what it is today" (Mary Gardiner)