Re: [HACKERS] Re: [BUGS] NOTICE:AbortTransaction and not in in-progress state - Mailing list pgsql-hackers

Bruce Momjian wrote:
> > Bruce Momjian wrote:
> > >
> > > > > QUERY: drop table test;
> > > > > WARN:Relation test Does Not Exist!
> > > > > QUERY: create table test (i int4);
> > > > > QUERY: create index iindex on test using btree(i);
> > > > > QUERY: begin;
> > > > > QUERY: insert into test values (100);
> >
> > There will be dirty heap & index buffers in the pool after insertion ...
> >
> > > > > QUERY: select * from test;
> > > > >   i
> > > > > ---
> > > > > 100
> > > > > (1 row)
> > > > >
> > > > > QUERY: rollback;
> >
> > They are still marked as dirty...
> >
> > > > > QUERY: drop table test;
> >
> > heap_destroy_with_catalog () calls ReleaseRelationBuffers:
> >
> >  *      this function unmarks all the dirty pages of a relation
> >  *      in the buffer pool so that at the end of transaction
> >  *      these pages will not be flushed.
> >
> > before unlinking relation, BUT index_destroy() unlinks index
> > and DOESN'T call this func ...
> >
> > > > > NOTICE:AbortTransaction and not in in-progress state
> > > > > NOTICE:AbortTransaction and not in in-progress state
> >
> > COMMIT (of drop table) tries to flush all dirty buffers
> > from pool but there is no index file any more ...
> >
> > > ERROR:  cannot write block 1 of iindex [test] blind
> >
> > smgrblindwrt () fails.
> >
> > > NOTICE:  AbortTransaction and not in in-progress state
> > > NOTICE:  EndTransactionBlock and not inprogress/abort state
> >
> > ...transaction state is IN_COMMIT...
> >
> > Seems that ReleaseRelationBuffers() should be called by
> > index_destroy() ... Note that heap_destroy() also calls
> >
> >     /* ok - flush the relation from the relcache */
> >     RelationForgetRelation(rid);
> >
> > at the end...
> >
> > Vadim
> >
>
> OK, the following patch fixes the problem.  Vadim, I added both function
> calls to index_destroy and made heap_destroy consistent with
> heap_destroy_with_catalog.
>
> ---------------------------------------------------------------------------
>
> Index: src/backend/catalog/heap.c
> ===================================================================
> RCS file: /usr/local/cvsroot/pgsql/src/backend/catalog/heap.c,v
> retrieving revision 1.48
> diff -c -r1.48 heap.c
> *** heap.c    1998/04/27 04:04:47    1.48
> --- heap.c    1998/06/13 20:13:18
> ***************
> *** 12,18 ****
>    * INTERFACE ROUTINES
>    *        heap_create()            - Create an uncataloged heap relation
>    *        heap_create_with_catalog() - Create a cataloged relation
> !  *        heap_destroy_with_catalog()            - Removes named relation from catalogs
>    *
>    * NOTES
>    *      this code taken from access/heap/create.c, which contains
> --- 12,18 ----
>    * INTERFACE ROUTINES
>    *        heap_create()            - Create an uncataloged heap relation
>    *        heap_create_with_catalog() - Create a cataloged relation
> !  *        heap_destroy_with_catalog()    - Removes named relation from catalogs
>    *
>    * NOTES
>    *      this code taken from access/heap/create.c, which contains
> ***************
> *** 1290,1307 ****
>        * ----------------
>        */
>       if (rdesc->rd_rel->relhasindex)
> -     {
>           RelationRemoveIndexes(rdesc);
> -     }
>
>       /* ----------------
>        *    remove rules if necessary
>        * ----------------
>        */
>       if (rdesc->rd_rules != NULL)
> -     {
>           RelationRemoveRules(rid);
> -     }
>
>       /* triggers */
>       if (rdesc->rd_rel->reltriggers > 0)
> --- 1290,1303 ----
> ***************
> *** 1347,1355 ****
>        * ----------------
>        */
>       if (!(rdesc->rd_istemp) || !(rdesc->rd_tmpunlinked))
> -     {
>           smgrunlink(DEFAULT_SMGR, rdesc);
> !     }
>       rdesc->rd_tmpunlinked = TRUE;
>
>       RelationUnsetLockForWrite(rdesc);
> --- 1343,1350 ----
>        * ----------------
>        */
>       if (!(rdesc->rd_istemp) || !(rdesc->rd_tmpunlinked))
>           smgrunlink(DEFAULT_SMGR, rdesc);
> !
>       rdesc->rd_tmpunlinked = TRUE;
>
>       RelationUnsetLockForWrite(rdesc);
> ***************
> *** 1375,1380 ****
> --- 1370,1376 ----
>       rdesc->rd_tmpunlinked = TRUE;
>       heap_close(rdesc);
>       RemoveFromTempRelList(rdesc);
> +     RelationForgetRelation(rdesc->rd_id);
>   }
>
>
> Index: src/backend/catalog/index.c
> ===================================================================
> RCS file: /usr/local/cvsroot/pgsql/src/backend/catalog/index.c,v
> retrieving revision 1.41
> diff -c -r1.41 index.c
> *** index.c    1998/05/09 23:42:59    1.41
> --- index.c    1998/06/13 20:13:27
> ***************
> *** 1270,1276 ****
>       while (tuple = heap_getnext(scan, 0, (Buffer *) NULL),
>              HeapTupleIsValid(tuple))
>       {
> -
>           heap_delete(catalogRelation, &tuple->t_ctid);
>       }
>       heap_endscan(scan);
> --- 1270,1275 ----
> ***************
> *** 1296,1307 ****
>       heap_close(catalogRelation);
>
>       /*
> !      * physically remove the file
>        */
>       if (FileNameUnlink(relpath(indexRelation->rd_rel->relname.data)) < 0)
>           elog(ERROR, "amdestroyr: unlink: %m");
>
>       index_close(indexRelation);
>   }
>
>   /* ----------------------------------------------------------------
> --- 1295,1309 ----
>       heap_close(catalogRelation);
>
>       /*
> !      * flush cache and physically remove the file
>        */
> +     ReleaseRelationBuffers(indexRelation);
> +
>       if (FileNameUnlink(relpath(indexRelation->rd_rel->relname.data)) < 0)
>           elog(ERROR, "amdestroyr: unlink: %m");
>
>       index_close(indexRelation);
> +     RelationForgetRelation(indexRelation->rd_id);
>   }
>
>   /* ----------------------------------------------------------------


Two comments:

 - I notice you getting rid of { } pairs eg:

   if (condition)
   {
       dosomething();
   }

   becomes

   if (condition)
       dosomething();

   Is this policy? I prefer to have the braces almost always as I find
   it easier to read, and less error prone if someone adds a statement or an
   else clause, so in most of my patches, I would tend to put braces in.
   If you are busy taking them out simaltaniously, this could get silly.

   Btw, I have been badly bit by:

   if (condition);
       dosomething();

   which turned out to be very hard to see indeed.


 - 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");

-dg

David Gould            dg@illustra.com           510.628.3783 or 510.305.9468
Informix Software  (No, really)         300 Lakeside Drive  Oakland, CA 94612
"Don't worry about people stealing your ideas.  If your ideas are any
 good, you'll have to ram them down people's throats." -- Howard Aiken