Re: [HACKERS] Duplicate usage of tablespace location? - Mailing list pgsql-hackers

Hello,

At Fri, 5 May 2017 21:42:47 +1000, Neha Khatri <nehakhatri5@gmail.com> wrote in
<CAFO0U+8fHoVDCJDLQP+g9nKFhOyiDYnFvNMSNbjTJQFY+SjS+A@mail.gmail.com>
> As Kyotaro san pointed out, the commit 22817041 started allowing creation
> of multiple "tablespace version directories" in same location. However the
> original purpose of that commit was to allow that just for the upgrade
> purpose. So couple of points:
> - The commit violated the requirement of emptiness of the tablespace
> location directory.
>     (Though it is still prevented to create multiple tablespaces belonging
> to one server, in same location.)
> - The comment did not document this change in specification.
> 
> Probably it was not anticipated at that time that a user could create the
> tablespaces for different server version at the same location.
> 
> Now that this behaviour is present in field for a while, there is
> likelihood of having systems with tablespaces for two different versions,
> in same location. To avoid the problem reported in [1] for such systems,
> here are couple of alternative approaches:
> 
> 1. Allow creation of multiple tablespaces in single location for different
> server versions, but not the same version(exception).
> a) Also allow this capability in utilities like pg_basebackup( and others
> that update tablespaces) .
> b) Update the documentation about this specification change.
> 
> I don't see this breaking any backwards compatibility.

Yeah, it is just clarification of the behavior in the
documentation. The current behavior is somewhat inconsistent but
practical.

> 2. Retain the current base rule of creating Tablespaces i.e. "The location
> must be an existing, empty directory". This means:
> a) For the future release, have a strict directory emptiness check while
> creating new tablespace.
> b) Only during upgrade, allow creation of multiple tablepaces in same
> location .
> c) Document the fact that only during upgrade the system would create
> multiple tablespaces in same location.

Honestly saying, I think it adds nothing good other than seeming
consistency. (Even though I sent such a patch:p)

> d) Provide a flexibility to change the location of an existing tablespace,
> something like:
> ALTER TABLESPACE tblspcname SET LOCATION '/path/to/newlocation'
> [where newlocation is an existing empty direcotry]
> 
> With the altered location of a tablespace it should be possible to perform
> the pg_basebackup successfully.

If we can accept multiple server versions share a tablespace
directory, pg_basebackup also can allow that situation. The
attached patch does that. Similary to the server code, it
correctly fails if the same version subdirectory exists.

$ pg_basebackup -D $PGDATA -h /tmp -p 5432 -X stream -T /home/horiguti/data/tsp1=/home/horiguti/data/tsp2
pg_basebackup: could not create directory "/home/horiguti/data/tsp2/PG_10_201705091": File exists
pg_basebackup: removing contents of data directory "/home/horiguti/data/data_work_s"

> I noticed some solutions for moving PostgreSQL tablesspaces, on internet.
> But some are slow, others cause incompatibility for tools like pgAdmin. I
> am not able to find any discussion about moving tablespace location in
> mailing lists too. So I am not sure if there is already any conclusion
> about supporting or not supporting ALTER TABLESPACE LOCATION.
> To me, the first approach above looks like providing more independence to
> the user about choice of tablespace location. Also, it is not clear that
> why the directory emptiness rule was introduced in first place. Any insight
> on that will be useful.

Originally (before 9.0) files in a tablespace is directly placed
in the "location" and it is reasonable at that time.

> Regards,
> Neha
> 
> [1]https://www.postgresql.org/message-id/2008148.rxBNyNRHPZ@peanuts2
> 
> Cheers,
> Neha
> 
> On Fri, Apr 7, 2017 at 11:02 AM, Kyotaro HORIGUCHI <
> horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> 
> > I don't mean that this is the only or best way to go.
> >
> > I apologize for the possible lack of explanation.
> >
> > At Thu, 06 Apr 2017 12:03:51 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> > <21084.1491494631@sss.pgh.pa.us>
> > > Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> > > > I noticed by the following report, PostgreSQL can share the same
> > > > directory as tablespaces of two servers with different
> > > > pg-versions.
> > >
> > > > https://www.postgresql.org/message-id/2008148.rxBNyNRHPZ@peanuts2
> > >
> > > > 8.4 checked that the tablespace location is empty, but from 9.0,
> > > > the check is replaced with creating a PG_PGVER_CATVER
> > > > subdirectory. This works for multiple servers with the same
> > > > version, but don't for servers with different versions.
> > >
> > > Please explain why you think it doesn't work.  This patch seems to
> > > be reverting an intentional behavioral change, and you haven't
> >
> > I understand that the change is for in-place upgrade, not for
> > sharing a tablespace diretory between two version of PostgreSQL
> > servers. It actually rejects the second server with the same
> > version to come. If this is correct, it doesn't seem right to
> > accept the second server of the different version.
> >
> > If we allow sharing of the directory, theoretically we can allow
> > the same between the same version of servers by adding system
> > identifier in the subdirectory name.
> >
> >
> > > really explained why we'd want to.  It certainly doesn't look like
> > > it addresses the referenced complaint about pg_basebackup behavior.
> >
> > My point is that "the direcotry for newly created tablespace is
> > really reuiqred to be literary empty or not?"
> >
> > Practically it doesn't need to be empty and succesful creation of
> > PG_VER_CATVER directory is enough as the current implement
> > does. If we take this way the documentation and pg_basebackup
> > should be changed and the problem will be resolved as the result.
> >
> > https://www.postgresql.org/docs/9.6/static/manage-ag-tablespaces.html
> >
> > - The location must be an existing, empty directory that is owned
> > - by the PostgreSQL operating system user. All objects subsequently
> > - created within the tablespace will be stored in files underneath
> > - this directory.
> > + CREATE TABLESPACE creates a subdirectory named after server
> > + version in the location. The location must not contain a file
> > + or directory of that name for the subdirectory. All objects
> > + subsequently created within the tablespace will be stored in
> > + files underneath the subdirectory.
> >
> > Then, modify pg_basebackup to follow the description above.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index e2a2ebb..969b600 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -130,7 +130,7 @@ static PQExpBuffer recoveryconfcontents = NULL;/* Function headers */static void usage(void);static
voiddisconnect_and_exit(int code);
 
-static void verify_dir_is_empty_or_create(char *dirname, bool *created, bool *found);
+static void verify_and_create_dir(char *dirname, bool *created, bool *found, bool allow_nonempty);static void
progress_report(inttablespacenum, const char *filename, bool force);static void ReceiveTarFile(PGconn *conn, PGresult
*res,int rownum);
 
@@ -145,6 +145,8 @@ static bool reached_end_position(XLogRecPtr segendpos, uint32 timeline,static const char
*get_tablespace_mapping(constchar *dir);static void tablespace_list_append(const char *arg);
 
+#define verify_dir_is_empty_or_create(dirname, created, found) \
+    verify_and_create_dir(dirname, created, found, false)static voidcleanup_directories_atexit(void)
@@ -646,7 +648,8 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier) * be give and the process
ended.*/static void
 
-verify_dir_is_empty_or_create(char *dirname, bool *created, bool *found)
+verify_and_create_dir(char *dirname, bool *created, bool *found,
+                     bool allow_nonempty){    switch (pg_check_dir(dirname))    {
@@ -680,6 +683,9 @@ verify_dir_is_empty_or_create(char *dirname, bool *created, bool *found)            /*
*Exists, not empty             */
 
+            if (allow_nonempty)
+                return;
+            fprintf(stderr,                    _("%s: directory \"%s\" exists but is not empty\n"),
progname,dirname);
 
@@ -1854,7 +1860,7 @@ BaseBackup(void)        {            char       *path = (char *)
get_tablespace_mapping(PQgetvalue(res,i, 1));
 
-            verify_dir_is_empty_or_create(path, &made_tablespace_dirs, &found_tablespace_dirs);
+            verify_and_create_dir(path, &made_tablespace_dirs, &found_tablespace_dirs, true);        }    }