Re: Avoid index rebuilds for no-rewrite ALTER TABLE ALTER TYPE

On Thu, Jun 30, 2011 at 11:55 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Wed, Jun 29, 2011 at 09:42:06AM -0400, Robert Haas wrote:
>> On Tue, Jun 28, 2011 at 3:40 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>
>> > Here's the call stack in question:
>> >
>> > ? ? ? ?RelationBuildLocalRelation
>> > ? ? ? ?heap_create
>> > ? ? ? ?index_create
>> > ? ? ? ?DefineIndex
>> > ? ? ? ?ATExecAddIndex
>> >
>> > Looking at it again, it wouldn't bring the end of the world to add a relfilenode
>> > argument to each. None of those have more than four callers.
>>
>> Yeah.  Those functions take an awful lot of arguments, which suggests
>> that some refactoring might be in order, but I still think it's
>> cleaner to add another argument than to change the state around
>> after-the-fact.
>>
>> > ATExecAddIndex()
>> > would then call RelationPreserveStorage() before calling DefineIndex(), which
>> > would in turn put things in a correct state from the start. ?Does that seem
>> > appropriate? ?Offhand, I do like it better than what I had.
>>
>> I wish we could avoid the whole death-and-resurrection thing
>> altogether, but off-hand I'm not seeing a real clean way to do that.
>> At the very least we had better comment it to death.
>
> I couldn't think of a massive amount to say about that, but see what you think
> of this level of commentary.
>
> Looking at this again turned up a live bug in the previous version: if the old
> index file were created in the current transaction, we would wrongly remove its
> delete-at-abort entry as well as its delete-at-commit entry.  This leaked the
> disk file.  Fixed by adding an argument to RelationPreserveStorage() indicating
> which kind to remove.  Test case:
>
>        BEGIN;
>        CREATE TABLE t AS SELECT * FROM generate_series(1,100000) t(n);
>        CREATE INDEX ti ON t(n);
>        SELECT pg_relation_filepath('ti');
>        ALTER TABLE t ALTER n TYPE int;
>        ROLLBACK;
>        CHECKPOINT;
>        -- file named above should be gone
>
> I also updated the ATPostAlterTypeCleanup() variable names per discussion and
> moved IndexStmt.oldNode to a more-natural position in the structure.

On first blush, that looks a whole lot cleaner. I'll try to find some
time for a more detailed review soon.

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