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

On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> This patch removes ALTER TYPE rewrites in cases we can already prove valid.  I
> add a function GetCoerceExemptions() that walks an Expr according to rules
> discussed in the design thread, simplified slightly pending additions in the
> next patch.  See the comment at that function for a refresher.  I use it to
> populate two new bools to AlteredTableInfo, "new_bits" and "mayerror".
> "new_bits" is a superset of "new_changedoids", so I subsume that.  I change
> ATRewriteTable to act on those and support the notion of evaluating the
> transformation expressions when we're not rewriting the table.
>
> This helps on conversions like varchar(X)->text, xml->text, and conversions
> between domains and their base types.

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.

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

On a related note, it is very helpful to avoid introducing unrelated
changes into a patch. Comment updates should reflect changes you
made, rather than editorialization on what's already there. There is
some value to the latter, but it makes it harder to understand what
the patch is doing.

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:

# The fact that ALTER TYPE requires rewriting the whole table is
sometimes an advantage, because the rewriting process
# eliminates any dead space in the table. For example, to reclaim the
space occupied by a dropped column immediately, the
# fastest way is:
#
# ALTER TABLE table ALTER COLUMN anycol TYPE anytype;

I'm not sure what we can recommend here instead.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company