On Sun, Feb 13, 2011 at 6:50 AM, Noah Misch <n...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
