Обсуждение: [PATCH] add CLUSTER table USING index (take 2)
Index: src/doc/src/sgml/ref/cluster.sgml
===================================================================
*** src.orig/doc/src/sgml/ref/cluster.sgml 2007-03-28 23:02:12.000000000 +0200
--- src/doc/src/sgml/ref/cluster.sgml 2007-03-28 23:03:14.000000000 +0200
***************
*** 20,27 ****
<refsynopsisdiv>
<synopsis>
! CLUSTER <replaceable class="PARAMETER">indexname</replaceable> ON <replaceable
class="PARAMETER">tablename</replaceable>
! CLUSTER <replaceable class="PARAMETER">tablename</replaceable>
CLUSTER
</synopsis>
</refsynopsisdiv>
--- 20,26 ----
<refsynopsisdiv>
<synopsis>
! CLUSTER <replaceable class="PARAMETER">tablename</replaceable> [ USING <replaceable
class="PARAMETER">indexname</replaceable>]
CLUSTER
</synopsis>
</refsynopsisdiv>
Index: src/src/backend/parser/gram.y
===================================================================
*** src.orig/src/backend/parser/gram.y 2007-03-28 22:58:48.000000000 +0200
--- src/src/backend/parser/gram.y 2007-03-28 22:59:15.000000000 +0200
***************
*** 209,215 ****
%type <str> relation_name copy_file_name
database_name access_method_clause access_method attr_name
! index_name name file_name
%type <list> func_name handler_name qual_Op qual_all_Op subquery_Op
opt_class opt_validator
--- 209,215 ----
%type <str> relation_name copy_file_name
database_name access_method_clause access_method attr_name
! index_name name file_name opt_cluster_using
%type <list> func_name handler_name qual_Op qual_all_Op subquery_Op
opt_class opt_validator
***************
*** 5327,5332 ****
--- 5327,5333 ----
*
* QUERY:
* cluster <index_name> on <qualified_name>
+ * cluster <qualified_name> USING <index_name>
* cluster <qualified_name>
* cluster
*
***************
*** 5340,5350 ****
n->indexname = $2;
$$ = (Node*)n;
}
! | CLUSTER qualified_name
{
ClusterStmt *n = makeNode(ClusterStmt);
n->relation = $2;
! n->indexname = NULL;
$$ = (Node*)n;
}
| CLUSTER
--- 5341,5351 ----
n->indexname = $2;
$$ = (Node*)n;
}
! | CLUSTER qualified_name opt_cluster_using
{
ClusterStmt *n = makeNode(ClusterStmt);
n->relation = $2;
! n->indexname = $3;
$$ = (Node*)n;
}
| CLUSTER
***************
*** 5356,5361 ****
--- 5357,5368 ----
}
;
+ opt_cluster_using:
+ USING index_name { $$ = $2; }
+ | /*EMPTY*/ { $$ = NULL; }
+ ;
+
+
/*****************************************************************************
*
* QUERY:
Index: src/src/bin/psql/tab-complete.c
===================================================================
*** src.orig/src/bin/psql/tab-complete.c 2007-03-28 22:58:48.000000000 +0200
--- src/src/bin/psql/tab-complete.c 2007-03-28 22:59:15.000000000 +0200
***************
*** 822,832 ****
COMPLETE_WITH_LIST(list_COLUMNALTER);
}
! else if (pg_strcasecmp(prev3_wd, "TABLE") == 0 &&
! pg_strcasecmp(prev_wd, "CLUSTER") == 0)
COMPLETE_WITH_CONST("ON");
else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 &&
- pg_strcasecmp(prev2_wd, "CLUSTER") == 0 &&
pg_strcasecmp(prev_wd, "ON") == 0)
{
completion_info_charp = prev3_wd;
--- 822,830 ----
COMPLETE_WITH_LIST(list_COLUMNALTER);
}
! else if (pg_strcasecmp(prev3_wd, "TABLE") == 0)
COMPLETE_WITH_CONST("ON");
else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 &&
pg_strcasecmp(prev_wd, "ON") == 0)
{
completion_info_charp = prev3_wd;
***************
*** 929,952 ****
/*
* If the previous word is CLUSTER and not without produce list of
! * indexes.
*/
else if (pg_strcasecmp(prev_wd, "CLUSTER") == 0 &&
pg_strcasecmp(prev2_wd, "WITHOUT") != 0)
! COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_indexes, NULL);
! /* If we have CLUSTER <sth>, then add "ON" */
else if (pg_strcasecmp(prev2_wd, "CLUSTER") == 0 &&
! pg_strcasecmp(prev_wd, "ON") != 0)
! COMPLETE_WITH_CONST("ON");
/*
! * If we have CLUSTER <sth> ON, then add the correct tablename as well.
*/
else if (pg_strcasecmp(prev3_wd, "CLUSTER") == 0 &&
! pg_strcasecmp(prev_wd, "ON") == 0)
{
completion_info_charp = prev2_wd;
! COMPLETE_WITH_QUERY(Query_for_table_owning_index);
}
/* COMMENT */
--- 927,951 ----
/*
* If the previous word is CLUSTER and not without produce list of
! * tables
*/
else if (pg_strcasecmp(prev_wd, "CLUSTER") == 0 &&
pg_strcasecmp(prev2_wd, "WITHOUT") != 0)
! COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, NULL);
! /* If we have CLUSTER <sth>, then add "USING" */
else if (pg_strcasecmp(prev2_wd, "CLUSTER") == 0 &&
! pg_strcasecmp(prev_wd, "ON") != 0) {
! COMPLETE_WITH_CONST("USING");
! }
/*
! * If we have CLUSTER <sth> USING, then add the index as well.
*/
else if (pg_strcasecmp(prev3_wd, "CLUSTER") == 0 &&
! pg_strcasecmp(prev_wd, "USING") == 0)
{
completion_info_charp = prev2_wd;
! COMPLETE_WITH_QUERY(Query_for_index_of_table);
}
/* COMMENT */
Index: src/src/test/regress/expected/cluster.out
===================================================================
*** src.orig/src/test/regress/expected/cluster.out 2007-03-28 22:58:48.000000000 +0200
--- src/src/test/regress/expected/cluster.out 2007-03-28 22:59:15.000000000 +0200
***************
*** 329,335 ****
CLUSTER clstr_2;
ERROR: there is no previously clustered index for table "clstr_2"
CLUSTER clstr_1_pkey ON clstr_1;
! CLUSTER clstr_2_pkey ON clstr_2;
SELECT * FROM clstr_1 UNION ALL
SELECT * FROM clstr_2 UNION ALL
SELECT * FROM clstr_3;
--- 329,335 ----
CLUSTER clstr_2;
ERROR: there is no previously clustered index for table "clstr_2"
CLUSTER clstr_1_pkey ON clstr_1;
! CLUSTER clstr_2 USING clstr_2_pkey;
SELECT * FROM clstr_1 UNION ALL
SELECT * FROM clstr_2 UNION ALL
SELECT * FROM clstr_3;
Index: src/src/test/regress/sql/cluster.sql
===================================================================
*** src.orig/src/test/regress/sql/cluster.sql 2007-03-28 22:58:48.000000000 +0200
--- src/src/test/regress/sql/cluster.sql 2007-03-28 22:59:15.000000000 +0200
***************
*** 122,128 ****
CLUSTER clstr_2;
CLUSTER clstr_1_pkey ON clstr_1;
! CLUSTER clstr_2_pkey ON clstr_2;
SELECT * FROM clstr_1 UNION ALL
SELECT * FROM clstr_2 UNION ALL
SELECT * FROM clstr_3;
--- 122,128 ----
CLUSTER clstr_2;
CLUSTER clstr_1_pkey ON clstr_1;
! CLUSTER clstr_2 USING clstr_2_pkey;
SELECT * FROM clstr_1 UNION ALL
SELECT * FROM clstr_2 UNION ALL
SELECT * FROM clstr_3;
Holger Schurig wrote: > Index: src/doc/src/sgml/ref/cluster.sgml > =================================================================== > *** src.orig/doc/src/sgml/ref/cluster.sgml 2007-03-28 23:02:12.000000000 +0200 > --- src/doc/src/sgml/ref/cluster.sgml 2007-03-28 23:03:14.000000000 +0200 > *************** > *** 20,27 **** > > <refsynopsisdiv> > <synopsis> > ! CLUSTER <replaceable class="PARAMETER">indexname</replaceable> ON <replaceable class="PARAMETER">tablename</replaceable> > ! CLUSTER <replaceable class="PARAMETER">tablename</replaceable> > CLUSTER > </synopsis> > </refsynopsisdiv> > --- 20,26 ---- > > <refsynopsisdiv> > <synopsis> > ! CLUSTER <replaceable class="PARAMETER">tablename</replaceable> [ USING <replaceable class="PARAMETER">indexname</replaceable>] > CLUSTER > </synopsis> > </refsynopsisdiv> We still need to document the old syntax, especially if we don't change the example as well. > Index: src/src/backend/parser/gram.y > =================================================================== > *** src.orig/src/backend/parser/gram.y 2007-03-28 22:58:48.000000000 +0200 > --- src/src/backend/parser/gram.y 2007-03-28 22:59:15.000000000 +0200 > *************** > *** 209,215 **** > > %type <str> relation_name copy_file_name > database_name access_method_clause access_method attr_name > ! index_name name file_name > > %type <list> func_name handler_name qual_Op qual_all_Op subquery_Op > opt_class opt_validator > --- 209,215 ---- > > %type <str> relation_name copy_file_name > database_name access_method_clause access_method attr_name > ! index_name name file_name opt_cluster_using > > %type <list> func_name handler_name qual_Op qual_all_Op subquery_Op > opt_class opt_validator Is the placement of opt_cluster_using completely arbitrary? I'm not very familiar with the parser, it really looks like those type-definitions are in random order. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
FYI, this is a great example of valuable patch review. --------------------------------------------------------------------------- Heikki Linnakangas wrote: > Holger Schurig wrote: > > Index: src/doc/src/sgml/ref/cluster.sgml > > =================================================================== > > *** src.orig/doc/src/sgml/ref/cluster.sgml 2007-03-28 23:02:12.000000000 +0200 > > --- src/doc/src/sgml/ref/cluster.sgml 2007-03-28 23:03:14.000000000 +0200 > > *************** > > *** 20,27 **** > > > > <refsynopsisdiv> > > <synopsis> > > ! CLUSTER <replaceable class="PARAMETER">indexname</replaceable> ON <replaceable class="PARAMETER">tablename</replaceable> > > ! CLUSTER <replaceable class="PARAMETER">tablename</replaceable> > > CLUSTER > > </synopsis> > > </refsynopsisdiv> > > --- 20,26 ---- > > > > <refsynopsisdiv> > > <synopsis> > > ! CLUSTER <replaceable class="PARAMETER">tablename</replaceable> [ USING <replaceable class="PARAMETER">indexname</replaceable>] > > CLUSTER > > </synopsis> > > </refsynopsisdiv> > > We still need to document the old syntax, especially if we don't change > the example as well. > > > Index: src/src/backend/parser/gram.y > > =================================================================== > > *** src.orig/src/backend/parser/gram.y 2007-03-28 22:58:48.000000000 +0200 > > --- src/src/backend/parser/gram.y 2007-03-28 22:59:15.000000000 +0200 > > *************** > > *** 209,215 **** > > > > %type <str> relation_name copy_file_name > > database_name access_method_clause access_method attr_name > > ! index_name name file_name > > > > %type <list> func_name handler_name qual_Op qual_all_Op subquery_Op > > opt_class opt_validator > > --- 209,215 ---- > > > > %type <str> relation_name copy_file_name > > database_name access_method_clause access_method attr_name > > ! index_name name file_name opt_cluster_using > > > > %type <list> func_name handler_name qual_Op qual_all_Op subquery_Op > > opt_class opt_validator > > Is the placement of opt_cluster_using completely arbitrary? I'm not very > familiar with the parser, it really looks like those type-definitions > are in random order. > > -- > Heikki Linnakangas > EnterpriseDB http://www.enterprisedb.com > > ---------------------------(end of broadcast)--------------------------- > TIP 2: Don't 'kill -9' the postmaster -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
> FYI, this is a great example of valuable patch review. It would have been better if the TODO entry would have been rigth :-)
> We still need to document the old syntax, especially if we don't change > the example as well. I agree that the example should be re-written. But I'm not sure if I need to have a paragraph about the old syntax. There are two reasons: - I haven't seen any other SQL command where an old syntax was documented - I thought I could come away without writing doc. After all, I'm not a native english speaker. That's a point where I could need some help ... (maybe my english is good enought, but it's not worth to make a "take 4" to "take 17" patch just for english grammar, typos, subtle meanings, whatever. > > Index: src/src/backend/parser/gram.y > > =================================================================== > > *** src.orig/src/backend/parser/gram.y 2007-03-28 22:58:48.000000000 +0200 > > --- src/src/backend/parser/gram.y 2007-03-28 22:59:15.000000000 +0200 > > *************** > > *** 209,215 **** > > > > %type <str> relation_name copy_file_name > > database_name access_method_clause access_method attr_name > > ! index_name name file_name > > > > %type <list> func_name handler_name qual_Op qual_all_Op subquery_Op > > opt_class opt_validator > > --- 209,215 ---- > > > > %type <str> relation_name copy_file_name > > database_name access_method_clause access_method attr_name > > ! index_name name file_name opt_cluster_using > > > > %type <list> func_name handler_name qual_Op qual_all_Op subquery_Op > > opt_class opt_validator > > Is the placement of opt_cluster_using completely arbitrary? I'm not very > familiar with the parser, it really looks like those type-definitions > are in random order. I thought so. As you can see in the above patch, there are things like opt_validator in the next "%type <list>" section. There are many other "%type <str>" section in gram.y, but I haven't found a structure yet. For example, some tokens are named "OptSchemaName", some are named "opt_encoding". Let's look at this one. It's used in line 1090, defined in 1218. Before and after the usage there is "transaction_mode_list" and "Colid_or_Sconst". Before and after the definition is "zone_value" and again "ColId_or_Sconst". But neither of this three is defined at the same "%type <str>" as "opt_encoding" is.
Holger Schurig wrote: > > FYI, this is a great example of valuable patch review. > > It would have been better if the TODO entry would have > been rigth :-) It was right when I wrote it. ;-) I have updated it now. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Holger Schurig <holgerschurig@gmx.de> writes:
> I agree that the example should be re-written. But I'm not sure if I need
> to have a paragraph about the old syntax. There are two reasons:
> - I haven't seen any other SQL command where an old syntax was
> documented
If we were deprecating the old syntax with intention to remove it, that
might be a defensible position, but I didn't think we were doing that.
IMHO both forms seriously do need to be documented so that people will
understand that the index/table order is different. Otherwise there'll
be enormous confusion.
> - I thought I could come away without writing doc. After all, I'm
> not a native english speaker. That's a point where I could need
> some help ... (maybe my english is good enought, but it's not
> worth to make a "take 4" to "take 17" patch just for english
> grammar, typos, subtle meanings, whatever.
Your English seems fine to me, certainly more than good enough to
produce first-draft documentation. Whoever reviews/commits it will
help out as needed.
>> Is the placement of opt_cluster_using completely arbitrary? I'm not very
>> familiar with the parser, it really looks like those type-definitions
>> are in random order.
> I thought so.
Yeah, it's just a mess :=(. Somebody might go through and sort them
into alphabetical order or something someday, but not today.
regards, tom lane