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

On Thu, Jan 27, 2011 at 2:48 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Wed, Jan 26, 2011 at 07:31:40AM -0500, Robert Haas wrote:
>> I'd also suggest that this big if-block you changed to a case
>> statement could just as well stay as an if-block.  There are only
>> three cases, and we want to avoid rearranging things more than
>> necessary.  It complicates both review and back-patching to no good
>> end.
>
> Okay.  I've also left out the large reindent in ATRewriteTable for now.  Easy to
> re-add it later if desired.
>
>> I think you should collect up what's left of ALTER TABLE 0 and the
>> stuff on this thread, rebase it, and submit it as a single patch on
>> this thread that applies directly against the master branch.  We may
>> decide to split it back up again in some other way, but I think the
>> current division isn't actually buying us much.
>
> Done as attached.  This preserves compatibility with our current handling of
> composite type dependencies.  The rest you've seen before.

OK, I finally got back to this. Sorry for the delay. I still feel
that it's possible to accomplish the same goal with less complexity.
See what you think of the attached.

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. What I ended up doing is replacing the 'bool
new_changedoids' member with a 'bool rewrite' member. I'm pretty sure
this is safe. The only reason we ever tested the new_changeoids
member was to see whether we needed to do a rewrite; it wasn't used
for anything else. The new member gets set either when we need to do
a rewrite, or when a column type changes in a fashion that requires a
rewrite. It's pretty easy to verify that this doesn't result in any
behavior change, except for one corner case: currently, if you add or
remove OIDs to/from a table and in the same ALTER TABLE command add a
constraint that involves creating an index, such as a primary key, the
new primary key index will get built, thrown away, and rebuilt a
second time. With this change, we just build it once, which seems
like an improvement, even though the case is vanishingly unlikely to
ever happen in practice.

I also have to say that after playing with a little bit, I find the
new debugging messages you added to be quite invaluable for
understanding what's really going on. The old output told you
basically nothing useful, even if you cranked it all the way up to
DEBUG3. This is much better.

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