Re: ALTER TYPE 2: skip already-provable no-work rewrites

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
Date: 2011-02-14 18:12:21
Message-ID: 20110214181221.GM4116@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
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?