On Fri, Feb 11, 2011 at 02:49:27PM -0500, Robert Haas wrote:
> You might want to consider a second boolean in lieu of a three way
> enum. I'm not sure if that's cleaner but if it lets you write:
>
> if (blah)
> at->verify = true;
>
> instead of:
>
> if (blah)
> at->worklevel = Min(at->worklevel, WORK_VERIFY);
>
> ...then I think that might be cleaner.
Good point; the Max() calls did not make much sense all by themselves. The
point was to make sure nothing decreased the worklevel. Wrapping them in a
macro, say, ATRequireWork, probably would have helped.
That said, I've tried both constructions, and I marginally prefer the end result
with AlteredTableInfo.verify. I've inlined ATColumnChangeRequiresRewrite into
ATPrepAlterColumnType; it would need to either pass back two bools or take an
AlteredTableInfo arg to mutate, so this seemed cleaner. I've omitted the
assertion that my previous version added to ATRewriteTable; it was helpful for
other scan-only type changes, but it's excessive for domains alone. Otherwise,
the differences are cosmetic.
The large block in ATRewriteTable is now superfluous. For easier review, I
haven't removed it.
I missed a typo in the last patch: "T if we a rewrite is forced". Not changed
in this patch as I assume you'll want to commit it separately.
nm