Thread: SET WITHOUT CLUSTER patch
Hi,
I have done a patch for turning off clustering on a table entirely.
Unforunately, of the three syntaxes I can think of, all cause
shift/reduce errors:
SET WITHOUT CLUSTER;
DROP CLUSTER
CLUSTER ON NONE;
This is the new grammar that I added:
/* ALTER TABLE <name> SET WITHOUT CLUSTER */
| ALTER TABLE relation_expr SET WITHOUT CLUSTER
{
AlterTableStmt *n = makeNode(AlterTableStmt);
n->subtype = 'L';
n->relation = $3;
n->name = NULL;
$$ = (Node *)n;
}
Now, I have to change that relation_expr to qualified_name. However,
this causes shift/reduce errors. (Due to ALTER TABLE relation_expr SET
WITHOUT OIDS.)
Even changing the syntax to "qualified_name DROP CLUSTER" doesn't work
due to the existence of "relation_expr DROP ...".
What's the solution? I can't figure it out...
Chris
Index: doc/src/sgml/ref/alter_table.sgml
===================================================================
RCS file: /projects/cvsroot/pgsql-server/doc/src/sgml/ref/alter_table.sgml,v
retrieving revision 1.66
diff -c -r1.66 alter_table.sgml
*** doc/src/sgml/ref/alter_table.sgml 9 Mar 2004 16:57:47 -0000 1.66
--- doc/src/sgml/ref/alter_table.sgml 18 Mar 2004 03:51:41 -0000
***************
*** 47,52 ****
--- 47,54 ----
OWNER TO <replaceable class="PARAMETER">new_owner</replaceable>
ALTER TABLE <replaceable class="PARAMETER">name</replaceable>
CLUSTER ON <replaceable class="PARAMETER">index_name</replaceable>
+ ALTER TABLE <replaceable class="PARAMETER">name</replaceable>
+ SET WITHOUT CLUSTER
</synopsis>
</refsynopsisdiv>
***************
*** 219,224 ****
--- 221,235 ----
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>SET WITHOUT CLUSTER</literal></term>
+ <listitem>
+ <para>
+ This form disables future <xref linkend="SQL-CLUSTER" endterm="sql-cluster-title"> on a table.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/commands/tablecmds.c,v
retrieving revision 1.100
diff -c -r1.100 tablecmds.c
*** src/backend/commands/tablecmds.c 13 Mar 2004 22:09:13 -0000 1.100
--- src/backend/commands/tablecmds.c 18 Mar 2004 03:51:42 -0000
***************
*** 3970,3999 ****
rel = heap_open(relOid, AccessExclusiveLock);
- indexOid = get_relname_relid(indexName, rel->rd_rel->relnamespace);
-
- if (!OidIsValid(indexOid))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("index \"%s\" for table \"%s\" does not exist",
- indexName, NameStr(rel->rd_rel->relname))));
-
- indexTuple = SearchSysCache(INDEXRELID,
- ObjectIdGetDatum(indexOid),
- 0, 0, 0);
- if (!HeapTupleIsValid(indexTuple))
- elog(ERROR, "cache lookup failed for index %u", indexOid);
- indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
-
/*
! * If this is the same index the relation was previously clustered on,
! * no need to do anything.
*/
! if (indexForm->indisclustered)
! {
! ReleaseSysCache(indexTuple);
! heap_close(rel, NoLock);
! return;
}
pg_index = heap_openr(IndexRelationName, RowExclusiveLock);
--- 3970,4010 ----
rel = heap_open(relOid, AccessExclusiveLock);
/*
! * We only fetch the index if indexName is not null. A null index
! * name indicates that we're removing all clustering on this table.
*/
! if (indexName != NULL) {
! indexOid = get_relname_relid(indexName, rel->rd_rel->relnamespace);
!
! if (!OidIsValid(indexOid))
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_OBJECT),
! errmsg("index \"%s\" for table \"%s\" does not exist",
! indexName, NameStr(rel->rd_rel->relname))));
!
! indexTuple = SearchSysCache(INDEXRELID,
! ObjectIdGetDatum(indexOid),
! 0, 0, 0);
! if (!HeapTupleIsValid(indexTuple))
! elog(ERROR, "cache lookup failed for index %u", indexOid);
! indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
!
! /*
! * If this is the same index the relation was previously clustered on,
! * no need to do anything.
! */
! if (indexForm->indisclustered)
! {
! ReleaseSysCache(indexTuple);
! heap_close(rel, NoLock);
! return;
! }
! }
! else {
! /* Set to NULL to prevent compiler warnings */
! indexTuple = NULL;
! indexForm = NULL;
}
pg_index = heap_openr(IndexRelationName, RowExclusiveLock);
***************
*** 4016,4022 ****
/*
* Unset the bit if set. We know it's wrong because we checked
! * this earlier.
*/
if (idxForm->indisclustered)
{
--- 4027,4033 ----
/*
* Unset the bit if set. We know it's wrong because we checked
! * this earlier. If we're removing all clustering, we do this too.
*/
if (idxForm->indisclustered)
{
***************
*** 4024,4030 ****
simple_heap_update(pg_index, &idxtuple->t_self, idxtuple);
CatalogUpdateIndexes(pg_index, idxtuple);
}
! else if (idxForm->indexrelid == indexForm->indexrelid)
{
idxForm->indisclustered = true;
simple_heap_update(pg_index, &idxtuple->t_self, idxtuple);
--- 4035,4045 ----
simple_heap_update(pg_index, &idxtuple->t_self, idxtuple);
CatalogUpdateIndexes(pg_index, idxtuple);
}
! /*
! * If the index is the one we're clustering, set its cluster flag to true.
! * However, we do this only if we're not removing all clustering.
! */
! else if (indexName != NULL && idxForm->indexrelid == indexForm->indexrelid)
{
idxForm->indisclustered = true;
simple_heap_update(pg_index, &idxtuple->t_self, idxtuple);
***************
*** 4035,4041 ****
heap_close(pg_index, RowExclusiveLock);
! ReleaseSysCache(indexTuple);
heap_close(rel, NoLock); /* close rel, but keep lock till commit */
}
--- 4050,4056 ----
heap_close(pg_index, RowExclusiveLock);
! if (indexName != NULL) ReleaseSysCache(indexTuple);
heap_close(rel, NoLock); /* close rel, but keep lock till commit */
}
Index: src/backend/parser/gram.y
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/parser/gram.y,v
retrieving revision 2.449
diff -c -r2.449 gram.y
*** src/backend/parser/gram.y 17 Mar 2004 20:48:42 -0000 2.449
--- src/backend/parser/gram.y 18 Mar 2004 03:51:45 -0000
***************
*** 1248,1253 ****
--- 1248,1262 ----
n->name = $6;
$$ = (Node *)n;
}
+ /* ALTER TABLE <name> SET WITHOUT CLUSTER */
+ | ALTER TABLE relation_expr SET WITHOUT CLUSTER
+ {
+ AlterTableStmt *n = makeNode(AlterTableStmt);
+ n->subtype = 'L';
+ n->relation = $3;
+ n->name = NULL;
+ $$ = (Node *)n;
+ }
;
alter_column_default:
Index: src/test/regress/expected/cluster.out
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/test/regress/expected/cluster.out,v
retrieving revision 1.14
diff -c -r1.14 cluster.out
*** src/test/regress/expected/cluster.out 2 Oct 2003 06:32:46 -0000 1.14
--- src/test/regress/expected/cluster.out 18 Mar 2004 03:51:45 -0000
***************
*** 297,302 ****
--- 297,313 ----
clstr_tst_b_c
(1 row)
+ -- Try turning off all clustering
+ ALTER TABLE clstr_tst SET WITHOUT CLUSTER;
+ SELECT pg_class.relname FROM pg_index, pg_class, pg_class AS pg_class_2
+ WHERE pg_class.oid=indexrelid
+ AND indrelid=pg_class_2.oid
+ AND pg_class_2.relname = 'clstr_tst'
+ AND indisclustered;
+ relname
+ ---------
+ (0 rows)
+
-- Verify that clustering all tables does in fact cluster the right ones
CREATE USER clstr_user;
CREATE TABLE clstr_1 (a INT PRIMARY KEY);
Index: src/test/regress/sql/cluster.sql
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/test/regress/sql/cluster.sql,v
retrieving revision 1.7
diff -c -r1.7 cluster.sql
*** src/test/regress/sql/cluster.sql 20 Mar 2003 18:52:48 -0000 1.7
--- src/test/regress/sql/cluster.sql 18 Mar 2004 03:51:46 -0000
***************
*** 95,100 ****
--- 95,108 ----
AND pg_class_2.relname = 'clstr_tst'
AND indisclustered;
+ -- Try turning off all clustering
+ ALTER TABLE clstr_tst SET WITHOUT CLUSTER;
+ SELECT pg_class.relname FROM pg_index, pg_class, pg_class AS pg_class_2
+ WHERE pg_class.oid=indexrelid
+ AND indrelid=pg_class_2.oid
+ AND pg_class_2.relname = 'clstr_tst'
+ AND indisclustered;
+
-- Verify that clustering all tables does in fact cluster the right ones
CREATE USER clstr_user;
CREATE TABLE clstr_1 (a INT PRIMARY KEY);
Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
> This is the new grammar that I added:
> | ALTER TABLE relation_expr SET WITHOUT CLUSTER
> Now, I have to change that relation_expr to qualified_name. However,
> this causes shift/reduce errors. (Due to ALTER TABLE relation_expr SET
> WITHOUT OIDS.)
Well, seems like what you have to do is leave it as relation_expr
as far as bison is concerned, but test in the C-code action and error
out if "*" was specified. (Accepting ONLY seems alright to me.)
You could possibly find a solution at the grammar level but it'd
probably be a much worse kluge ...
regards, tom lane
>>Now, I have to change that relation_expr to qualified_name. However, >>this causes shift/reduce errors. (Due to ALTER TABLE relation_expr SET >>WITHOUT OIDS.) > > Well, seems like what you have to do is leave it as relation_expr > as far as bison is concerned, but test in the C-code action and error > out if "*" was specified. (Accepting ONLY seems alright to me.) Actually, it occurs to me that the SET WITHOUT CLUSTER form CAN recurse. Should I make it do that, even though the CLUSTERON form cannot? Chris
Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
>> Well, seems like what you have to do is leave it as relation_expr
>> as far as bison is concerned, but test in the C-code action and error
>> out if "*" was specified. (Accepting ONLY seems alright to me.)
> Actually, it occurs to me that the SET WITHOUT CLUSTER form CAN recurse.
> Should I make it do that, even though the CLUSTER ON form cannot?
Seems like more work than it's really worth ... but if you wanna ...
regards, tom lane
Christopher Kings-Lynne wrote: > >>Now, I have to change that relation_expr to qualified_name. However, > >>this causes shift/reduce errors. (Due to ALTER TABLE relation_expr SET > >>WITHOUT OIDS.) > > > > Well, seems like what you have to do is leave it as relation_expr > > as far as bison is concerned, but test in the C-code action and error > > out if "*" was specified. (Accepting ONLY seems alright to me.) > > Actually, it occurs to me that the SET WITHOUT CLUSTER form CAN recurse. > Should I make it do that, even though the CLUSTER ON form cannot? I just thought about this. CLUSTER is more of a storage-level specification, rather than a logical one. Seems it is OK that WITOUTH CLUSTER not recurse into inherited tables, especially since the CLUSTER command does not. -- 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, Pennsylvania19073
>>Actually, it occurs to me that the SET WITHOUT CLUSTER form CAN recurse. >> Should I make it do that, even though the CLUSTER ON form cannot? > > I just thought about this. CLUSTER is more of a storage-level > specification, rather than a logical one. Seems it is OK that WITOUTH > CLUSTER not recurse into inherited tables, especially since the CLUSTER > command does not. The patch I submitted earlier already does do recursion - I don't see why it shouldn't really. It's better than failing saying that legal grammar is in fact illegal :) Chris
Christopher Kings-Lynne wrote: > >>Actually, it occurs to me that the SET WITHOUT CLUSTER form CAN recurse. > >> Should I make it do that, even though the CLUSTER ON form cannot? > > > > I just thought about this. CLUSTER is more of a storage-level > > specification, rather than a logical one. Seems it is OK that WITOUTH > > CLUSTER not recurse into inherited tables, especially since the CLUSTER > > command does not. > > The patch I submitted earlier already does do recursion - I don't see > why it shouldn't really. It's better than failing saying that legal > grammar is in fact illegal :) Uh, if the CLUSTER doesn't recurse, the WITHOUT shouldn't either, I think, and throwing an error seems fine to me, even if it isn't the same wording as a syntax error. -- 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, Pennsylvania19073
> Uh, if the CLUSTER doesn't recurse, the WITHOUT shouldn't either, I > think, and throwing an error seems fine to me, even if it isn't the same > wording as a syntax error. Well, maybe - up to you.
Christopher Kings-Lynne wrote: > > Uh, if the CLUSTER doesn't recurse, the WITHOUT shouldn't either, I > > think, and throwing an error seems fine to me, even if it isn't the same > > wording as a syntax error. > > Well, maybe - up to you. Well, if we don't recurse on creation, does it make sense to recurse on destruction? Seems it might surpise people. Do we have that asymetry in any other area? -- 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, Pennsylvania19073
On Sun, May 02, 2004 at 06:23:30PM -0400, Bruce Momjian wrote: > Christopher Kings-Lynne wrote: > > > Uh, if the CLUSTER doesn't recurse, the WITHOUT shouldn't either, I > > > think, and throwing an error seems fine to me, even if it isn't the same > > > wording as a syntax error. > > > > Well, maybe - up to you. > > Well, if we don't recurse on creation, does it make sense to recurse on > destruction? Seems it might surpise people. Do we have that asymetry > in any other area? I'm not sure if it's assymetric. You can't recursively set the cluster bit, because child tables may not have an equally named index. However when you are unsetting the bit it doesn't matter how is the index named. I'm not sure what side does this argument favor. I'd say ALTER TABLE/WITHOUT CLUSTER shouldn't recurse but I don't feel strongly about it. -- Alvaro Herrera (<alvherre[a]dcc.uchile.cl>) "Everybody understands Mickey Mouse. Few understand Hermann Hesse. Hardly anybody understands Einstein. And nobody understands Emperor Norton."
Alvaro Herrera wrote: > On Sun, May 02, 2004 at 06:23:30PM -0400, Bruce Momjian wrote: > > Christopher Kings-Lynne wrote: > > > > Uh, if the CLUSTER doesn't recurse, the WITHOUT shouldn't either, I > > > > think, and throwing an error seems fine to me, even if it isn't the same > > > > wording as a syntax error. > > > > > > Well, maybe - up to you. > > > > Well, if we don't recurse on creation, does it make sense to recurse on > > destruction? Seems it might surpise people. Do we have that asymetry > > in any other area? > > I'm not sure if it's assymetric. You can't recursively set the cluster > bit, because child tables may not have an equally named index. However > when you are unsetting the bit it doesn't matter how is the index named. Right, we can recurse on WITHOUT and not using WITH, but would people expect WITHOUT to recurse? If we allowed indexes to span tables, it would be nice for both to recurse, but because we don't, I think neither should. -- 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, Pennsylvania19073