SEO

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...3/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);


---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

Seems like an idea to me...

On another note, what about the problem I pointed out where it's not
possible to drop the default on a serial column after you alter it to a
varchar, for example...

Chris


On Sat, 7 May 2005, Bruce Momjian wrote:

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

---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I looked over this item, originally posted as:
> http://archives.postgresql.org/pgsql...3/msg01055.php

I still think this is substantial complication (more than likely
introducing some bugs) to deal with a non problem.

http://archives.postgresql.org/pgsql...3/msg01058.php

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to majordomo@postgresql.org)

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I looked over this item, originally posted as:
> > http://archives.postgresql.org/pgsql...3/msg01055.php
>
> I still think this is substantial complication (more than likely
> introducing some bugs) to deal with a non problem.
>
> http://archives.postgresql.org/pgsql...3/msg01058.php

Well, it is a demonstrated problem, and we currently don't even report
the index name that caused the cluster to fail.

Here is a new patch that continues to throw an error for a failed
database-wide cluster (no warning) if a single table fails. It does
print the index name and it prints a hint that the user can use ALTER
TABLE ... SET WITHOUT CLUSTER to avoid the failure:

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;
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, or use ALTER TABLE ... SET WITHOUT CLUSTER to remove the cluster
specification from the table.

--
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 10 May 2005 00:26:48 -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,298 ----
OldHeap = heap_open(rvtc->tableOid, AccessExclusiveLock);

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

/* rebuild_relation does all the dirty work */
rebuild_relation(OldHeap, rvtc->indexOid);
***************
*** 309,315 ****
* definition can't change under us.
*/
void
! check_index_is_clusterable(Relation OldHeap, Oid indexOid)
{
Relation OldIndex;

--- 309,315 ----
* definition can't change under us.
*/
void
! check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck)
{
Relation OldIndex;

***************
*** 336,342 ****
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;
--- 336,344 ----
if (!heap_attisnull(OldIndex->rd_indextuple, Anum_pg_index_indpred))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg("cannot cluster on partial index \"%s\"",
! RelationGetRelationName(OldIndex))));
!
if (!OldIndex->rd_am->amindexnulls)
{
AttrNumber colno;
***************
*** 354,374 ****
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)
{
/* system column --- okay, always non-null */
}
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")));
! }
}

/*
--- 356,380 ----
if (!OldHeap->rd_att->attrs[colno - 1]->attnotnull)
ereport(ERROR,
(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%s",
! NameStr(OldHeap->rd_att->attrs[colno - 1]->attname),
! recheck ? ",\nor use ALTER TABLE ... SET WITHOUT CLUSTER to remove the cluster\n"
! "specification from the table." : ".")));
}
else if (colno < 0)
{
/* system column --- okay, always non-null */
}
else
/* index expression, lose... */
ereport(ERROR,
(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: 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 10 May 2005 00:26:51 -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, false);

/* 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 10 May 2005 00:26:53 -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 void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
! bool recheck);
extern void mark_index_clustered(Relation rel, Oid indexOid);
extern Oid make_new_heap(Oid OIDOldHeap, const char *NewName,
Oid NewTableSpace);


---------------------------(end of broadcast)---------------------------
TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org

Christopher Kings-Lynne wrote:
> Seems like an idea to me...
>
> On another note, what about the problem I pointed out where it's not
> possible to drop the default on a serial column after you alter it to a
> varchar, for example...

Uh, should we allow a columns data type to be changed if it has a
default, and if so, how? It seems a conversion should have to apply to
the default value as well.

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

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly