On 01/15/2014 03:07 AM, Michael Paquier wrote:
> I just had a quick look at this patch, no testing at all. 

Thank you for looking at it.

> I am seeing that regression tests are still missing here, they should be 
> added in
> src/test/regress/input/tablespace.source. 

New patch attached, with regression tests.

> Then, for the use of either
> WITH or SET... For example CREATE FUNCTION uses SET for a
> configuration parameter, and ALTER FUNCTION is consistent with that.
> So SET makes more sense to be consistent with CREATE TABLESPACE? This
> patch should not be modified once again as long as there are no more
> opinions though...

Based on your analysis of current behavior elsewhere, and the messier
patch with SET, I have switched my vote from ambivalent to preferring WITH.

-- 
Vik

*** a/doc/src/sgml/ref/create_tablespace.sgml
--- b/doc/src/sgml/ref/create_tablespace.sgml
***************
*** 21,27 **** PostgreSQL documentation
  
   <refsynopsisdiv>
  <synopsis>
! CREATE TABLESPACE <replaceable class="parameter">tablespace_name</replaceable> [ OWNER <replaceable class="parameter">user_name</replaceable> ] LOCATION '<replaceable class="parameter">directory</replaceable>'
  </synopsis>
   </refsynopsisdiv>
  
--- 21,30 ----
  
   <refsynopsisdiv>
  <synopsis>
! CREATE TABLESPACE <replaceable class="parameter">tablespace_name</replaceable>
!     [ OWNER <replaceable class="parameter">user_name</replaceable> ]
!     LOCATION '<replaceable class="parameter">directory</replaceable>'
!     [ WITH ( <replaceable class="PARAMETER">tablespace_option</replaceable> = <replaceable class="PARAMETER">value</replaceable> [, ... ] ) ]
  </synopsis>
   </refsynopsisdiv>
  
***************
*** 87,92 **** CREATE TABLESPACE <replaceable class="parameter">tablespace_name</replaceable> [
--- 90,113 ----
         </para>
        </listitem>
       </varlistentry>
+ 
+      <varlistentry>
+       <term><replaceable class="parameter">tablespace_option</replaceable></term>
+       <listitem>
+        <para>
+         A tablespace parameter to be set or reset.  Currently, the only
+         available parameters are <varname>seq_page_cost</> and
+         <varname>random_page_cost</>.  Setting either value for a particular
+         tablespace will override the planner's usual estimate of the cost of
+         reading pages from tables in that tablespace, as established by
+         the configuration parameters of the same name (see
+         <xref linkend="guc-seq-page-cost">,
+         <xref linkend="guc-random-page-cost">).  This may be useful if one
+         tablespace is located on a disk which is faster or slower than the
+         remainder of the I/O subsystem.
+        </para>
+       </listitem>
+      </varlistentry>
    </variablelist>
   </refsect1>
  
*** a/src/backend/commands/tablespace.c
--- b/src/backend/commands/tablespace.c
***************
*** 234,239 **** CreateTableSpace(CreateTableSpaceStmt *stmt)
--- 234,240 ----
  	Oid			tablespaceoid;
  	char	   *location;
  	Oid			ownerId;
+ 	Datum		newOptions;
  
  	/* Must be super user */
  	if (!superuser())
***************
*** 317,323 **** CreateTableSpace(CreateTableSpaceStmt *stmt)
  	values[Anum_pg_tablespace_spcowner - 1] =
  		ObjectIdGetDatum(ownerId);
  	nulls[Anum_pg_tablespace_spcacl - 1] = true;
! 	nulls[Anum_pg_tablespace_spcoptions - 1] = true;
  
  	tuple = heap_form_tuple(rel->rd_att, values, nulls);
  
--- 318,333 ----
  	values[Anum_pg_tablespace_spcowner - 1] =
  		ObjectIdGetDatum(ownerId);
  	nulls[Anum_pg_tablespace_spcacl - 1] = true;
! 
! 	/* Generate new proposed spcoptions (text array) */
! 	newOptions = transformRelOptions((Datum) 0,
! 									 stmt->options,
! 									 NULL, NULL, false, false);
! 	(void) tablespace_reloptions(newOptions, true);
! 	if (newOptions != (Datum) 0)
! 		values[Anum_pg_tablespace_spcoptions - 1] = newOptions;
! 	else
! 		nulls[Anum_pg_tablespace_spcoptions - 1] = true;
  
  	tuple = heap_form_tuple(rel->rd_att, values, nulls);
  
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 3370,3375 **** _copyCreateTableSpaceStmt(const CreateTableSpaceStmt *from)
--- 3370,3376 ----
  	COPY_STRING_FIELD(tablespacename);
  	COPY_STRING_FIELD(owner);
  	COPY_STRING_FIELD(location);
+ 	COPY_NODE_FIELD(options);
  
  	return newnode;
  }
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 1610,1615 **** _equalCreateTableSpaceStmt(const CreateTableSpaceStmt *a, const CreateTableSpace
--- 1610,1616 ----
  	COMPARE_STRING_FIELD(tablespacename);
  	COMPARE_STRING_FIELD(owner);
  	COMPARE_STRING_FIELD(location);
+ 	COMPARE_NODE_FIELD(options);
  
  	return true;
  }
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***************
*** 3588,3599 **** opt_procedural:
   *
   *****************************************************************************/
  
! CreateTableSpaceStmt: CREATE TABLESPACE name OptTableSpaceOwner LOCATION Sconst
  				{
  					CreateTableSpaceStmt *n = makeNode(CreateTableSpaceStmt);
  					n->tablespacename = $3;
  					n->owner = $4;
  					n->location = $6;
  					$$ = (Node *) n;
  				}
  		;
--- 3588,3600 ----
   *
   *****************************************************************************/
  
! CreateTableSpaceStmt: CREATE TABLESPACE name OptTableSpaceOwner LOCATION Sconst opt_reloptions
  				{
  					CreateTableSpaceStmt *n = makeNode(CreateTableSpaceStmt);
  					n->tablespacename = $3;
  					n->owner = $4;
  					n->location = $6;
+ 					n->options = $7;
  					$$ = (Node *) n;
  				}
  		;
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 1669,1674 **** typedef struct CreateTableSpaceStmt
--- 1669,1675 ----
  	char	   *tablespacename;
  	char	   *owner;
  	char	   *location;
+ 	List	   *options;
  } CreateTableSpaceStmt;
  
  typedef struct DropTableSpaceStmt
*** a/src/test/regress/input/tablespace.source
--- b/src/test/regress/input/tablespace.source
***************
*** 1,3 ****
--- 1,13 ----
+ -- create a tablespace using WITH clause
+ CREATE TABLESPACE testspacewith LOCATION '@testtablespace@' WITH (some_nonexistent_parameter = true); -- fail
+ CREATE TABLESPACE testspacewith LOCATION '@testtablespace@' WITH (random_page_cost = 3.0); -- ok
+ 
+ -- check to see the parameter was used
+ SELECT spcoptions FROM pg_tablespace WHERE spcname = 'testspacewith';
+ 
+ -- drop the tablespace so we can re-use the location
+ DROP TABLESPACE testspacewith;
+ 
  -- create a tablespace we can use
  CREATE TABLESPACE testspace LOCATION '@testtablespace@';
  
*** a/src/test/regress/output/tablespace.source
--- b/src/test/regress/output/tablespace.source
***************
*** 1,3 ****
--- 1,16 ----
+ -- create a tablespace using WITH clause
+ CREATE TABLESPACE testspacewith LOCATION '/home/vik/postgresql/9.4/git/src/test/regress/testtablespace' WITH (some_nonexistent_parameter = true); -- fail
+ ERROR:  unrecognized parameter "some_nonexistent_parameter"
+ CREATE TABLESPACE testspacewith LOCATION '/home/vik/postgresql/9.4/git/src/test/regress/testtablespace' WITH (random_page_cost = 3.0); -- ok
+ -- check to see the parameter was used
+ SELECT spcoptions FROM pg_tablespace WHERE spcname = 'testspacewith';
+        spcoptions       
+ ------------------------
+  {random_page_cost=3.0}
+ (1 row)
+ 
+ -- drop the tablespace so we can re-use the location
+ DROP TABLESPACE testspacewith;
  -- create a tablespace we can use
  CREATE TABLESPACE testspace LOCATION '@testtablespace@';
  -- try setting and resetting some properties for the new tablespace
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to