Re: postmaster errors with index on temp table?

Attached is a patch for temp tables that implements Tom's requests.

> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> >> But, when a temp rel is dropped it seems that heap_drop_with_catalog also
> >> drops the indexes, so the error occurs when index_drop is called (at least
> >> I think this is the case). Certainly commenting out the last two lines
> >> *seems* to work.
>
> > Not sure why I introduced that bug in 1.18.  Your suggestion was 100%
> > correct.  I have applied the following patch.
>
> Actually, I don't think this is the true explanation.  index_drop'ing
> the temp indexes may not be *necessary*, but it shouldn't *hurt* either.
>
> Now that I think about it, the reason for the failure is probably that
> there's no CommandCounterIncrement in this loop.  Therefore, when we
> index_drop an index, the resulting system-table updates are *not seen*
> by heap_drop_with_catalog when it comes time to drop the owning table,
> and so it tries to drop the index again.
>
> Your solution of not doing index_drop at all is sufficient for the
> table-and-index case, but I bet it is not sufficient for more complex
> cases like RI checks between temp relations.  I'd recommend doing
> CommandCounterIncrement after each temp item is dropped, instead.
>
> There is another potential bug here: remove_all_temp_relations() is
> failing to consider the possibility that removing one list entry may
> cause other ones (eg, indexes) to go away.  It's holding onto a "next"
> pointer to a list entry that may not be there by the time control comes
> back from heap_drop_with_catalog.  This is probably OK for tables and
> indexes because the indexes will always be added after their table and
> thus appear earlier in the list, but again it seems like trouble just
> waiting to happen.  I would recommend logic along the lines of
>
>     while (temp_rels != NIL)
>     {
>         get first entry in temp_rels,
>         and either heap_drop or index_drop it as appropriate;
>         CommandCounterIncrement();
>     }
>
> This relies on the drop to come back and remove the temp_rels entry
> (and, possibly, other entries); so it's not an infinite loop even
> though it looks like one.
>
>             regards, tom lane
>


--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
? config.log
? config.cache
? config.status
? GNUmakefile
? src/Makefile.custom
? src/GNUmakefile
? src/Makefile.global
? src/log
? src/crtags
? src/backend/postgres
? src/backend/catalog/global.bki
? src/backend/catalog/global.description
? src/backend/catalog/template1.bki
? src/backend/catalog/template1.description
? src/backend/port/Makefile
? src/bin/x
? src/bin/initdb/initdb
? src/bin/initlocation/initlocation
? src/bin/ipcclean/ipcclean
? src/bin/pg_config/pg_config
? src/bin/pg_ctl/pg_ctl
? src/bin/pg_dump/pg_dump
? src/bin/pg_dump/pg_restore
? src/bin/pg_dump/pg_dumpall
? src/bin/pg_id/pg_id
? src/bin/pg_passwd/pg_passwd
? src/bin/pgaccess/pgaccess
? src/bin/pgtclsh/Makefile.tkdefs
? src/bin/pgtclsh/Makefile.tcldefs
? src/bin/pgtclsh/pgtclsh
? src/bin/pgtclsh/pgtksh
? src/bin/psql/psql
? src/bin/scripts/createlang
? src/include/config.h
? src/include/stamp-h
? src/interfaces/ecpg/lib/libecpg.so.3.2.0
? src/interfaces/ecpg/preproc/ecpg
? src/interfaces/libpgeasy/libpgeasy.so.2.1
? src/interfaces/libpgtcl/libpgtcl.so.2.1
? src/interfaces/libpq/libpq.so.2.1
? src/interfaces/perl5/blib
? src/interfaces/perl5/Makefile
? src/interfaces/perl5/pm_to_blib
? src/interfaces/perl5/Pg.c
? src/interfaces/perl5/Pg.bs
? src/pl/plpgsql/src/libplpgsql.so.1.0
? src/pl/tcl/Makefile.tcldefs
? src/test/regress/pg_regress
? src/test/regress/regress.out
? src/test/regress/results
? src/test/regress/regression.diffs
? src/test/regress/expected/copy.out
? src/test/regress/expected/create_function_1.out
? src/test/regress/expected/create_function_2.out
? src/test/regress/expected/misc.out
? src/test/regress/expected/constraints.out
? src/test/regress/sql/copy.sql
? src/test/regress/sql/misc.sql
? src/test/regress/sql/create_function_1.sql
? src/test/regress/sql/create_function_2.sql
? src/test/regress/sql/constraints.sql
Index: src/backend/access/transam/xact.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/access/transam/xact.c,v
retrieving revision 1.71
diff -c -r1.71 xact.c
*** src/backend/access/transam/xact.c    2000/09/27 10:41:55    1.71
--- src/backend/access/transam/xact.c    2000/10/11 21:22:55
***************
*** 1119,1125 ****
      AtEOXact_portals();
      RecordTransactionAbort();
      RelationPurgeLocalRelation(false);
!     invalidate_temp_relations();
      AtEOXact_SPI();
      AtEOXact_nbtree();
      AtAbort_Cache();
--- 1119,1125 ----
      AtEOXact_portals();
      RecordTransactionAbort();
      RelationPurgeLocalRelation(false);
!     remove_temp_rel_in_myxid();
      AtEOXact_SPI();
      AtEOXact_nbtree();
      AtAbort_Cache();
Index: src/backend/catalog/heap.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/catalog/heap.c,v
retrieving revision 1.147
diff -c -r1.147 heap.c
*** src/backend/catalog/heap.c    2000/10/05 19:48:21    1.147
--- src/backend/catalog/heap.c    2000/10/11 21:22:59
***************
*** 131,141 ****
      MaxCommandIdAttributeNumber, 0, -1, -1, '\001', 'p', '\0', 'i', '\0', '\0'
  };

! /*
     We decide to call this attribute "tableoid" rather than say
  "classoid" on the basis that in the future there may be more than one
  table of a particular class/type. In any case table is still the word
! used in SQL.
  */
  static FormData_pg_attribute a7 = {
      0xffffffff, {"tableoid"}, OIDOID, 0, sizeof(Oid),
--- 131,141 ----
      MaxCommandIdAttributeNumber, 0, -1, -1, '\001', 'p', '\0', 'i', '\0', '\0'
  };

! /*
     We decide to call this attribute "tableoid" rather than say
  "classoid" on the basis that in the future there may be more than one
  table of a particular class/type. In any case table is still the word
! used in SQL.
  */
  static FormData_pg_attribute a7 = {
      0xffffffff, {"tableoid"}, OIDOID, 0, sizeof(Oid),
***************
*** 1489,1495 ****
      RelationForgetRelation(rid);

      if (istemp)
!         remove_temp_relation(rid);

      if (has_toasttable)
      {
--- 1489,1495 ----
      RelationForgetRelation(rid);

      if (istemp)
!         remove_temp_rel_by_relid(rid);

      if (has_toasttable)
      {
Index: src/backend/catalog/index.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/catalog/index.c,v
retrieving revision 1.127
diff -c -r1.127 index.c
*** src/backend/catalog/index.c    2000/10/05 19:48:21    1.127
--- src/backend/catalog/index.c    2000/10/11 21:23:00
***************
*** 1145,1151 ****
      RelationForgetRelation(indexId);

      /* does something only if it is a temp index */
!     remove_temp_relation(indexId);
  }

  /* ----------------------------------------------------------------
--- 1145,1151 ----
      RelationForgetRelation(indexId);

      /* does something only if it is a temp index */
!     remove_temp_rel_by_relid(indexId);
  }

  /* ----------------------------------------------------------------
***************
*** 1374,1380 ****
      if (!LockClassinfoForUpdate(relid, &tuple, &buffer, confirmCommitted))
          elog(ERROR, "IndexesAreActive couldn't lock %u", relid);
      if (((Form_pg_class) GETSTRUCT(&tuple))->relkind != RELKIND_RELATION &&
!         ((Form_pg_class) GETSTRUCT(&tuple))->relkind != RELKIND_TOASTVALUE)
          elog(ERROR, "relation %u isn't an indexable relation", relid);
      isactive = ((Form_pg_class) GETSTRUCT(&tuple))->relhasindex;
      ReleaseBuffer(buffer);
--- 1374,1380 ----
      if (!LockClassinfoForUpdate(relid, &tuple, &buffer, confirmCommitted))
          elog(ERROR, "IndexesAreActive couldn't lock %u", relid);
      if (((Form_pg_class) GETSTRUCT(&tuple))->relkind != RELKIND_RELATION &&
!         ((Form_pg_class) GETSTRUCT(&tuple))->relkind != RELKIND_TOASTVALUE)
          elog(ERROR, "relation %u isn't an indexable relation", relid);
      isactive = ((Form_pg_class) GETSTRUCT(&tuple))->relhasindex;
      ReleaseBuffer(buffer);
Index: src/backend/utils/cache/temprel.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/utils/cache/temprel.c,v
retrieving revision 1.27
diff -c -r1.27 temprel.c
*** src/backend/utils/cache/temprel.c    2000/07/12 18:04:45    1.27
--- src/backend/utils/cache/temprel.c    2000/10/11 21:23:01
***************
*** 91,125 ****
  void
  remove_all_temp_relations(void)
  {
-     List       *l,
-                *next;
-
-     if (temp_rels == NIL)
-         return;
-
      AbortOutOfAnyTransaction();
      StartTransactionCommand();

!     l = temp_rels;
!     while (l != NIL)
      {
!         TempTable  *temp_rel = (TempTable *) lfirst(l);

!         next = lnext(l);        /* do this first, l is deallocated */
!
!         /* Indexes are dropped during heap drop */
          if (temp_rel->relkind != RELKIND_INDEX)
-         {
-             char        relname[NAMEDATALEN];
-
-             /* safe from deallocation */
-             strcpy(relname, temp_rel->user_relname);
              heap_drop_with_catalog(relname, allowSystemTableMods);
!         }
!
!         l = next;
      }
-     temp_rels = NIL;
      CommitTransactionCommand();
  }

--- 91,112 ----
  void
  remove_all_temp_relations(void)
  {
      AbortOutOfAnyTransaction();
      StartTransactionCommand();

!     while (temp_rels != NIL)
      {
!         char        relname[NAMEDATALEN];
!         TempTable  *temp_rel = (TempTable *) lfirst(temp_rels);

!         /* safe from deallocation */
!         strcpy(relname, temp_rel->user_relname);
          if (temp_rel->relkind != RELKIND_INDEX)
              heap_drop_with_catalog(relname, allowSystemTableMods);
!         else
!             index_drop(temp_rel->relid);
!         CommandCounterIncrement();
      }
      CommitTransactionCommand();
  }

***************
*** 129,135 ****
   * we don't have the relname for indexes, so we just pass the oid
   */
  void
! remove_temp_relation(Oid relid)
  {
      MemoryContext oldcxt;
      List       *l,
--- 116,122 ----
   * we don't have the relname for indexes, so we just pass the oid
   */
  void
! remove_temp_rel_by_relid(Oid relid)
  {
      MemoryContext oldcxt;
      List       *l,
***************
*** 179,185 ****
   * We just have to delete the map entry.
   */
  void
! invalidate_temp_relations(void)
  {
      MemoryContext oldcxt;
      List       *l,
--- 166,172 ----
   * We just have to delete the map entry.
   */
  void
! remove_temp_rel_in_myxid(void)
  {
      MemoryContext oldcxt;
      List       *l,
Index: src/include/utils/temprel.h
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/include/utils/temprel.h,v
retrieving revision 1.10
diff -c -r1.10 temprel.h
*** src/include/utils/temprel.h    2000/06/20 06:41:11    1.10
--- src/include/utils/temprel.h    2000/10/11 21:23:14
***************
*** 18,29 ****

  extern void create_temp_relation(const char *relname,
                                   HeapTuple pg_class_tuple);
! extern void remove_temp_relation(Oid relid);
  extern bool rename_temp_relation(const char *oldname,
                                   const char *newname);

  extern void remove_all_temp_relations(void);
! extern void invalidate_temp_relations(void);

  extern char *get_temp_rel_by_username(const char *user_relname);
  extern char *get_temp_rel_by_physicalname(const char *relname);
--- 18,29 ----

  extern void create_temp_relation(const char *relname,
                                   HeapTuple pg_class_tuple);
! extern void remove_temp_rel_by_relid(Oid relid);
  extern bool rename_temp_relation(const char *oldname,
                                   const char *newname);

  extern void remove_all_temp_relations(void);
! extern void remove_temp_rel_in_myxid(void);

  extern char *get_temp_rel_by_username(const char *user_relname);
  extern char *get_temp_rel_by_physicalname(const char *relname);