Re: [PATCHES] [PATCH] add CLUSTER table USING index (take 2)
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.0 +0200 --- src/doc/src/sgml/ref/cluster.sgml 2007-03-28 23:03:14.0 +0200 *** *** 20,27 refsynopsisdiv synopsis ! CLUSTER replaceable class=PARAMETERindexname/replaceable ON replaceable class=PARAMETERtablename/replaceable ! CLUSTER replaceable class=PARAMETERtablename/replaceable CLUSTER /synopsis /refsynopsisdiv --- 20,26 refsynopsisdiv synopsis ! CLUSTER replaceable class=PARAMETERtablename/replaceable [ USING replaceable class=PARAMETERindexname/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.0 +0200 --- src/src/backend/parser/gram.y 2007-03-28 22:59:15.0 +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
Re: [PATCHES] [PATCH] add CLUSTER table USING index (take 2)
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.0 +0200 --- src/doc/src/sgml/ref/cluster.sgml 2007-03-28 23:03:14.0 +0200 *** *** 20,27 refsynopsisdiv synopsis ! CLUSTER replaceable class=PARAMETERindexname/replaceable ON replaceable class=PARAMETERtablename/replaceable ! CLUSTER replaceable class=PARAMETERtablename/replaceable CLUSTER /synopsis /refsynopsisdiv --- 20,26 refsynopsisdiv synopsis ! CLUSTER replaceable class=PARAMETERtablename/replaceable [ USING replaceable class=PARAMETERindexname/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.0 +0200 --- src/src/backend/parser/gram.y 2007-03-28 22:59:15.0 +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 [EMAIL PROTECTED] http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [PATCH] add CLUSTER table USING index (take 2)
FYI, this is a great example of valuable patch review. It would have been better if the TODO entry would have been rigth :-) ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] [PATCH] add CLUSTER table USING index (take 2)
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.0 +0200 --- src/src/backend/parser/gram.y 2007-03-28 22:59:15.0 +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. ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [PATCH] add CLUSTER table USING index (take 2)
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 [EMAIL PROTECTED] http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] [PATCH] add CLUSTER table USING index (take 2)
Holger Schurig [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
[PATCHES] [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.0
+0200
--- src/doc/src/sgml/ref/cluster.sgml 2007-03-28 23:03:14.0 +0200
***
*** 20,27
refsynopsisdiv
synopsis
! CLUSTER replaceable class=PARAMETERindexname/replaceable ON
replaceable class=PARAMETERtablename/replaceable
! CLUSTER replaceable class=PARAMETERtablename/replaceable
CLUSTER
/synopsis
/refsynopsisdiv
--- 20,26
refsynopsisdiv
synopsis
! CLUSTER replaceable class=PARAMETERtablename/replaceable [ USING
replaceable class=PARAMETERindexname/replaceable ]
CLUSTER
/synopsis
/refsynopsisdiv
Index: src/src/backend/parser/gram.y
===
*** src.orig/src/backend/parser/gram.y 2007-03-28 22:58:48.0 +0200
--- src/src/backend/parser/gram.y 2007-03-28 22:59:15.0 +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.c2007-03-28 22:58:48.0
+0200
--- src/src/bin/psql/tab-complete.c 2007-03-28 22:59:15.0 +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)
!
