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

Robert,

Thanks for the review.

On Sat, Jan 22, 2011 at 11:28:48PM -0500, Robert Haas wrote:
> This certainly looks like a worthwhile thing to do, and it doesn't
> seem to need a lot of code, which is great.  But I confess I'm not
> confident I really understand what this patch is changing or why it's
> changing it.
>
> I think the problem is partly that the terminology used
> is not very consistent:
>
> +         if (!(exempt & COERCE_EXEMPT_NOCHANGE))
> +             tab->new_bits = true;
> +         if (!(exempt & COERCE_EXEMPT_NOERROR))
> +             tab->mayerror = true;
>
> These are the same two bits of status that are elsewhere called
> always-noop and never-error.  Or maybe not quite the same... but
> close.  A related problem is that I think only three of the four
> combinations are actually interesting: if there are new bits...  that
> is, if !COERCE_EXEMPT_NOCHANGE... i.e. not always-noop, then the state
> of the other bit is irrelevant.  I think maybe this ought to just be
> rephrased as an enum with three elements, describing the operation to
> be performed: rewrite, check, nothing.

I've fixed the GetCoerceExemptions header comments to follow the #define'd
names.  You are correct that only three of the four possibilities are distinct
for ALTER TABLE purposes.  I've adopted the enum in tablecmds.c.

> > As unintended fallout, it's no longer an error to add oids or a column with a
> > default value to a table whose rowtype is used in columns elsewhere.  This seems
> > best.  Defaults on the origin table do not even apply to new inserts into such a
> > column, and the rowtype does not gain an OID column via its table.
>
> I think this should be split into two patches (we can discuss both on
> this thread, no need to start any new ones), one that implements just
> the above improvement and another that accomplishes the main purpose
> of the patch.  Patches that do two or three or four things are quite a
> bit harder to understand than patches that just do one thing.

Sounds good; done.

> Also, you need to update the ALTER TABLE documentation.  The whole
> notes section needs to be gone over, but the following part in
> particular seems problematic, since we're proposing to break this:

Done.

I'm attaching four patches:

* at1.1-default-composite.patch
Remove the error when the user adds a column with a default value to a table
whose rowtype is used in a column elsewhere.
* at1.2-doc-set-data-type.patch
The documentation used "ALTER TYPE" when it meant "SET DATA TYPE", a subform of
"ALTER TABLE" or "ALTER FOREIGN TABLE".  Fixes just that.
* at1.3-tablecmds-enum.patch
Implements your suggestion of using an enum to represent the choice between
rewriting, scanning, and doing nothing.  No functional changes.  Most of this
patch is re-indentation, so I'm also attaching "at1.3w-tablecmds-enum.patch",
the same change under "git diff -w".  I reflowed the comment blocks that became
too wide, but I did not reflow the ones that now have more width available.
* at2v2-skip-nowork.patch
The remainder of the original patch, with the updates noted above.

nm