Re: [PATCHES] Auto create (top level) directory for create tablespace

[Mark Kirkwood]

Tom Lane wrote:

Mark Kirkwood <[EMAIL PROTECTED]> writes:
  
I thought it made sense for CREATE TABLESPACE to attempt to create the 
top level location directory -



I thought we had deliberately made it not do that.  Auto-recreate during
replay sounds even worse.  The problem is that a tablespace would
normally be under a mount point, and auto-create has zero chance of
getting such a path right.

Ignoring this point is actually a fine recipe for destroying your data;
see Joe Conway's report a couple years back about getting burnt by a
soft NFS mount.  If the DB directory is not there, auto-creating it is
a horrible idea.

  


Hmm - ok, unmounted filesystems could bite you. However, they could bite 
folks creating the directory manually too...(I guess you could argue it 
is less likely though).


On the replay front, the use case I was thinking about is standby 
database - the classic foot gun there is to create a tablespace on 
source box and forget to add the appropriate directory on the target 
and bang! replay fails.


It does seem to me like there are scenarios where either behavior is 
undesirable... a possible option is a configuration parameter to choose 
between auto creation or not. However I'm happy to go with the consensus 
here - if its universally deemed to be a terrible idea, then let's ditch 
the patch :-)


Best wishes

Mark

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

Re: [PATCHES] Auto create (top level) directory for create tablespace

[Tom Lane]

Mark Kirkwood <[EMAIL PROTECTED]> writes:
> I thought it made sense for CREATE TABLESPACE to attempt to create the 
> top level location directory -

I thought we had deliberately made it not do that.  Auto-recreate during
replay sounds even worse.  The problem is that a tablespace would
normally be under a mount point, and auto-create has zero chance of
getting such a path right.

Ignoring this point is actually a fine recipe for destroying your data;
see Joe Conway's report a couple years back about getting burnt by a
soft NFS mount.  If the DB directory is not there, auto-creating it is
a horrible idea.

regards, tom lane

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

[PATCHES] Auto create (top level) directory for create tablespace

[Mark Kirkwood]

I thought it made sense for CREATE TABLESPACE to attempt to create the 
top level location directory - and also for tablespace redo to do 
likwewise during WAL replay.


Tablespace creation then behaves a bit more like intidb with respect to 
directory creation, which I found quite nice.


Patch against HEAD, passes regression tests.

Cheers

Mark
Index: src/backend/commands/tablespace.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/commands/tablespace.c,v
retrieving revision 1.51
diff -c -r1.51 tablespace.c
*** src/backend/commands/tablespace.c	15 Nov 2007 21:14:34 -	1.51
--- src/backend/commands/tablespace.c	15 Dec 2007 19:04:45 -
***
*** 199,204 
--- 199,205 
  	Oid			tablespaceoid;
  	char	   *location;
  	char	   *linkloc;
+ 	struct stat	st;
  	Oid			ownerId;
  
  	/* Must be super user */
***
*** 297,302 
--- 298,317 
  	recordDependencyOnOwner(TableSpaceRelationId, tablespaceoid, ownerId);
  
  	/*
+ 	 * Try to create the target directory if it does not exist.
+ 	 */
+ 	if (stat(location, &st) < 0)
+ 	{
+ 		if (mkdir(location, 0700) != 0)
+ ereport(ERROR,
+ 	(errcode_for_file_access(),
+  errmsg("could not create location directory \"%s\": %m",
+ 		location)));
+ 	
+ 	}
+ 
+ 
+ 	/*
  	 * Attempt to coerce target directory to safe permissions.	If this fails,
  	 * it doesn't exist or has the wrong owner.
  	 */
***
*** 1279,1284 
--- 1294,1314 
  		xl_tblspc_create_rec *xlrec = (xl_tblspc_create_rec *) XLogRecGetData(record);
  		char	   *location = xlrec->ts_path;
  		char	   *linkloc;
+ 		struct stat	st;
+ 
+ 		/*
+ 		 * Try to create the target directory if it does not exist.
+ 		 */
+ 		if (stat(location, &st) < 0)
+ 		{
+ 			if (mkdir(location, 0700) != 0)
+ 			{
+ ereport(ERROR,
+ 		(errcode_for_file_access(),
+ 	 errmsg("could not create location directory \"%s\": %m",
+ 			location)));
+ 			}
+ 		}
  
  		/*
  		 * Attempt to coerce target directory to safe permissions.	If this
Index: src/test/regress/output/tablespace.source
===
RCS file: /projects/cvsroot/pgsql/src/test/regress/output/tablespace.source,v
retrieving revision 1.5
diff -c -r1.5 tablespace.source
*** src/test/regress/output/tablespace.source	3 Jun 2007 22:16:03 -	1.5
--- src/test/regress/output/tablespace.source	15 Dec 2007 19:04:46 -
***
*** 57,63 
  
  -- Will fail with bad path
  CREATE TABLESPACE badspace LOCATION '/no/such/location';
! ERROR:  could not set permissions on directory "/no/such/location": No such file or directory
  -- No such tablespace
  CREATE TABLE bar (i int) TABLESPACE nosuchspace;
  ERROR:  tablespace "nosuchspace" does not exist
--- 57,63 
  
  -- Will fail with bad path
  CREATE TABLESPACE badspace LOCATION '/no/such/location';
! ERROR:  could not create location directory "/no/such/location": No such file or directory
  -- No such tablespace
  CREATE TABLE bar (i int) TABLESPACE nosuchspace;
  ERROR:  tablespace "nosuchspace" does not exist

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly