On Sun, Feb 13, 2011 at 6:50 AM, Noah Misch <noah@leadboat.com> wrote:
>> Yikes.
>> I didn't like that Assert much, but maybe we need it, because this is
>> scary.
>
> Can you elaborate on the fear-inducing aspect? I don't mind re-adding the
> Assert, but it seems that some positive understanding of the assumption's
> validity is in order.
Well, it's pretty obvious that if somehow we were to go down that code
path and not get back a value that was identical to the one we had
before, we'd have a corrupted table. It seems that just about any
refactoring of tablecmds.c might create such a possibility.
Assertions are a good idea when the reasons why something is true are
far-removed (in the code) from the places where they need to be true.
I'm somewhat uncomfortable also with the dependency of this code on
very subtle semantic differences between if (newrel) and if
(tab->newvals != NIL). I think this would blow up and die if for any
reason newrel were non-NULL but tab->newvals were NIL. Actually,
doesn't that happen if we're adding or dropping an OID column?
I think it might be cleaner to have tab->verify_constraints rather
than tab->verify. Then ATRewriteTables() could test if
(tab->verify_constraints || tab->new_notnull), and you wouldn't need
the bit that sets at->verify if at->new_notnull is already set. That
part makes the semantics of tab->verify depend on which point in
processing we're at, which I have found to be a recipe for confusion
in other parts of the source base (planner, I'm looking at you).
Correct me if I'm wrong here, but we could handle the
domains-without-contraints part here with about three additional lines
of code, right? It's only the domains-with-constraints part that
requires the rest of this. I'm half-tempted to put that part off to
9.2, in the hopes of getting a more substantial solution that can also
handle things like text -> xml which we don't have time to re-engineer
right now.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company