Thread: Tablespaces

Hi all,

Attached is my latest patch implementing tablespaces. This has all the
functionality I was planning for 7.5.

Most of the information about the patch is contained in the
patch/documentation, previous submissions and the archives.

Testing, review, comments would be greatly appreciated.

Gavin

Compile error?

gmake[2]: Entering directory
`/space/1/home/chriskl/pgsql-server/src/timezone'
gcc -O2 -fno-strict-aliasing -g -Wall -Wmissing-prototypes
-Wmissing-declarations -I../../src/include   -c -o pgtz.o pgtz.c -MMD
In file included from ../../src/include/storage/bufmgr.h:20,
                  from ../../src/include/storage/bufpage.h:18,
                  from ../../src/include/access/htup.h:17,
                  from ../../src/include/tcop/dest.h:64,
                  from ../../src/include/utils/guc.h:17,
                  from pgtz.c:26:
../../src/include/storage/relfilenode.h:29: syntax error before `.'
../../src/include/storage/relfilenode.h:29: syntax error before `.'

Chris

Gavin Sherry wrote:

> Hi all,
>
> Attached is my latest patch implementing tablespaces. This has all the
> functionality I was planning for 7.5.
>
> Most of the information about the patch is contained in the
> patch/documentation, previous submissions and the archives.
>
> Testing, review, comments would be greatly appreciated.
>
> Gavin
>
>
> ------------------------------------------------------------------------
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings

Gavin Sherry wrote:
> Attached is my latest patch implementing tablespaces. This has all the
> functionality I was planning for 7.5.

A few minor points I happened to notice while reading through the patch:

+  * To simply initialisation and XLog activity, have create and
maintain
+  * a symbolic link map in data/pg_tablespaces.

Grammar errors.

+ void
+ TblspcCreateDbspace(Oid tbloid)
+ {
+ #ifndef HAVE_SYMLINK
+     return;
+ #endif
+     struct stat st;
+     char        *dir;

If HAVE_SYMLINK is undefined, this is a syntax error (at least in
C89, which is what we ought to limit ourselves to). Similar problems
elsewhere in the same file (tablespc.c)

+     dir = (char *) palloc(strlen(DataDir) + 14 + 10 + 10 + 10 + 3 + 1);
+     sprintf(dir, "%s/pg_tablespaces/%u/%u", DataDir, tbloid,
+                 MyDatabaseId);

Is the length of that buffer right? At the least the addition is a
little weird (why are you adding 10 three times for two numeric
variables?) I noticed another buffer allocation (linkloc) that
looked dubious at first glance as well.

+     char        realnewpath[MAXPGPATH];

This is somewhat pedantic, but how do we know that MAXPGPATH >=
PATH_MAX (the minimum safe size of the second argument to
realpath(), at least on my local system)?

-Neil

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

Just reminding someone to review this some time...it does all seem to
work very well :)

Chris

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
>
>
> ------------------------------------------------------------------------
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index scan if your
>       joining column's datatypes do not match

Yes, I was wondering if it was ready for application.  Tom, you want to
eyeball it?

---------------------------------------------------------------------------

Christopher Kings-Lynne wrote:
> Just reminding someone to review this some time...it does all seem to
> work very well :)
>
> Chris
>
> 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
> >
> >
> > ------------------------------------------------------------------------
> >
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 9: the planner will ignore your desire to choose an index scan if your
> >       joining column's datatypes do not match
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
>

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Yes, I was wondering if it was ready for application.  Tom, you want to
> eyeball it?

I do.  I'd like to get the composite-type-column stuff out of the way
first, and then I'll buckle down to reviewing patches (this one and the
other major patches in the queue...)  Should be able to start on that
within a couple days.

            regards, tom lane

Your patch has been added to the PostgreSQL unapplied patches list at:

    http://momjian.postgresql.org/cgi-bin/pgpatches

I will try to apply it after review..

---------------------------------------------------------------------------


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

Content-Description:

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index scan if your
>       joining column's datatypes do not match

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

I'm starting to review this patch, and almost immediately came across
what seemed a spectacularly bad idea:

*** src/backend/storage/buffer/bufmgr.c    8 May 2004 19:09:25 -0000    1.165
--- src/backend/storage/buffer/bufmgr.c    26 May 2004 06:21:01 -0000
***************
*** 1148,1152 ****
          {
              bufHdr = &LocalBufferDescriptors[i];
!             if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
              {
                  bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
--- 1148,1156 ----
          {
              bufHdr = &LocalBufferDescriptors[i];
!             /* special case for default tblNode */
!             if (RelFileNodeEquals(bufHdr->tag.rnode, rnode) ||
!                     (!OidIsValid(rnode.tblNode) &&
!                      bufHdr->tag.rnode.relNode == rnode.relNode &&
!                      bufHdr->tag.rnode.dbNode == rnode.dbNode))
              {
                  bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);

There has got to be a better way than this.  In the first place the
code seems able to seize on the wrong buffer if it's not checking
all three fields; in the second place, if the weak matching is correct
here why is it not needed everyplace else?

What's the purpose of this?

            regards, tom lane

Hi Tom,

On Wed, 16 Jun 2004, Tom Lane wrote:

> I'm starting to review this patch, and almost immediately came across
> what seemed a spectacularly bad idea:
>
> *** src/backend/storage/buffer/bufmgr.c    8 May 2004 19:09:25 -0000    1.165
> --- src/backend/storage/buffer/bufmgr.c    26 May 2004 06:21:01 -0000
> ***************
> *** 1148,1152 ****
>           {
>               bufHdr = &LocalBufferDescriptors[i];
> !             if (RelFileNodeEquals(bufHdr->tag.rnode, rnode))
>               {
>                   bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
> --- 1148,1156 ----
>           {
>               bufHdr = &LocalBufferDescriptors[i];
> !             /* special case for default tblNode */
> !             if (RelFileNodeEquals(bufHdr->tag.rnode, rnode) ||
> !                     (!OidIsValid(rnode.tblNode) &&
> !                      bufHdr->tag.rnode.relNode == rnode.relNode &&
> !                      bufHdr->tag.rnode.dbNode == rnode.dbNode))
>               {
>                   bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
>
> There has got to be a better way than this.  In the first place the
> code seems able to seize on the wrong buffer if it's not checking
> all three fields; in the second place, if the weak matching is correct
> here why is it not needed everyplace else?
>

Ahh. This is a hang over from some tests I was doing. I must have missed
it when I send the patch in. The patch should certainly work without this
change. I will verify later today when I have access to my development
machine.

Gavin

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

Bruce Momjian wrote:
> 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.

I have a few other questions:

What is the procedure for moving tablespace directories?  I assume with
the postmaster down the directory can be moved and the symlink changed.
However, pg_tablespace still has the old location.  Should we use lstat
so pg_tablespace gets updated automatically or as part of pg_dump, or
throw a server message if the symlink doesn't match pg_tablespace.  We
need to add instructions that pg_tablespace needs to be updated if the
symlink is changed.  What bothers me is that someone updating just the
symlink might run fine but would not be able to restore a dump to the
same machine.

And about restore, particularly to another machine, what do we do if the
tablespace can't be created in the location specified in the dump?  The
tablespace creation fails, and all objects specified in that tablespace
also fail?  Seems bad, particularly if you are restoring after a
hardware failure.  Do we need a GUC that says "if the tablespace doesn't
exist, create the object in the default location?"   Do we need a
pg_dump option that ignores tablespaces completely for portability and
for restoring to another server?

Is pg_dump smart enough not to emit the tablespace if the object would
already be created in the right tablespace, perhaps because of its
schema?  The new tablespace clause adds a non-standard clause to CREATE
TABLE, something we were hoping to avoid, but I doubt it is possible.

Do we need ALTER TABLESPACE to move tablespaces, and ALTER clauses to
move objects to other tablespaces?  Are these TODO items for later?

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

I wrote the pg_dump bits, so I guess I can answer these...

> And about restore, particularly to another machine, what do we do if the
> tablespace can't be created in the location specified in the dump?  The
> tablespace creation fails, and all objects specified in that tablespace
> also fail?  Seems bad, particularly if you are restoring after a
> hardware failure.  Do we need a GUC that says "if the tablespace doesn't
> exist, create the object in the default location?"   Do we need a
> pg_dump option that ignores tablespaces completely for portability and
> for restoring to another server?

Weeeell.  There's a lot of stuff that can already fail in a restore -
you always must watch your restore output to ensure things have worked.
  When I restore and it can't create a tsearch function because I forgot
to install the tsearch.so, it errors and then keeps going with tables
and triggers and all sorts of stuff failing to restore because they
depended on that function.  I don't see the failure to create a
tablespace as being any different.

> Is pg_dump smart enough not to emit the tablespace if the object would
> already be created in the right tablespace, perhaps because of its
> schema?

Yes it is, I was very careful about that.  If you never use a tablespace
in your life, you will never see tablespace commands in your dumps.  The
changes to pg_dump work just fine against a pre-7.5 backend and again no
tablespace commands will be output.

> The new tablespace clause adds a non-standard clause to CREATE
> TABLE, something we were hoping to avoid, but I doubt it is possible.

Well, if you don't use them, you won't see them, so I don't have a
problem with that :)

> Do we need ALTER TABLESPACE to move tablespaces, and ALTER clauses to
> move objects to other tablespaces?  Are these TODO items for later?

Would be nice :)  I'm also in favour of "ADD [PRIMARY KEY | UNIQUE
](blah) TABLESPACE asdf" which isn't in the current patch.

Chris

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> OK, I have reviewed the patch.  I think Tom is doing the same, but I
> want to report the things I found.

I just came up for air after about two solid days of working on this
patch ... had not seen your message before committing it.  The good
news is that I think I did see all the stuff you found.

> What facility is there for moving objects between tablespaces?

None, as yet.

> Seems we should be consistent in having WIN32 defs or not.

Probably.  I removed #ifdefs whereever possible --- there are just a few
left in tablespace.c and dbcommands.c now.  I was contemplating
replacing HAVE_SYMLINKS with a HAVE_TABLESPACES flag, but with the
occurrences isolated to one file I'm not sure it's worth the trouble.

> Your code in tablespc.c calls realpath().  Do all OS's have that?

It doesn't anymore --- I was concerned about the portability question
too.  The only point of that code AFAICS was to prevent creation of
two pg_tablespace entries pointing at the same directory.  I felt that
the better way to handle this was to write a PG_VERSION file in the
tablespace directory during CREATE TABLESPACE.  A subsequent CREATE
TABLESPACE on the same directory will fail because the directory isn't
empty anymore.  And the version file might come in handy someday...

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

I hacked up some logic to deal with this, based on taking out an
ExclusiveLock on pg_tablespace when adding a per-database subdirectory
to a tablespace directory or doing DROP TABLESPACE.  It works but it'd
be nice to reduce the strength of the lock ...

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

s/interesting/doesn't work/ ... I didn't commit this stuff.  Nor the
smgr changes either; those were far from ready for prime time.

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

Not done yet, but it's dead and should be removed as soon as a
decent respect for the deceased permits ;-)

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

Yeah.  The patch as committed covers the reference pages, but we
desperately need a higher-level discussion of tablespaces for the
administrator's guide.

            regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I have a few other questions:

> What is the procedure for moving tablespace directories?

There is none.

> However, pg_tablespace still has the old location.

Yup.  The *only* thing that pg_tablespace.spclocation is used for is
for pg_dumpall to dump appropriate CREATE TABLESPACE commands, so it's
not like you couldn't hack it after the fact.

> Do we need ALTER TABLESPACE to move tablespaces, and ALTER clauses to
> move objects to other tablespaces?  Are these TODO items for later?

TODO.  You sound like a man who's expecting a
several-generations-polished facility when we only just committed
the first version today.  I do not feel a need to have any of these
features in 7.5 ...

            regards, tom lane

On Fri, 18 Jun 2004, Tom Lane wrote:

> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > OK, I have reviewed the patch.  I think Tom is doing the same, but I
> > want to report the things I found.
>
> I just came up for air after about two solid days of working on this
> patch ... had not seen your message before committing it.  The good
> news is that I think I did see all the stuff you found.

Awesome.

>
> > What facility is there for moving objects between tablespaces?
>
> None, as yet.
>
> > Seems we should be consistent in having WIN32 defs or not.
>
> Probably.  I removed #ifdefs whereever possible --- there are just a few
> left in tablespace.c and dbcommands.c now.  I was contemplating
> replacing HAVE_SYMLINKS with a HAVE_TABLESPACES flag, but with the
> occurrences isolated to one file I'm not sure it's worth the trouble.
>
> > Your code in tablespc.c calls realpath().  Do all OS's have that?
>
> It doesn't anymore --- I was concerned about the portability question
> too.  The only point of that code AFAICS was to prevent creation of
> two pg_tablespace entries pointing at the same directory.  I felt that
> the better way to handle this was to write a PG_VERSION file in the
> tablespace directory during CREATE TABLESPACE.  A subsequent CREATE
> TABLESPACE on the same directory will fail because the directory isn't
> empty anymore.  And the version file might come in handy someday...

Yes. That's a better idea.

[snip]

> > 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.
>
> Yeah.  The patch as committed covers the reference pages, but we
> desperately need a higher-level discussion of tablespaces for the
> administrator's guide.

I'll look at this tomorrow.

Thanks for your assistance.

>
>             regards, tom lane
>

Gavin

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I have a few other questions:
>
> > What is the procedure for moving tablespace directories?
>
> There is none.
>
> > However, pg_tablespace still has the old location.
>
> Yup.  The *only* thing that pg_tablespace.spclocation is used for is
> for pg_dumpall to dump appropriate CREATE TABLESPACE commands, so it's
> not like you couldn't hack it after the fact.
>
> > Do we need ALTER TABLESPACE to move tablespaces, and ALTER clauses to
> > move objects to other tablespaces?  Are these TODO items for later?
>
> TODO.  You sound like a man who's expecting a
> several-generations-polished facility when we only just committed
> the first version today.  I do not feel a need to have any of these
> features in 7.5 ...

I just need to know what to add to the TODO list, and so we can answer
people who are going to ask for this functionality.  Added to TODO:

    * Allow reporting of which objects are in which tablespaces
    * Allow database recovery where tablespaces can't be created
        o Add ALTER TABLESPACE to change location, name, owner
        o Allow objects to be moved between tablespaces

I think this all the items still needed.

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

Tom Lane wrote:
> > Are we ripping out our initlocation code at the same time?
>
> Not done yet, but it's dead and should be removed as soon as a
> decent respect for the deceased permits ;-)

You want me to do the honors?

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>>> Are we ripping out our initlocation code at the same time?
>>
>> Not done yet, but it's dead and should be removed as soon as a
>> decent respect for the deceased permits ;-)

> You want me to do the honors?

Nah, I'll get it.  I want to do some other small cleanup on that patch,
too.  (But Gavin, you're on the hook for a rewrite of the admin guide
section about alternate locations into something about tablespaces...)

Somebody's got to fix oid2name and dbsize though.  Bruce, you want
to catch those?

            regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Tom Lane wrote:
> >>> Are we ripping out our initlocation code at the same time?
> >>
> >> Not done yet, but it's dead and should be removed as soon as a
> >> decent respect for the deceased permits ;-)
>
> > You want me to do the honors?
>
> Nah, I'll get it.  I want to do some other small cleanup on that patch,
> too.  (But Gavin, you're on the hook for a rewrite of the admin guide
> section about alternate locations into something about tablespaces...)
>
> Somebody's got to fix oid2name and dbsize though.  Bruce, you want
> to catch those?

Uh, how do they have to be fixed?  Isn't the relfilenode unchanged?  Do
we just need to add tablespace lookups?

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> Somebody's got to fix oid2name and dbsize though.  Bruce, you want
>> to catch those?

> Uh, how do they have to be fixed?  Isn't the relfilenode unchanged?  Do
> we just need to add tablespace lookups?

How useful will oid2name be if it doesn't understand about tablespaces?
I dunno how it ought to be changed, but surely it needs some thought.

dbsize doesn't even compile right now, because it's using
GetDatabasePath which now has another argument.  I did not patch it
because it needs more thought: should it report the total of all
tablespaces for the database, or should its API be extended so you
can ask about individual tablespaces, or what?  In any case it's
not a one-liner fix...

            regards, tom lane

On Fri, 18 Jun 2004, Tom Lane wrote:

> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Tom Lane wrote:
> >>> Are we ripping out our initlocation code at the same time?
> >>
> >> Not done yet, but it's dead and should be removed as soon as a
> >> decent respect for the deceased permits ;-)
>
> > You want me to do the honors?
>
> Nah, I'll get it.  I want to do some other small cleanup on that patch,
> too.  (But Gavin, you're on the hook for a rewrite of the admin guide
> section about alternate locations into something about tablespaces...)

I can either replace the "Alternative Locations" section or make a higher
level reference to tablespaces under Server Administration in the main
index. What do people think?

Gavin

On Fri, 18 Jun 2004, Bruce Momjian wrote:

[snip]

> > TODO.  You sound like a man who's expecting a
> > several-generations-polished facility when we only just committed
> > the first version today.  I do not feel a need to have any of these
> > features in 7.5 ...
>
> I just need to know what to add to the TODO list, and so we can answer
> people who are going to ask for this functionality.  Added to TODO:
>
>     * Allow reporting of which objects are in which tablespaces

Do we need an information_schema.tablespaces view as well as an update to
information_schema.{tables|indexes|...} ?

Gavin

Gavin Sherry wrote:

>On Fri, 18 Jun 2004, Bruce Momjian wrote:
>
>[snip]
>
>
>
>>>TODO.  You sound like a man who's expecting a
>>>several-generations-polished facility when we only just committed
>>>the first version today.  I do not feel a need to have any of these
>>>features in 7.5 ...
>>>
>>>
>>I just need to know what to add to the TODO list, and so we can answer
>>people who are going to ask for this functionality.  Added to TODO:
>>
>>    * Allow reporting of which objects are in which tablespaces
>>
>>
>
>Do we need an information_schema.tablespaces view as well as an update to
>information_schema.{tables|indexes|...} ?
>

I checked this to implement it, and found it being less then trivial
when *all* objects of a tablespace should be displayed, not just the
ones in the current database.

There seem to be several ways to implement:
(1) dblink like access iterating all databases. This seems
extraordinarily expensive
(2) low level scanning tablespace location, then try to translate found
oids to names withoug SPI. Probably not desirable.
(3) extending parser/executor to allow a table specification of the form
database.namespace.tablename. This has been requested several times
before, but was rejected, with the advise to use dblink.
(4) Copy the contents of the desired cross-db table to a temporary table
(generate new oid in local db, copy cross-db table file to new oid file,
insert pg_class). A typically weird Andreas' approach :-)

Regards,
Andreas

On Fri, 18 Jun 2004, Andreas Pflug wrote:

> Gavin Sherry wrote:
>
> >On Fri, 18 Jun 2004, Bruce Momjian wrote:
> >
> >[snip]
> >
> >
> >
> >>>TODO.  You sound like a man who's expecting a
> >>>several-generations-polished facility when we only just committed
> >>>the first version today.  I do not feel a need to have any of these
> >>>features in 7.5 ...
> >>>
> >>>
> >>I just need to know what to add to the TODO list, and so we can answer
> >>people who are going to ask for this functionality.  Added to TODO:
> >>
> >>    * Allow reporting of which objects are in which tablespaces
> >>
> >>
> >
> >Do we need an information_schema.tablespaces view as well as an update to
> >information_schema.{tables|indexes|...} ?
> >
>
> I checked this to implement it, and found it being less then trivial
> when *all* objects of a tablespace should be displayed, not just the
> ones in the current database.

I don't think we should try and show all objects for a tablespace in
information_schema. That, as you point out, is hard and possibly gets
users into some messy situations.

Being able to list all objects in a tablespace, including which databases
they are in, is clearly useful, however (eg: hunting down use of a give
tablespace that you want dropped). Sounds like a script in contrib (or the
main source tree?) to me.

Gavin

Gavin Sherry <swm@linuxworld.com.au> writes:
> On Fri, 18 Jun 2004, Bruce Momjian wrote:
>> * Allow reporting of which objects are in which tablespaces

> Do we need an information_schema.tablespaces view as well as an update to
> information_schema.{tables|indexes|...} ?

That would depend on your theology about whether implementation-specific
additions to information_schema are a good idea or not.  I'd lean
against doing this myself.  ISTM the entire point of information_schema
is to be standard, and additions are, well, not.  Worse, they might
conflict with future extensions to the standard.

I don't actually see a way *inside the database* to implement Bruce's
TODO item; there's no way for a backend to look at the catalogs of other
databases.  We could look to see which databases had nonempty
subdirectories of a tablespace, and report those databases by name, but
explaining exactly what's inside those subdirectories is a lot harder.

It might be better to think about supporting this need with oid2name
or some similar client-level tool.  It's easy to see how oid2name
might be extended to produce an output like

    Tablespace t1
        Database d1
            Table x
            Table y
        Database d2
    etc etc

            regards, tom lane

Gavin Sherry wrote:

>On Fri, 18 Jun 2004, Andreas Pflug wrote:
>
>
>
>>Gavin Sherry wrote:
>>
>>
>>
>>>On Fri, 18 Jun 2004, Bruce Momjian wrote:
>>>
>>>[snip]
>>>
>>>
>>>
>>>
>>>
>>>>>TODO.  You sound like a man who's expecting a
>>>>>several-generations-polished facility when we only just committed
>>>>>the first version today.  I do not feel a need to have any of these
>>>>>features in 7.5 ...
>>>>>
>>>>>
>>>>>
>>>>>
>>>>I just need to know what to add to the TODO list, and so we can answer
>>>>people who are going to ask for this functionality.  Added to TODO:
>>>>
>>>>    * Allow reporting of which objects are in which tablespaces
>>>>
>>>>
>>>>
>>>>
>>>Do we need an information_schema.tablespaces view as well as an update to
>>>information_schema.{tables|indexes|...} ?
>>>
>>>
>>>
>>I checked this to implement it, and found it being less then trivial
>>when *all* objects of a tablespace should be displayed, not just the
>>ones in the current database.
>>
>>
>
>I don't think we should try and show all objects for a tablespace in
>information_schema.
>
Agreed, information_schema is database specific. I was thinking about a
pg_tablespace_contents(..) function anyway.

>Being able to list all objects in a tablespace, including which databases
>they are in, is clearly useful, however (eg: hunting down use of a give
>tablespace that you want dropped). Sounds like a script in contrib (or the
>main source tree?) to me.
>
>
You're suggesting the dblink way using a shell script. Imagine 20, 200,
... databases. This would be a costly thing (and has to be  implemented
differently in win32).
I'd like to see an implementation that enables gui interfaces to show
objects that depend on a tablespace, so you'd need to be aware of a user
clicking on "show what's in that tablespace" and he probably wouldn't
expect to wait an extended period of time for all databases to be
scanned, or impose a 200-connection load on the server.

IMHO checking objects in a tablespace is a routine administrative task,
so it should be supported natively by the server without need of
contribs. And for win user acceptance, a command line tool won't be
sufficient either.

Regards,
Andreas

On Sat, 19 Jun 2004, Andreas Pflug wrote:

[snip]

> >I don't think we should try and show all objects for a tablespace in
> >information_schema.
> >
> Agreed, information_schema is database specific. I was thinking about a
> pg_tablespace_contents(..) function anyway.
>
> >Being able to list all objects in a tablespace, including which databases
> >they are in, is clearly useful, however (eg: hunting down use of a give
> >tablespace that you want dropped). Sounds like a script in contrib (or the
> >main source tree?) to me.
> >
> >
> You're suggesting the dblink way using a shell script. Imagine 20, 200,
> ... databases. This would be a costly thing (and has to be  implemented
> differently in win32).
> I'd like to see an implementation that enables gui interfaces to show
> objects that depend on a tablespace, so you'd need to be aware of a user
> clicking on "show what's in that tablespace" and he probably wouldn't
> expect to wait an extended period of time for all databases to be
> scanned, or impose a 200-connection load on the server.

I see this more as a script like Tom described in another email. We'd have
a list of tablespacecs and databases and scan each database (on connection
at a time) and get the information the user wants.

> IMHO checking objects in a tablespace is a routine administrative task,
> so it should be supported natively by the server without need of
> contribs. And for win user acceptance, a command line tool won't be
> sufficient either.

I would debate that.

Firstly, tablespaces aren't supported on windows yet. Secondly, I'd think
that Unix users would be fine with a command line tool, especially one
that can connect to a remote host.

For those not used to command line tools, I can imagine extensions to
pgadmin or phppgadmin.

Gavin

Gavin Sherry wrote:

>I would debate that.
>
>Firstly, tablespaces aren't supported on windows yet.
>
Just a matter of time. And I'm talking of win32 workstations connecting
to *ix servers too.

> Secondly, I'd think
>that Unix users would be fine with a command line tool, especially one
>that can connect to a remote host.
>
>For those not used to command line tools, I can imagine extensions to
>pgadmin or phppgadmin.
>
>
>

:-) :-) :-)

Unfortunately, us admin tool programmers can't practice witchcraft, so
we need a pgsql function for that...
Certainly, we could iterate all known databases making a one-time
connection (if allowed to connect, what about template0?), retrieving
tablespace dependencies, and close again. As debated above, that's quite
costly, especially if more sophisticated authentication mechanisms are used.


Regards,
Andreas

Andreas Pflug <pgadmin@pse-consulting.de> writes:
> IMHO checking objects in a tablespace is a routine administrative task,
> so it should be supported natively by the server without need of
> contribs.

I strongly disagree.  Dropping a tablespace is not a routine activity,
and we don't have to have submillisecond response to operations that
are only needed when your first attempt to drop one fails.

As for the authentication-is-expensive issue, what of it?  You *should*
have to authenticate yourself in order to look inside another person's
database.  The sort of cross-database inspection being proposed here
would be a big security hole in many people's view.

> And for win user acceptance, a command line tool won't be
> sufficient either.

This does not deserve a response.

            regards, tom lane

Tom Lane wrote:
> Gavin Sherry <swm@linuxworld.com.au> writes:
> > On Fri, 18 Jun 2004, Bruce Momjian wrote:
> >> * Allow reporting of which objects are in which tablespaces

This would have to be an external tool that connects to each database.

>
> > Do we need an information_schema.tablespaces view as well as an update to
> > information_schema.{tables|indexes|...} ?
>
> That would depend on your theology about whether implementation-specific
> additions to information_schema are a good idea or not.  I'd lean
> against doing this myself.  ISTM the entire point of information_schema
> is to be standard, and additions are, well, not.  Worse, they might
> conflict with future extensions to the standard.
>
> I don't actually see a way *inside the database* to implement Bruce's
> TODO item; there's no way for a backend to look at the catalogs of other
> databases.  We could look to see which databases had nonempty
> subdirectories of a tablespace, and report those databases by name, but
> explaining exactly what's inside those subdirectories is a lot harder.

The best we could do here is to just report the database name and the
relfilenode.


>
> It might be better to think about supporting this need with oid2name
> or some similar client-level tool.  It's easy to see how oid2name
> might be extended to produce an output like
>
>     Tablespace t1
>         Database d1
>             Table x
>             Table y
>         Database d2
>     etc etc

Yep, that's what I was thinking --- an external tool.

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

Tom Lane wrote:

>Andreas Pflug <pgadmin@pse-consulting.de> writes:
>
>
>>IMHO checking objects in a tablespace is a routine administrative task,
>>so it should be supported natively by the server without need of
>>contribs.
>>
>>
>
>I strongly disagree.  Dropping a tablespace is not a routine activity,
>
>
Dropping certainly not. But inspecting? If implemented in a gui, it's
just a click away.

>
>As for the authentication-is-expensive issue, what of it?  You *should*
>have to authenticate yourself in order to look inside another person's
>database.  The sort of cross-database inspection being proposed here
>would be a big security hole in many people's view.
>
>
Accessing pg_class et al using the current sysuseid with acl checking
should be ok and satisfy security demands, no? Since it's the same
cluster, we can be sure that it 's the same user in that cross db too.
If the user has no access, the result won't have a meaning either.

The auth-is-expensive issue is about creating the db connection itself
again and again, when we only want to change a database.

>>And for win user acceptance, a command line tool won't be
>>sufficient either.
>>
>>
>
>This does not deserve a response.
>
>

Well, that's not quite appropriate. A 'command line is enough for server
maintenance' attitude won't attract win people; they're blind without a
mouse.

Regards,
Andreas

Andreas Pflug <pgadmin@pse-consulting.de> writes:
> Tom Lane wrote:
>> As for the authentication-is-expensive issue, what of it?  You *should*
>> have to authenticate yourself in order to look inside another person's
>> database.  The sort of cross-database inspection being proposed here
>> would be a big security hole in many people's view.
>>
> Accessing pg_class et al using the current sysuseid with acl checking
> should be ok and satisfy security demands, no?

No.  If the other user has you locked out from connecting to his
database at all, he's probably not going to feel that he should have to
disable your access to individual objects inside it.

This has some connections to the discussions we periodically have about
preventing Joe User from looking at the system catalogs.  If we make any
changes in this area at all, I would expect them to be in the direction
of narrowing access, not widening it to include being able to see
other databases' catalogs.

            regards, tom lane

Andreas Pflug wrote:
> >>And for win user acceptance, a command line tool won't be
> >>sufficient either.
> >>
> >>
> >
> >This does not deserve a response.
> >
> >
>
> Well, that's not quite appropriate. A 'command line is enough for server
> maintenance' attitude won't attract win people; they're blind without a
> mouse.

We can build a gui on top of the command-line tool, no?

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

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Tom Lane wrote:
> >> Somebody's got to fix oid2name and dbsize though.  Bruce, you want
> >> to catch those?
>
> > Uh, how do they have to be fixed?  Isn't the relfilenode unchanged?  Do
> > we just need to add tablespace lookups?
>
> How useful will oid2name be if it doesn't understand about tablespaces?
> I dunno how it ought to be changed, but surely it needs some thought.

Well, I figure we would copy the database capability we have for
tablespaces.

If you call oid2name with no args, you get:

    All databases:
    ---------------------------------
    17219  = test
    1      = template1
    17218  = template0

If we specify just the database name we get:

    (2) aspg oid2name -d template1
    All tables from database "template1":
    ---------------------------------
    17147  = sql_features
    17152  = sql_implementation_info
    17157  = sql_languages
    17162  = sql_packages
    17167  = sql_sizing
    17172  = sql_sizing_profiles

I assume we just need to add a tablespace display when run with no args,
and a -s option to display _with_ -d to display only objects in that
database.  We could go fancy and spin through all the databases and list
the datbase name and objects in that tablespace.

> dbsize doesn't even compile right now, because it's using
> GetDatabasePath which now has another argument.  I did not patch it
> because it needs more thought: should it report the total of all
> tablespaces for the database, or should its API be extended so you
> can ask about individual tablespaces, or what?  In any case it's
> not a one-liner fix...

For dbsize, I assume we have to follow the symlinks.  We would have to
spin through all the tablespaces looking for directories with the
database oid.

Given the number of open items for 7.5, I am thinking of keeping this
for post-feature freeze.  Both are contrib.

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> How useful will oid2name be if it doesn't understand about tablespaces?
>> I dunno how it ought to be changed, but surely it needs some thought.

> I assume we just need to add a tablespace display when run with no args,
> and a -s option to display _with_ -d to display only objects in that
> database.  We could go fancy and spin through all the databases and list
> the datbase name and objects in that tablespace.

I should think that the table-level display ought to show both the
relfilenode and tablespace OIDs for each table.

> Given the number of open items for 7.5, I am thinking of keeping this
> for post-feature freeze.  Both are contrib.

Right, I doubt Marc will object to fixing contrib stuff after feature
freeze ...

            regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Tom Lane wrote:
> >> How useful will oid2name be if it doesn't understand about tablespaces?
> >> I dunno how it ought to be changed, but surely it needs some thought.
>
> > I assume we just need to add a tablespace display when run with no args,
> > and a -s option to display _with_ -d to display only objects in that
> > database.  We could go fancy and spin through all the databases and list
> > the datbase name and objects in that tablespace.
>
> I should think that the table-level display ought to show both the
> relfilenode and tablespace OIDs for each table.

This is the existing display:

    (3) aspg oid2name -d test
    All tables from database "test":
    ---------------------------------
    17147  = sql_features
    17152  = sql_implementation_info
    17157  = sql_languages
    17162  = sql_packages
    17167  = sql_sizing
    17172  = sql_sizing_profiles
    17220  = x

For objects in the default tablespace, they don't show a tablespace oid,
right?  Where do we put it?  A column that will be empty if they don't
use tablespaces?

> > Given the number of open items for 7.5, I am thinking of keeping this
> > for post-feature freeze.  Both are contrib.
>
> Right, I doubt Marc will object to fixing contrib stuff after feature
> freeze ...

Also, remember I am only online fulltime for another two days, then I am
leaving for Europe, return on July 3, after feature freeze.

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

>>>Are we ripping out our initlocation code at the same time?
>>
>>Not done yet, but it's dead and should be removed as soon as a
>>decent respect for the deceased permits ;-)
>
>
> You want me to do the honors?

What about people upgrading from 7.4 databases that used database locations?

Chris

Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
> What about people upgrading from 7.4 databases that used database locations?

They'll get a nice warning:

regression=# create database foo location 'bar';
WARNING:  LOCATION is not supported anymore
HINT:  Consider using tablespaces instead.
CREATE DATABASE

and everything will go into the default tablespace.  I don't really
see how to do much better than that ...

            regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> I should think that the table-level display ought to show both the
>> relfilenode and tablespace OIDs for each table.

> For objects in the default tablespace, they don't show a tablespace oid,
> right?  Where do we put it?  A column that will be empty if they don't
> use tablespaces?

pg_class will show a zero for objects in the default tablespace, but
I think oid2name should pull the actual tablespace ID from
pg_database.dattablespace and show that.  The convention about zero
is just to make life simple for CREATE DATABASE --- users of oid2name
should not have to think about it.

            regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Tom Lane wrote:
> >> I should think that the table-level display ought to show both the
> >> relfilenode and tablespace OIDs for each table.
>
> > For objects in the default tablespace, they don't show a tablespace oid,
> > right?  Where do we put it?  A column that will be empty if they don't
> > use tablespaces?
>
> pg_class will show a zero for objects in the default tablespace, but
> I think oid2name should pull the actual tablespace ID from
> pg_database.dattablespace and show that.  The convention about zero
> is just to make life simple for CREATE DATABASE --- users of oid2name
> should not have to think about it.

Well, I didn't use tablespaces here so the pg_tablespaces directory is
empty, so I can't think of what the tablespace is.  Is it the database
oid?   Also, are we calling it pg_tablespaces (plural) rather than
pg_tablespace?

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Well, I didn't use tablespaces here so the pg_tablespaces directory is
> empty, so I can't think of what the tablespace is.

You look in the pg_tablespace catalog for the row with that OID.

> Also, are we calling it pg_tablespaces (plural) rather than
> pg_tablespace?

I didn't have any particular opinion about that till just now ...
but now I see that it's a good idea for the pg_tablespaces directory
(the one that holds all the symlinks) to have a different name from the
pg_tablespace catalog, especially since the latter has a couple of rows
that do not correspond to any entries in the former.

I'm not wedded to "pg_tablespaces" as the name in particular, but
it should not be "pg_tablespace", or we'll suffer the same confusion
over and over that you just did.

            regards, tom lane

> regression=# create database foo location 'bar';
> WARNING:  LOCATION is not supported anymore
> HINT:  Consider using tablespaces instead.
> CREATE DATABASE
>
> and everything will go into the default tablespace.  I don't really
> see how to do much better than that ...

You could create an automatically named tablespace instead of the
location...

By the way, I think that we should deny users the ability to create
tablespaces that begin with pg_.  Also, the existing ones should be
pg_global and pg_default.  That way, we have room to move if ever we
decide we want more system tablespaces.  It also makes it easier to dump
non-system tablespaces.

Chris

Tom Lane wrote:

>Andreas Pflug <pgadmin@pse-consulting.de> writes:
>
>
>>Tom Lane wrote:
>>
>>
>>>As for the authentication-is-expensive issue, what of it?  You *should*
>>>have to authenticate yourself in order to look inside another person's
>>>database.  The sort of cross-database inspection being proposed here
>>>would be a big security hole in many people's view.
>>>
>>>
>>>
>>Accessing pg_class et al using the current sysuseid with acl checking
>>should be ok and satisfy security demands, no?
>>
>>
>
>No.  If the other user has you locked out from connecting to his
>database at all, he's probably not going to feel that he should have to
>disable your access to individual objects inside it.
>
>
Well he's using my tablespace, so I'd like to know at least the object name.

>This has some connections to the discussions we periodically have about
>preventing Joe User from looking at the system catalogs.  If we make any
>changes in this area at all, I would expect them to be in the direction
>of narrowing access, not widening it to include being able to see
>other databases' catalogs.
>
>
Superuser/tablespace owner isn't quite Joe User, I believe.

Actually, there seem quite some other cross database/shared table issues
(schema default tablespace, dropping user who owns objects) which make
it desirable to have superuser readonly access to pg_catalog tables.
Maybe a todo for 7.6...

Regards,
Andreas

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Well, I didn't use tablespaces here so the pg_tablespaces directory is
> > empty, so I can't think of what the tablespace is.
>
> You look in the pg_tablespace catalog for the row with that OID.
>
> > Also, are we calling it pg_tablespaces (plural) rather than
> > pg_tablespace?
>
> I didn't have any particular opinion about that till just now ...
> but now I see that it's a good idea for the pg_tablespaces directory
> (the one that holds all the symlinks) to have a different name from the
> pg_tablespace catalog, especially since the latter has a couple of rows
> that do not correspond to any entries in the former.

If you want something distinct, which I understand, perhaps pg_tblspc.

> I'm not wedded to "pg_tablespaces" as the name in particular, but
> it should not be "pg_tablespace", or we'll suffer the same confusion
> over and over that you just did.

OK.

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

On Thu, 2004-05-27 at 07:59, Gavin Sherry wrote:
> Attached is my latest patch implementing tablespaces. This has all the
> functionality I was planning for 7.5.
>
> Most of the information about the patch is contained in the
> patch/documentation, previous submissions and the archives.
>
> Testing, review, comments would be greatly appreciated.
>

I've reviewed your patch by eye, but can't see anything in your patch
about relocating the pg_xlog directory.

There was/is a TODO item:
- Allow xlog directory location to be specified during initdb, perhaps
using symlinks

Is that something you've addressed in your patch?

If not, is that something that would be easily do-able (before freeze)?

AFAICS, pg_xlog could be treated as the name of a tablespace, rather
than as a fixed directory beneath PGDATA.

pg_xlog is only referred to in 4 lines in the code (incl. PITR patch):
- xlog.c
- pgarch.c (PITR patch)
- initdb.c
- pgresetxlog.c
Each time it is simply setting a string to the location of the xlog
directory.

If we could work out a way of...
i) letting the pg_xlog be created by default
ii) then transferring this to another tablespace later?
That would give us maximum flexibility, since you may wish to change
location later when workload changes/increases.

Perhaps adding a GUC...for wal_tablespace (pls suggest name!)
defaults to the pg_xlog directory, when not listed?
Changeable only at postmaster startup...

This could be done independently of tablespaces, but I think any
directory flexibility/change should work using the tablespace
infrastructure, not in addition to it.

Could we discuss this? it sounds like a change we could make happen
fairly quickly now your code is in place.

Of course, I accept that many may say that such a change is not really
needed, but then...

Best regards, Simon Riggs

On Sun, 20 Jun 2004, Simon Riggs wrote:

> On Thu, 2004-05-27 at 07:59, Gavin Sherry wrote:
> > Attached is my latest patch implementing tablespaces. This has all the
> > functionality I was planning for 7.5.
> >
> > Most of the information about the patch is contained in the
> > patch/documentation, previous submissions and the archives.
> >
> > Testing, review, comments would be greatly appreciated.
> >
>
> I've reviewed your patch by eye, but can't see anything in your patch
> about relocating the pg_xlog directory.

I didn't intend on looking at that in this patch.

> pg_xlog is only referred to in 4 lines in the code (incl. PITR patch):
> - xlog.c
> - pgarch.c (PITR patch)
> - initdb.c
> - pgresetxlog.c
> Each time it is simply setting a string to the location of the xlog
> directory.
>
> If we could work out a way of...
> i) letting the pg_xlog be created by default
> ii) then transferring this to another tablespace later?
> That would give us maximum flexibility, since you may wish to change
> location later when workload changes/increases.

Sounds reasonable.

>
> Perhaps adding a GUC...for wal_tablespace (pls suggest name!)
> defaults to the pg_xlog directory, when not listed?
> Changeable only at postmaster startup...
>
> This could be done independently of tablespaces, but I think any
> directory flexibility/change should work using the tablespace
> infrastructure, not in addition to it.

If the change is as simple as you propose, it has nothing to do with
the tablespace code. Also, I don't see any situation where we would want
to make use of the tablespace code.

> Could we discuss this? it sounds like a change we could make happen
> fairly quickly now your code is in place.

Again, I don't think my code really has any affect on the location of
xlog.

>
> Of course, I accept that many may say that such a change is not really
> needed, but then...

Comments anyone?

Gavin

Simon Riggs <simon@2ndquadrant.com> writes:
> I've reviewed your patch by eye, but can't see anything in your patch
> about relocating the pg_xlog directory.

I see no reasonable way to move pg_xlog while the postmaster is up.
So this is something that will always require outside-the-database
activity.  We could maybe offer a script to do it, but I can't see
any fundamental advantage there over telling people how to do it by
hand.

> There was/is a TODO item:
> - Allow xlog directory location to be specified during initdb, perhaps
> using symlinks

Note this says "during initdb", which is a whole different ballgame.
We could certainly add a "symlink pg_xlog to foo" switch to initdb,
but again, just how much functionality is that really adding?
Not a whole lot.  (I'll not stand in the way if someone wants to write
and document such a switch in the next ten days; but really the bang
for buck ratio seems low, compared to working on things that you can't
do nohow in current PG.)

> AFAICS, pg_xlog could be treated as the name of a tablespace, rather
> than as a fixed directory beneath PGDATA.

Definitely not.  pg_xlog is *not* a tablespace.  xlog activity happens
below the level at which it is reasonable to pay attention to tablespaces.

            regards, tom lane

On Sun, 2004-06-20 at 22:28, Tom Lane wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:

> > There was/is a TODO item:
> > - Allow xlog directory location to be specified during initdb, perhaps
> > using symlinks
>
> Note this says "during initdb", which is a whole different ballgame.
> We could certainly add a "symlink pg_xlog to foo" switch to initdb,
> but again, just how much functionality is that really adding?
> Not a whole lot.  (I'll not stand in the way if someone wants to write
> and document such a switch in the next ten days; but really the bang
> for buck ratio seems low, compared to working on things that you can't
> do nohow in current PG.)
>

I think its less useful at initdb time....

We probably have time for a change, but robustness makes me think not
to, especially as its gloss, not feature.

So, buried it is then.

Best Regards, Simon Riggs