Tablespace patch review - Mailing list pgsql-patches

Gavin Sherry wrote:
> Attached is an updated patch, fixing a compile error which my compiler
> didn't seem to detect/suffer from and incorporating fixes to problems
> raised by Neil.
>
> Thanks,
>
> Gavin

OK, I have reviewed the patch.  I think Tom is doing the same, but I
want to report the things I found.

First, let me say that the patch is very good.  It adds the
creation/destruction of tablespaces and the infrastructure for object
creation in various tablespaces.  The patch seems to cover all the areas
need for change, including reference manual changes.  Even though I have
found a number of items to question, these are nit-pick items and should
not reflect on the high quality of the patch.

Here is what I found:

In create_tablespace.sgml:

    +        <para>
    +     The name of a schema to be created.
    +        </para>

I assume this should say "tablespace".

In drop_tablespace.sgml, it says all objects must be removed from the
tablespace to drop it, including objects created in other databases.  I
imagine it is going to be pretty hard for folks to find all the objects
defined in a tablespaces.  We might need to add instructions on how to
do that.

I assume this change in xlog.c wasn't intended:

    ! bool        XLOG_DEBUG = true;

I see the global tablespace is for global tables, and the default is for
everything else.

In catalog.c, should this be pg_tablespaces or pg_tablespace?

!         sprintf(path, "%s/pg_tablespaces/%u/global/%u", DataDir,

I don't think we pluralize often.  For example, we use "global", not
"globals".

What facility is there for moving objects between tablespaces?

This "<" in dbcommands.c should be removed:

+      * pg_tablespaces/<tblspcoid/src_dbid exists and if so, copy it to the new

Because Win32 doesn't have symlinks, I assume we don't have to support
it with copy:

+         snprintf(buf, sizeof(buf), "cp -r '%s' '%s'", tblspcpath, tblspctarget);

Ah, later I see:

    + #ifndef WIN32
    +         snprintf(buf, sizeof(buf), "rm -rf '%s'", dbdir);
    + #else
    +         snprintf(buf, sizeof(buf), "rmdir /s /q \"%s\"", dbdir);
    + #endif

Seems we should be consistent in having WIN32 defs or not.  I suggest
adding a WIN32 define for copy.  Also, we might find a way to do
symlinks on Win32.

In tablespace.c, typo, "wee":

+  * To simplify initialization and XLog activity, wee create and maintain

same file use->user:

+  * (and also as a feature unto itself) when a use creates an object without

It seems that stat() test in tablespc.c should check errno for ENOENT.

Your code in tablespc.c calls realpath().  Do all OS's have that?  Also,
my docs say:

CAVEATS
     This implementation of realpath() differs slightly from the Solaris im-
     plementation.  The 4.4BSD version always returns absolute pathnames,
     whereas the Solaris implementation will, under certain circumstances, re-
     turn a relative resolved_path when given a relative pathname.

SEE ALSO
     getcwd(3)

HISTORY
     The realpath() function call first appeared in 4.4BSD.

I think we might have portability problems with it, but we can find out
once it is applied.

Would this be clearer as true/false?

  + tblnameexists = (strcmp(NameStr(tmpname), stmt->tblspcname)  == 0 ? 1 : 0);

Also in tablespc.c, I see the comment:

+          * We want to avoid situations where user passes in
+          * /path/to/tblspc and a tablespace exists with a loc of
+          * /path/to/tblspc/
+          *                ^

port/path.c now has trim_trailing_separator() that might help, though I
don't see any code related to that comment.

Another 1/0 case:

  + tblspcexists = (strncmp(realp, realnewpath, PATH_MAX) == 0 ? 1 : 0);

Does all object creation code put a lock in the tablespace row to
prevent DROP TABLESPACE from removing a tablespace in use?

If someone creates a non-directory file in the tablespace directory, we
report "Can not open directory".  Should we test explicitly for
non-directory files in there?

I would like to see this code converted to a macro:

+             if(sde->d_name[0] == '.' &&
+                     (sde->d_name[1] == '\0' ||
+                     (sde->d_name[1] == '.' && sde->d_name[2] == '\0')))


There is interesting code that checks to see of the objects existing in
a tablespace are about to be dropped by the transaction.

I think this should be a for() loop:

+                 int ntmp = npdels;
+                 while(ntmp > 0)

If we can't drop a tablespace, should we report the database and
relfilenode of one of the entries that is preventing the drop.

In commands/*.c I see two references to:

!     createStmt->tblspcname = "";

Is that the proper way to specify no tablespace name, rather than NULL?

I see this in gram.y:

+ OptTableSpace:   TABLESPACE name                    { $$ = $2; }
+             | /*EMPTY*/                                { $$ = ""; }
+         ;
+

Again, shouldn't we be using NULL?  The only '= ""' I see in gram.y is:

    opt_lancompiler:
                LANCOMPILER Sconst                      { $$ = $2; }
                | /*EMPTY*/                             { $$ = ""; }
            ;

Are we ripping out our initlocation code at the same time?

Would someone research if we can get WIN32 to work for this with symlinks?

Typo in initdb.c:

+  * make a symbolic link from old to new. Preceed pathes with pg_data

We need to check for duplicate oids after patch application.

Where do we need to add mention of tablespaces in the main
non-reference-page docs?  Clearly at least in the section on managing
disk space.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073