Hello
I rebased this patch to review it. Overall, I think this is a great
idea. Here are some comments on the patch, which I hope are not
anything major.
I'm not really sure that I agree with the way ATRewriteTable works now.
It seems that we scan the list of columns, and verify each one for nulls,
but abort midway if a column is generated. Surely we should check for
generated columns first, to avoid wasting time verifying earlier
columns in case a later column is generated?
That said, we do check for notnull_virtual_attrs to be NIL. It seems to
me that this implies that the checked columns are not generated. In
other words, the test for whether columns are generated is unnecessary
and confusing, and in which case you don't have to change anything, just
remove the "if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)"
inner block.
However, if we do this, then I think computing notnull_attrs is
pointless. So we should only change the order: do this new check first,
and if we find that any new not-null column is on a generated column,
then we compute both notnull_virtual_attrs and notnull_attrs. No?
The separation of labor between index_check_notnull() and
index_check_notnull_internal() seems a bit pointless. Why not have a
single routine that does both things? Though, to be honest, it does
look like the former should live in tablecmds.c instead of
execIndexing.c. (But if you do split things that way, then you need to
make index_check_notnull_internal extern).
The tests look a bit excessive. Why do we need an isolation test for
this? And it looks to me like the other test could be in
src/test/regress rather than be a TAP test. After all, that's what you
have the ereport(DEBUG1) there, isn't it?
"veritify" doesn't exist. The word is "verify".
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.
