On Fri, Feb 11, 2011 at 1:08 PM, Noah Misch <n...@leadboat.com> wrote: > It's a nice, clean patch. However, it achieves that by targeting a smaller > goal. Specifically, it omits: > > 1) Test cases and DEBUG messages sufficient to facilitate them
That was an intentional omission. > 2) Skip rewrite for T -> constraint-free domain over T > 3) Downgrade rewrite to scan for T -> constrained domain over T These were not. >> Upon examination, it appeared to me that trying to make the >> AlteredTableInfo contain an enum indicating whether we were doing a >> scan, a rewrite, or nothing was requiring more code change than I >> could really justify. > > Offhand, I count 12 changed lines to introduce the enum. There may be other > good reasons not to do it, but surely that wasn't it? I was unable to see what were getting out of that logic, and I couldn't readily verify that it was correct. >> @@ -766,9 +766,13 @@ ALTER TABLE <replaceable >> class="PARAMETER">name</replaceable> >> <para> >> Adding a column with a non-null default or changing the type of an >> existing column will require the entire table and indexes to be >> rewritten. >> - This might take a significant amount of time for a large table; and it >> will >> - temporarily require double the disk space. Adding or removing a system >> - <literal>oid</> column likewise requires rewriting the entire table. >> + As an exception, if the old type and new types are binary compatible and > > In the documentation for CREATE CAST, we define "binary compatible" using "Two > types that are binary coercible both ways are also referred to as binary > compatible." This feature does not require binary compatibility, merely a > one-way binary coercion. Generally, though, I like where you're going. My > version was accurate but obtuse. OK, I have touched up that language in the attached version. >> + the <literal>USING</> clause does not change the column contents, the > > This is probably fine, but note that "USING col || ''" does not change the > column contents, yet we won't optimize it. True; but I think we can let the fine user figure that out for themselves. :-) >> + table rewrite will be skipped, but the index rebuild is still required. > > This wrongly suggests that the rebuild mentioned earlier in the paragraph, > affecting all indexes, will take place. It's a more-limited rebuild. Ah, OK. I've changed the wording there. > This presents three options without any indication of how to choose between > them. A user wanting just a rewrite should look no further than VACUUM FULL. > We should document that ALTER TABLE can rewrite, both for general user > planning > and to dispel thoughts of doing both a VACUUM FULL and an ALTER TABLE in short > succession. However, it doesn't help to advertise ALTER TABLE or CLUSTER as > rewrite tools per se. I was wondering if we should try to be more clear about that, but I think it's a separate issue from this patch. >> --- a/src/backend/commands/tablecmds.c >> +++ b/src/backend/commands/tablecmds.c >> @@ -71,6 +71,7 @@ >> #include "storage/smgr.h" >> #include "utils/acl.h" >> #include "utils/builtins.h" >> +#include "utils/datum.h" > > This header is not needed in your version, is it? Looks like it isn't, thanks. > Moving this from parse_coerce.c to tablecmds.c does make sense. OK, glad you agree. It seemed sensible to me.. I think what I'd like to do if there are no major objections is commit this as-is, and then if you could perhaps provide a patch containing the set of changes that are necessary to recapture the cases I inadvertently failed to handle, namely: > 2) Skip rewrite for T -> constraint-free domain over T > 3) Downgrade rewrite to scan for T -> constrained domain over T Then I'll review that separately. I think this change stands on its own, and committing it in steps will be simple for me than doing the whole thing in one go. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
alter-type-2-rmh-v2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
