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

On Sun, Feb 13, 2011 at 02:53:07PM -0500, Robert Haas wrote:
> On Sun, Feb 13, 2011 at 6:50 AM, Noah Misch <noah(at)leadboat(dot)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.

Certainly. Understand that the first increment of this patch already made a
similar assumption about RelabelType, and CoerceToDomain is no less stable in
this respect. We're exposed to this level of responsibility already. We had
better get the code right, but what else is new?

> It seems that just about any
> refactoring of tablecmds.c might create such a possibility.

Uhhh. Example?

> 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.

Yes. Anyway, since we both clearly like the assertion, I've re-added it.

> 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?

Adding or dropping OIDs as the sole operation of the ALTER TABLE does result in
newrel != NULL and tab->newvals == NIL, but the code handles that fine. The
loop just re-forms the tuple from its original values/nulls lists.

> 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.

I wouldn't care for that change. Despite the name, NOT NULL and FOREIGN KEY
constraints wouldn't be represented. Casting to a domain to check the domain
constraints is a stretch of the term, and it will be fully obsolete when we
optimize xml -> text and such.

However, I've moved the setting of tab->verify to the points where we set
tab->new_notnull, rather than doing it later in that one place.

> 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.

Correct.

> 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.

I see.

Anyway, I've attached an updated version with these changes:
- Restored the transform expression walk to its own function
- Assert re-added
- tab->verify set alongside tab->new_notnull, not later

nm