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)
