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_index es, 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_table s, 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;

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

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

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. +

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

> 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

> 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.


---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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. +

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

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

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org