Noah,
I'm even less familiar w/ this code than Robert, but figured I'd give a
shot at reviewing this anyway. I definitely like avoiding table
rewrites if I can get away with it. :)
First question is- why do you use #ifdef USE_ASSERT_CHECKING ..?
assert_enabled exists and will work the way you expect regardless, no?
Strikes me as unlikely that the checks would be a real performance
problem..
In ATColumnChangeSetWorkLevel(), I'm really not a huge fan of using a
passed-in argument to move through a list with.. I'd suggest using a
local variable that is set from what's passed in. I do see that's
inheirited, but still, you've pretty much redefined that entire
function anyway..
Also, I feel like that while(!tab->rewrite) really deserves more
explanation of what's happening than the function-level comment above
gives it. I'd prefer to see a comment above the while() explaining
that we're moving through a list to see if there's any level at which
expr is something complicated or is referring to a domain which has
constraints on it (presuming that I've followed what's going on
correctly, that is..). It also seems like it'd make more sense to me to
be a while() controlled by (IsA(expr, Var) && ((Var *) expr)->varattno
== varattno), since that's really the normal "stopping point".
These are all more stylistic issues than anything else. Last, but not
least, I do worry about how this may impact contrib modules, external
projects, or user-added data types, such as PostGIS, hstore, and ip4r.
Could we/should we limit this to only PG data types that we 'know' are
binary compatible? Is there any way or reason such external modules
could be fouled up by this?
Thanks!
Stephen
