|From:||Stephen Frost <sfrost(at)snowman(dot)net>|
|To:||Noah Misch <noah(at)leadboat(dot)com>|
|Cc:||Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org|
|Subject:||Re: ALTER TYPE 2: skip already-provable no-work rewrites|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
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
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?
|Next Message||Rémi Zara||2011-02-14 18:27:51||Re: pika failing since the per-column collation patch|
|Previous Message||Kevin Grittner||2011-02-14 18:10:49||Re: SSI bug?|