Dealing with CLUSTER failures - Mailing list pgsql-patches

Christopher Kings-Lynne wrote:
> > I don't think that's a bug.  You may not intend ever to cluster on that
> > index again, and if you try it will tell you about the problem.
>
> Except it breaks the 'cluster everything' case:
>
> test=# cluster;
> ERROR:  cannot cluster when index access method does not handle null values
> HINT:  You may be able to work around this by marking column "a" NOT NULL.

I looked over this item, originally posted as:

    http://archives.postgresql.org/pgsql-hackers/2005-03/msg01055.php

It seems that if you use an index method that doesn't support NULLs, you
can use ALTER to set the column as NOT NULL, then CLUSTER, and then set
it to allow NULLs, and when you CLUSTER all tables, the cluster errors
out on that table.

I thought about removing the cluster bit when you do the alter or
something like that, but it seems too confusing and error-prone to be
sure we get every case.

I think the main problem is that while cluster is a performance-only
feature, we error out if we can't cluster one table, basically treating
it as though it needs transaction semantics.  It doesn't.

This patch throws an ERROR of you cluster a specific index that can't be
clustered, but issues only a WARNING if you are clustering all tables.
This allows it to report the failed cluster but keep going.  I also
modified the code to print the index name in case of failure, because
without that the user doesn't know the failing index name in a
database-wide cluster failure.

Here is an example:

    test=> cluster  test_gist_idx on  test;
    ERROR:  cannot cluster on index "test_gist_idx" because access method
    does not handle null values
    HINT:  You may be able to work around this by marking column "a" NOT NULL.
    test=> cluster;
    WARNING:  cannot cluster on index "test_gist_idx" because access method
    does not handle null values
    HINT:  You may be able to work around this by marking column "a" NOT NULL.
    CLUSTER

You can see the ERROR for a specific index, and WARNING for full
database cluster.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/backend/commands/cluster.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/cluster.c,v
retrieving revision 1.137
diff -c -c -r1.137 cluster.c
*** src/backend/commands/cluster.c    6 May 2005 17:24:53 -0000    1.137
--- src/backend/commands/cluster.c    8 May 2005 02:40:50 -0000
***************
*** 292,298 ****
      OldHeap = heap_open(rvtc->tableOid, AccessExclusiveLock);

      /* Check index is valid to cluster on */
!     check_index_is_clusterable(OldHeap, rvtc->indexOid);

      /* rebuild_relation does all the dirty work */
      rebuild_relation(OldHeap, rvtc->indexOid);
--- 292,303 ----
      OldHeap = heap_open(rvtc->tableOid, AccessExclusiveLock);

      /* Check index is valid to cluster on */
!     if (!check_index_is_clusterable(OldHeap, rvtc->indexOid,
!             recheck ? WARNING : ERROR))
!     {
!         heap_close(OldHeap, NoLock);
!         return;
!     }

      /* rebuild_relation does all the dirty work */
      rebuild_relation(OldHeap, rvtc->indexOid);
***************
*** 308,315 ****
   * redundant, but it seems best to grab it anyway to ensure the index
   * definition can't change under us.
   */
! void
! check_index_is_clusterable(Relation OldHeap, Oid indexOid)
  {
      Relation    OldIndex;

--- 313,320 ----
   * redundant, but it seems best to grab it anyway to ensure the index
   * definition can't change under us.
   */
! bool
! check_index_is_clusterable(Relation OldHeap, Oid indexOid, int elevel)
  {
      Relation    OldIndex;

***************
*** 321,331 ****
       */
      if (OldIndex->rd_index == NULL ||
          OldIndex->rd_index->indrelid != RelationGetRelid(OldHeap))
!         ereport(ERROR,
                  (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                   errmsg("\"%s\" is not an index for table \"%s\"",
                          RelationGetRelationName(OldIndex),
                          RelationGetRelationName(OldHeap))));

      /*
       * Disallow clustering on incomplete indexes (those that might not
--- 326,340 ----
       */
      if (OldIndex->rd_index == NULL ||
          OldIndex->rd_index->indrelid != RelationGetRelid(OldHeap))
!     {
!         ereport(elevel,
                  (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                   errmsg("\"%s\" is not an index for table \"%s\"",
                          RelationGetRelationName(OldIndex),
                          RelationGetRelationName(OldHeap))));
+         index_close(OldIndex);
+         return false;
+     }

      /*
       * Disallow clustering on incomplete indexes (those that might not
***************
*** 334,342 ****
       * that seems expensive and tedious.
       */
      if (!heap_attisnull(OldIndex->rd_indextuple, Anum_pg_index_indpred))
!         ereport(ERROR,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!                  errmsg("cannot cluster on partial index")));
      if (!OldIndex->rd_am->amindexnulls)
      {
          AttrNumber    colno;
--- 343,357 ----
       * that seems expensive and tedious.
       */
      if (!heap_attisnull(OldIndex->rd_indextuple, Anum_pg_index_indpred))
!     {
!         ereport(elevel,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!                  errmsg("cannot cluster on partial index \"%s\"",
!                         RelationGetRelationName(OldIndex))));
!         index_close(OldIndex);
!         return false;
!     }
!
      if (!OldIndex->rd_am->amindexnulls)
      {
          AttrNumber    colno;
***************
*** 352,362 ****
          {
              /* ordinary user attribute */
              if (!OldHeap->rd_att->attrs[colno - 1]->attnotnull)
!                 ereport(ERROR,
                          (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!                          errmsg("cannot cluster when index access method does not handle null values"),
                           errhint("You may be able to work around this by marking column \"%s\" NOT NULL.",
                    NameStr(OldHeap->rd_att->attrs[colno - 1]->attname))));
          }
          else if (colno < 0)
          {
--- 367,383 ----
          {
              /* ordinary user attribute */
              if (!OldHeap->rd_att->attrs[colno - 1]->attnotnull)
!             {
!                 ereport(elevel,
                          (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!                          errmsg("cannot cluster on index \"%s\" because access method\n"
!                                 "does not handle null values",
!                               RelationGetRelationName(OldIndex)),
                           errhint("You may be able to work around this by marking column \"%s\" NOT NULL.",
                    NameStr(OldHeap->rd_att->attrs[colno - 1]->attname))));
+                 index_close(OldIndex);
+                 return false;
+             }
          }
          else if (colno < 0)
          {
***************
*** 365,373 ****
          else
          {
              /* index expression, lose... */
!             ereport(ERROR,
                      (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!                      errmsg("cannot cluster on expressional index when index access method does not handle null
values")));
          }
      }

--- 386,398 ----
          else
          {
              /* index expression, lose... */
!             ereport(elevel,
                      (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!                      errmsg("cannot cluster on expressional index \"%s\" because its index access\n"
!                             "method does not handle null values",
!                         RelationGetRelationName(OldIndex))));
!             index_close(OldIndex);
!             return false;
          }
      }

***************
*** 379,400 ****
       * might work for other system relations, but I ain't gonna risk it.
       */
      if (IsSystemRelation(OldHeap))
!         ereport(ERROR,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                   errmsg("\"%s\" is a system catalog",
                          RelationGetRelationName(OldHeap))));

      /*
       * Don't allow cluster on temp tables of other backends ... their
       * local buffer manager is not going to cope.
       */
      if (isOtherTempNamespace(RelationGetNamespace(OldHeap)))
!         ereport(ERROR,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
             errmsg("cannot cluster temporary tables of other sessions")));

      /* Drop relcache refcnt on OldIndex, but keep lock */
      index_close(OldIndex);
  }

  /*
--- 404,434 ----
       * might work for other system relations, but I ain't gonna risk it.
       */
      if (IsSystemRelation(OldHeap))
!     {
!         ereport(elevel,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                   errmsg("\"%s\" is a system catalog",
                          RelationGetRelationName(OldHeap))));
+         index_close(OldIndex);
+         return false;
+     }

      /*
       * Don't allow cluster on temp tables of other backends ... their
       * local buffer manager is not going to cope.
       */
      if (isOtherTempNamespace(RelationGetNamespace(OldHeap)))
!     {
!         ereport(elevel,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
             errmsg("cannot cluster temporary tables of other sessions")));
+         index_close(OldIndex);
+         return false;
+     }

      /* Drop relcache refcnt on OldIndex, but keep lock */
      index_close(OldIndex);
+     return true;
  }

  /*
Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.156
diff -c -c -r1.156 tablecmds.c
*** src/backend/commands/tablecmds.c    6 May 2005 17:24:53 -0000    1.156
--- src/backend/commands/tablecmds.c    8 May 2005 02:40:53 -0000
***************
*** 5464,5470 ****
                          indexName, RelationGetRelationName(rel))));

      /* Check index is valid to cluster on */
!     check_index_is_clusterable(rel, indexOid);

      /* And do the work */
      mark_index_clustered(rel, indexOid);
--- 5464,5470 ----
                          indexName, RelationGetRelationName(rel))));

      /* Check index is valid to cluster on */
!     check_index_is_clusterable(rel, indexOid, ERROR);

      /* And do the work */
      mark_index_clustered(rel, indexOid);
Index: src/include/commands/cluster.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/commands/cluster.h,v
retrieving revision 1.27
diff -c -c -r1.27 cluster.h
*** src/include/commands/cluster.h    31 Dec 2004 22:03:28 -0000    1.27
--- src/include/commands/cluster.h    8 May 2005 02:40:54 -0000
***************
*** 19,25 ****

  extern void cluster(ClusterStmt *stmt);

! extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid);
  extern void mark_index_clustered(Relation rel, Oid indexOid);
  extern Oid make_new_heap(Oid OIDOldHeap, const char *NewName,
                Oid NewTableSpace);
--- 19,26 ----

  extern void cluster(ClusterStmt *stmt);

! extern bool check_index_is_clusterable(Relation OldHeap, Oid indexOid,
!               int elevel);
  extern void mark_index_clustered(Relation rel, Oid indexOid);
  extern Oid make_new_heap(Oid OIDOldHeap, const char *NewName,
                Oid NewTableSpace);