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
signature.asc
Description: Digital signature