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
