Re: [HACKERS] Re: [BUGS] NOTICE:AbortTransaction and not in in-progress state

> I think several developers agreed that they were just wasting screen
> space.  The code is large enough without brace noise.  I have considered
> writing a script to remove the single-statement braces, but have not
> done it yet.
>
> If people would like to re-vote on this issue, I would be glad to hear
> about it.

Ok, I won't add them. I'm not taking them out if I see them though ;-).

> Sure, but braces don't help you either.  This is just as legal:
>
>     if (condition);
>     {
>         dosomething();
>     }

True enough, but I think less likely to happen.

> >  - I think the bit at line 1295-1309 might want to do all the work before
> >    the elog. Otherwise the elog leave the buffer cache polluted with buffers
> >    belonging to a mostly deleted index. Eg:
> >
> >    +     ReleaseRelationBuffers(indexRelation);
> >    +
> >          fname = relpath(indexRelation->rd_rel->relname.data);
> >      status = FileNameUnlink(fname);
> >
> >          index_close(indexRelation);
> >    +     RelationForgetRelation(indexRelation->rd_id);
> >
> >          if (status < 0)
> >                  elog(ERROR, "amdestroyr: unlink: %m");
>
> Yes, that is true, but I kept the order as used in the rest of the code,
> figuring the original coder knew better than I do.  IMHO, if we get that
> "amdestroyr" error, we have bigger problems than an invalid relation
> cache

Well, the code in heap.c calls smgrunlink() without checking the return code.
smgrunlink() calls mdunlink() which contains:

        if (FileNameUnlink(fname) < 0)
                return (SM_FAIL);

So heap_destroy does not even through an elog() at all if the FileNameUnlink()
fails. I think this is actually the right policy since if FileNameUnlink()
fails the only consequence is that a file is left on the disk (or maybe
the unlink failed because it was already gone). The system state (buffers etc)
and catalogs are consitant with the heap having been destroyed. So not a
problem from the database's perspective.

I suggest you change your patch to simply ignore the return code from
FileNameUnlink(). As in:

     ReleaseRelationBuffers(indexRelation);

     (void) FileNameUnlink(relpath(indexRelation->rd_rel->relname.data));

     index_close(indexRelation);
     RelationForgetRelation(indexRelation->rd_id);

In this way it will have the same behaviour as heap_destroy...

-dg

David Gould           dg@illustra.com            510.628.3783 or 510.305.9468
Informix Software                      300 Lakeside Drive   Oakland, CA 94612
 - A child of five could understand this!  Fetch me a child of five.