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
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)
> 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 > > > > %typerelation_name copy_file_name > > database_name access_method_clause > > access_method attr_name > > ! index_name name file_name > > > > %type func_name handler_name qual_Op qual_all_Op subquery_Op > > opt_class opt_validator > > --- 209,215 > > > > %typerelation_name copy_file_name > > database_name access_method_clause > > access_method attr_name > > ! index_name name file_name opt_cluster_using > > > > %type 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 " section. There are many other "%type " 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 " as "opt_encoding" is. ---(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)
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 > > > > > > > > ! CLUSTER indexname ON > > tablename > > ! CLUSTER tablename > > CLUSTER > > > > > > --- 20,26 > > > > > > > > ! CLUSTER tablename [ USING > > indexname ] > > CLUSTER > > > > > > 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 > > > > %typerelation_name copy_file_name > > database_name access_method_clause > > access_method attr_name > > ! index_name name file_name > > > > %type func_name handler_name qual_Op qual_all_Op subquery_Op > > opt_class opt_validator > > --- 209,215 > > > > %typerelation_name copy_file_name > > database_name access_method_clause > > access_method attr_name > > ! index_name name file_name opt_cluster_using > > > > %type 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)
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 ! CLUSTER indexname ON tablename ! CLUSTER tablename CLUSTER --- 20,26 ! CLUSTER tablename [ USING indexname ] CLUSTER 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 relation_name copy_file_name database_name access_method_clause access_method attr_name ! index_name name file_name %type func_name handler_name qual_Op qual_all_Op subquery_Op opt_class opt_validator --- 209,215 %type relation_name copy_file_name database_name access_method_clause access_method attr_name ! index_name name file_name opt_cluster_using %type 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
