On Sun, Jan 9, 2011 at 5:01 PM, Noah Misch <n...@leadboat.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
