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@leadboat.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.
Thanks,
nm
