Bug in DROP NOT NULL

Sorry, was in a rush before. I still don't have time to fix this for
8.0.2, so that's why I rushed out the report. Here is a full description...

You can drop a NOT NULL on a column, even if that column is part of an
index that is clustered, where the index does not index NULLs.

First, install tsearch2...

test=# create table test (a tsvector);
CREATE TABLE
test=# create index test_gist_idx on test using gist (a);
CREATE INDEX
test=# \d test
Table "public.test"
Column | Type | Modifiers
--------+----------+-----------
a | tsvector |
Indexes:
"test_gist_idx" gist (a)

test=# cluster test_gist_idx on test;
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.
test=# alter table test alter a set not null;
ALTER TABLE
test=# cluster test_gist_idx on test;
CLUSTER
test=# \d test
Table "public.test"
Column | Type | Modifiers
--------+----------+-----------
a | tsvector | not null
Indexes:
"test_gist_idx" gist (a) CLUSTER

test=# alter table test alter a drop not null;
ALTER TABLE
test=# cluster test_gist_idx on test;
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.
test=# \d test
Table "public.test"
Column | Type | Modifiers
--------+----------+-----------
a | tsvector |
Indexes:
"test_gist_idx" gist (a) CLUSTER

Note that index is still 'clustered', but unclusterable.

The correct behaviour IMHO is to prevent dropping NOT NULL on a column
that particpates in such an index. (Index access method that does not
handle nulls)

Also, I dont think that ALTER TABLE blah CLUSTER ON foo; actually warns
about clustering a non-null indexing index. However, CLUSTER foo ON
blah; does.

I was wrong about that...

Chris

Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:

You can drop a NOT NULL on a column, even if that column is part of an
index that is clustered, where the index does not index NULLs.

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.

regards, tom lane

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.

Chris

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.

Any further comments on this? I still think it's a bug, ie. an
invariant that's not being maintained...

Chris

On Mon, Apr 04, 2005 at 10:17:40PM +0800, 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.

Any further comments on this? I still think it's a bug, ie. an
invariant that's not being maintained...

I agree with your observation. What sense does it make to allow marking
an index "clusterable", knowing in advance that it will error out at
cluster time? None to me.

--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"La persona que no quer�a pecar / estaba obligada a sentarse
en duras y empinadas sillas / desprovistas, por cierto
de blandos atenuantes" (Patricio Vogel)

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

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:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I looked over this item, originally posted as:
http://archives.postgresql.org/pgsql-hackers/2005-03/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-hackers/2005-03/msg01058.php

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I looked over this item, originally posted as:
http://archives.postgresql.org/pgsql-hackers/2005-03/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-hackers/2005-03/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

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