[PATCH] add CLUSTER table USING index (take 2)

Started by Holger Schurigover 19 years ago7 messagespatches

holgerschurig@gmx.de

over 19 years ago

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;

Heikki Linnakangas

heikki.linnakangas@enterprisedb.com

over 19 years ago

In reply to: Holger Schurig (#1)

Re: [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.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

Bruce Momjian

bruce@momjian.us

over 19 years ago

In reply to: Heikki Linnakangas (#2)

Re: [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.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. +

Holger Schurig

holgerschurig@gmx.de

over 19 years ago

In reply to: Heikki Linnakangas (#2)

Re: [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.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

holgerschurig@gmx.de

over 19 years ago

In reply to: Bruce Momjian (#3)

Re: [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 :-)

Bruce Momjian

bruce@momjian.us

over 19 years ago

In reply to: Holger Schurig (#5)

Re: [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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane

tgl@sss.pgh.pa.us

over 19 years ago

In reply to: Holger Schurig (#4)

Re: [PATCH] add CLUSTER table USING index (take 2)

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