Обсуждение: Re: [BUGS] NOTICE:AbortTransaction and not in in-progress state

>
> > Please enter a FULL description of your problem:
> > ------------------------------------------------
> > Dropping table after aborting a transanction makes PosgresSQL unsable.
> >
> >
> > Please describe a way to repeat the problem.   Please try to provide a
> > concise reproducible example, if at all possible:
> > ----------------------------------------------------------------------
> > [srashd]t-ishii{67} psql -e test < b
> > 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);
> > QUERY: select * from test;
> >   i
> > ---
> > 100
> > (1 row)
> >
> > QUERY: rollback;
> > QUERY: drop table test;
> > NOTICE:AbortTransaction and not in in-progress state
> > NOTICE:AbortTransaction and not in in-progress state
> >
> > Note that if I do not make an index, it would be ok.
>
> Can someone comment on the cause of the above problem?  Is it a bug to
> add to the TODO list?  I have verified it still exists in the current
> sources.
>

The message I see in the logs for this is:

ERROR:  cannot write block 1 of iindex [test] blind
NOTICE:  AbortTransaction and not in in-progress state
NOTICE:  EndTransactionBlock and not inprogress/abort state

Vadim, sounds familiar.

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

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

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

  /* ----------------------------------------------------------------

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

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

> 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.

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.

>
>    Btw, I have been badly bit by:
>
>    if (condition);
>        dosomething();
>
>    which turned out to be very hard to see indeed.

Sure, but braces don't help you either.  This is just as legal:

    if (condition);
    {
        dosomething();
    }


>
>
>  - 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

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

> 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.

I've been trying to use them in my patches as well, maily for
consistency as its something I'd never do myself.  I'll gladly
stop though.

On Sat, 13 June 1998, at 19:59:12, David Gould wrote:

> 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.

Bruce Momjian wrote:
>
> Index: src/backend/catalog/heap.c
> ===================================================================
> ***************
> *** 1375,1380 ****
> --- 1370,1376 ----
>         rdesc->rd_tmpunlinked = TRUE;
>         heap_close(rdesc);
>         RemoveFromTempRelList(rdesc);
> +       RelationForgetRelation(rdesc->rd_id);

We need not in RelationForgetRelation() in heap_destroy().
Local relations are handled in other way...

Vadim