pgsql: Map basebackup tablespaces using a tablespace_map file

On 05/12/2015 04:42 PM, Andrew Dunstan wrote:

+
+               /*
+                * Remove the existing symlink if any and Create the symlink
+                * under PGDATA.  We need to use rmtree instead of rmdir as
+                * the link location might contain directories or files
+                * corresponding to the actual path. Some tar utilities do
+                * things that way while extracting symlinks.
+                */
+               if (lstat(linkloc, &st) == 0 && S_ISDIR(st.st_mode))
+               {
+                   if (!rmtree(linkloc,true))
+                       ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not remove directory \"%s\": %m",
+                                       linkloc)));
+               }
+               else
+               {
+                   if (unlink(linkloc) < 0 && errno != ENOENT)
+                       ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not remove symbolic link \"%s\": %m",
+                                       linkloc)));
+               }
+

That's scary. What tar utilitiy replaces the symlink with a non-empty
directory?

What if you call pg_start_backup() and take the backup with a utility
that follows symlinks? I wouldn't recommend using such a tool, but with
this patch, it will zap all the tablespaces. Before this, you at least
got a working database and could read out all the data or fix the
symlinks afterwards.

Why is the RelationCacheInitFileRemove() call moved?

XLogRecPtr
do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
-                  char **labelfile)
+                  char **labelfile, DIR *tblspcdir, List **tablespaces,
+                  char **tblspcmapfile, bool infotbssize,
+                  bool needtblspcmapfile)

Why does this need to take tblspcdir as argument? Wouldn't it make more
sense to open the directory inside do_pg_start_backup?

In the BASE_BACKUP command, is there any reason to not always include
the tablespace map? IOW, do we really need the TABLESPACE_MAP option?

- Heikki

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On 05/12/2015 10:33 AM, Heikki Linnakangas wrote:

On 05/12/2015 04:42 PM, Andrew Dunstan wrote:

+
+               /*
+                * Remove the existing symlink if any and Create the 
symlink
+                * under PGDATA.  We need to use rmtree instead of 
rmdir as
+                * the link location might contain directories or files
+                * corresponding to the actual path. Some tar 
utilities do
+                * things that way while extracting symlinks.
+                */
+               if (lstat(linkloc, &st) == 0 && S_ISDIR(st.st_mode))
+               {
+                   if (!rmtree(linkloc,true))
+                       ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not remove directory 
\"%s\": %m",
+                                       linkloc)));
+               }
+               else
+               {
+                   if (unlink(linkloc) < 0 && errno != ENOENT)
+                       ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not remove symbolic 
link \"%s\": %m",
+                                       linkloc)));
+               }
+

That's scary. What tar utilitiy replaces the symlink with a non-empty
directory?

What if you call pg_start_backup() and take the backup with a utility
that follows symlinks? I wouldn't recommend using such a tool, but
with this patch, it will zap all the tablespaces. Before this, you at
least got a working database and could read out all the data or fix
the symlinks afterwards.

Why is the RelationCacheInitFileRemove() call moved?

I will let Amit comment on this.

XLogRecPtr
do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID 
*starttli_p,
-                  char **labelfile)
+                  char **labelfile, DIR *tblspcdir, List **tablespaces,
+                  char **tblspcmapfile, bool infotbssize,
+                  bool needtblspcmapfile)

Why does this need to take tblspcdir as argument? Wouldn't it make
more sense to open the directory inside do_pg_start_backup?

Good point. It does look cleaner to do that.

In the BASE_BACKUP command, is there any reason to not always include
the tablespace map? IOW, do we really need the TABLESPACE_MAP option?

I assume it was done for minimal disturbance, i.e. that's where we need
it. Amit, comments?

cheers

andrew

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Tue, May 12, 2015 at 9:02 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 05/12/2015 10:33 AM, Heikki Linnakangas wrote:

On 05/12/2015 04:42 PM, Andrew Dunstan wrote:

+
+               /*
+                * Remove the existing symlink if any and Create the
symlink
+                * under PGDATA.  We need to use rmtree instead of rmdir
as
+                * the link location might contain directories or files
+                * corresponding to the actual path. Some tar utilities
do
+                * things that way while extracting symlinks.
+                */
+               if (lstat(linkloc, &st) == 0 && S_ISDIR(st.st_mode))
+               {
+                   if (!rmtree(linkloc,true))
+                       ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not remove directory
\"%s\": %m",
+                                       linkloc)));
+               }
+               else
+               {
+                   if (unlink(linkloc) < 0 && errno != ENOENT)
+                       ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not remove symbolic link
\"%s\": %m",
+                                       linkloc)));
+               }
+

That's scary. What tar utilitiy replaces the symlink with a non-empty
directory?

IIRC, it was star utility by using --copysymlinks options.
It will actually copy the symlink data at appropriate location,
but will not maintain symlink after extraction.
I don't have a link handly for it, but can again search for
it and send you if you want to have a look.

What if you call pg_start_backup() and take the backup with a utility that

follows symlinks? I wouldn't recommend using such a tool, but with this
patch, it will zap all the tablespaces. Before this, you at least got a
working database and could read out all the data or fix the symlinks
afterwards.

Yeah, but I don't think user will do such a thing without
being aware of the same and if he is aware, he will either
restore the symlinks before starting the server or would
atleast keep a copy of such a backup until he is able to
restore the database completely.

Do you think adding a note in docs makes sense?

Why is the RelationCacheInitFileRemove() call moved?

I will let Amit comment on this.

Because it assumes that tablespace directory pg_tblspc is all in
place and it tries to remove the files by reading pg_tblspc
directory as well. Now as we setup the symlinks in pg_tblspc
after reading tablespace_map file, so we should remove relcache init
file once the symlinks are setup in pg_tblspc directory.

XLogRecPtr

do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID
*starttli_p,
-                  char **labelfile)
+                  char **labelfile, DIR *tblspcdir, List **tablespaces,
+                  char **tblspcmapfile, bool infotbssize,
+                  bool needtblspcmapfile)

Why does this need to take tblspcdir as argument?

It needs to be called from perform_base_backup() which already
has access to tblspcdir.

Wouldn't it make more sense to open the directory inside

do_pg_start_backup?

Good point. It does look cleaner to do that.

If you prefer that way, I can look into rearranging the
code.

In the BASE_BACKUP command, is there any reason to not always include the
tablespace map?

The main reason is that it is not required to restore symlinks
for plain format. Also I think it can cause a problem if user
has used --tablespace-mapping option in plain format as the
tablespace_map file doesn't contain information for same and recovery
will try to restore symlinks for wrong path based on tablespace_map
file.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

[redirecting to -hackers]

On 05/12/2015 01:30 PM, Amit Kapila wrote:

On Tue, May 12, 2015 at 9:02 PM, Andrew Dunstan <andrew@dunslane.net
<mailto:andrew@dunslane.net>> wrote:

On 05/12/2015 10:33 AM, Heikki Linnakangas wrote:

On 05/12/2015 04:42 PM, Andrew Dunstan wrote:

+
+               /*
+                * Remove the existing symlink if any and
Create the symlink
+                * under PGDATA.  We need to use rmtree
instead of rmdir as
+                * the link location might contain
directories or files
+                * corresponding to the actual path. Some
tar utilities do
+                * things that way while extracting symlinks.
+                */
+               if (lstat(linkloc, &st) == 0 &&
S_ISDIR(st.st_mode))
+               {
+                   if (!rmtree(linkloc,true))
+                       ereport(ERROR,
+  (errcode_for_file_access(),
+                                errmsg("could not remove
directory \"%s\": %m",
+                                       linkloc)));
+               }
+               else
+               {
+                   if (unlink(linkloc) < 0 && errno !=
ENOENT)
+                       ereport(ERROR,
+  (errcode_for_file_access(),
+                                errmsg("could not remove
symbolic link \"%s\": %m",
+                                       linkloc)));
+               }
+

That's scary. What tar utilitiy replaces the symlink with a
non-empty directory?

IIRC, it was star utility by using --copysymlinks options.
It will actually copy the symlink data at appropriate location,
but will not maintain symlink after extraction.
I don't have a link handly for it, but can again search for
it and send you if you want to have a look.

What if you call pg_start_backup() and take the backup with a
utility that follows symlinks? I wouldn't recommend using such
a tool, but with this patch, it will zap all the tablespaces.
Before this, you at least got a working database and could
read out all the data or fix the symlinks afterwards.

Yeah, but I don't think user will do such a thing without
being aware of the same and if he is aware, he will either
restore the symlinks before starting the server or would
atleast keep a copy of such a backup until he is able to
restore the database completely.

Do you think adding a note in docs makes sense?

How about if we simply abort if we find a non-symlink where we want the
symlink to be, and only remove something that is actually a symlink (or
a junction point, which is more or less the same thing)? Then it would
be up to the user to recover the situation, by moving or removing the
offending file/directory, and trying again.

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

How about if we simply abort if we find a non-symlink where we want the

symlink to be, and only remove something that is actually a symlink (or a
junction point, which is more or less the same thing)?

We can do that way and for that I think we need to use rmdir
instead of rmtree in the code being discussed (recovery path),
OTOH we should try to minimize the errors raised during
recovery.

Then it would be up to the user to recover the situation, by moving or

removing the offending file/directory, and trying again.

Yes, I think as we only create/maintain symlinks in pg_tblspc
for tablespaces, so it seems okay even if we error out when we
find directories instead of symlinks in that path.

I can write a patch for this if you, Heikki and or others think that
is the better way to deal with this case.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 14, 2015 at 12:12 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

How about if we simply abort if we find a non-symlink where we want the
symlink to be, and only remove something that is actually a symlink (or a
junction point, which is more or less the same thing)?

We can do that way and for that I think we need to use rmdir
instead of rmtree in the code being discussed (recovery path),
OTOH we should try to minimize the errors raised during
recovery.

I'm not sure I understand this issue in detail, but why would using
rmtree() on something you expect to be a symlink ever be a good idea?
It seems like if things are the way you expect them to be, it has no
benefit, but if they are different from what you expect, you might
blow away a ton of important data.

Maybe I am just confused.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 05/14/2015 10:52 AM, Robert Haas wrote:

On Thu, May 14, 2015 at 12:12 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

How about if we simply abort if we find a non-symlink where we want the
symlink to be, and only remove something that is actually a symlink (or a
junction point, which is more or less the same thing)?

We can do that way and for that I think we need to use rmdir
instead of rmtree in the code being discussed (recovery path),
OTOH we should try to minimize the errors raised during
recovery.

I'm not sure I understand this issue in detail, but why would using
rmtree() on something you expect to be a symlink ever be a good idea?
It seems like if things are the way you expect them to be, it has no
benefit, but if they are different from what you expect, you might
blow away a ton of important data.

Maybe I am just confused.

The suggestion is to get rid of using rmtree. Instead, if we find a
non-symlink in pg_tblspc we'll make the user clean it up before we can
continue. So your instinct is in tune with my suggestion.

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, May 14, 2015 at 12:59 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

I'm not sure I understand this issue in detail, but why would using
rmtree() on something you expect to be a symlink ever be a good idea?
It seems like if things are the way you expect them to be, it has no
benefit, but if they are different from what you expect, you might
blow away a ton of important data.

Maybe I am just confused.

The suggestion is to get rid of using rmtree. Instead, if we find a
non-symlink in pg_tblspc we'll make the user clean it up before we can
continue. So your instinct is in tune with my suggestion.

Right. Maybe I should have just said "+1".

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, May 14, 2015 at 10:29 PM, Andrew Dunstan <andrew@dunslane.net>
wrote:

On 05/14/2015 10:52 AM, Robert Haas wrote:

On Thu, May 14, 2015 at 12:12 AM, Amit Kapila <amit.kapila16@gmail.com>

wrote:

On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan <andrew@dunslane.net>

wrote:

How about if we simply abort if we find a non-symlink where we want the
symlink to be, and only remove something that is actually a symlink

(or a

junction point, which is more or less the same thing)?

We can do that way and for that I think we need to use rmdir
instead of rmtree in the code being discussed (recovery path),
OTOH we should try to minimize the errors raised during
recovery.

I'm not sure I understand this issue in detail, but why would using
rmtree() on something you expect to be a symlink ever be a good idea?
It seems like if things are the way you expect them to be, it has no
benefit, but if they are different from what you expect, you might
blow away a ton of important data.

Maybe I am just confused.

The suggestion is to get rid of using rmtree. Instead, if we find a

non-symlink in pg_tblspc we'll make the user clean it up before we can
continue. So your instinct is in tune with my suggestion.

Find the patch which gets rid of rmtree usage. I have made it as
a separate function because the same code is used from
create_tablespace_directories() as well. I thought of extending the
same API for using it from destroy_tablespace_directories() as well,
but due to special handling (especially for ENOENT) in that function,
I left it as of now.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, May 15, 2015 at 11:51 AM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Thu, May 14, 2015 at 10:29 PM, Andrew Dunstan <andrew@dunslane.net>

wrote:

On 05/14/2015 10:52 AM, Robert Haas wrote:

On Thu, May 14, 2015 at 12:12 AM, Amit Kapila <amit.kapila16@gmail.com>

wrote:

On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan <andrew@dunslane.net>

wrote:

How about if we simply abort if we find a non-symlink where we want

the

symlink to be, and only remove something that is actually a symlink

(or a

junction point, which is more or less the same thing)?

We can do that way and for that I think we need to use rmdir
instead of rmtree in the code being discussed (recovery path),
OTOH we should try to minimize the errors raised during
recovery.

I'm not sure I understand this issue in detail, but why would using
rmtree() on something you expect to be a symlink ever be a good idea?
It seems like if things are the way you expect them to be, it has no
benefit, but if they are different from what you expect, you might
blow away a ton of important data.

Maybe I am just confused.

The suggestion is to get rid of using rmtree. Instead, if we find a

non-symlink in pg_tblspc we'll make the user clean it up before we can
continue. So your instinct is in tune with my suggestion.

Find the patch which gets rid of rmtree usage. I have made it as
a separate function because the same code is used from
create_tablespace_directories() as well. I thought of extending the
same API for using it from destroy_tablespace_directories() as well,
but due to special handling (especially for ENOENT) in that function,
I left it as of now.

Does it make sense to track this in 9.5 Open Items [1]https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items?

[1]: https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 05/23/2015 01:29 AM, Amit Kapila wrote:

On Fri, May 15, 2015 at 11:51 AM, Amit Kapila <amit.kapila16@gmail.com
<mailto:amit.kapila16@gmail.com>> wrote:

On Thu, May 14, 2015 at 10:29 PM, Andrew Dunstan

<andrew@dunslane.net <mailto:andrew@dunslane.net>> wrote:

On 05/14/2015 10:52 AM, Robert Haas wrote:

On Thu, May 14, 2015 at 12:12 AM, Amit Kapila

<amit.kapila16@gmail.com <mailto:amit.kapila16@gmail.com>> wrote:

On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan

<andrew@dunslane.net <mailto:andrew@dunslane.net>> wrote:

How about if we simply abort if we find a non-symlink where we

want the

symlink to be, and only remove something that is actually a

symlink (or a

junction point, which is more or less the same thing)?

We can do that way and for that I think we need to use rmdir
instead of rmtree in the code being discussed (recovery path),
OTOH we should try to minimize the errors raised during
recovery.

I'm not sure I understand this issue in detail, but why would using
rmtree() on something you expect to be a symlink ever be a good idea?
It seems like if things are the way you expect them to be, it has no
benefit, but if they are different from what you expect, you might
blow away a ton of important data.

Maybe I am just confused.

The suggestion is to get rid of using rmtree. Instead, if we find

a non-symlink in pg_tblspc we'll make the user clean it up before we
can continue. So your instinct is in tune with my suggestion.

Find the patch which gets rid of rmtree usage. I have made it as
a separate function because the same code is used from
create_tablespace_directories() as well. I thought of extending the
same API for using it from destroy_tablespace_directories() as well,
but due to special handling (especially for ENOENT) in that function,
I left it as of now.

Does it make sense to track this in 9.5 Open Items [1]?

[1] - https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items

Sure. It's on my list of things to get to, but by all means put it there.

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, May 23, 2015 at 9:28 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 05/23/2015 01:29 AM, Amit Kapila wrote:

Find the patch which gets rid of rmtree usage. I have made it as
a separate function because the same code is used from
create_tablespace_directories() as well. I thought of extending the
same API for using it from destroy_tablespace_directories() as well,
but due to special handling (especially for ENOENT) in that function,
I left it as of now.

Does it make sense to track this in 9.5 Open Items [1]?

[1] - https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items

Sure. It's on my list of things to get to, but by all means put it there.

Thanks for tracking. I have added to open items as well.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 05/15/2015 02:21 AM, Amit Kapila wrote:

On Thu, May 14, 2015 at 10:29 PM, Andrew Dunstan <andrew@dunslane.net
<mailto:andrew@dunslane.net>> wrote:

On 05/14/2015 10:52 AM, Robert Haas wrote:

On Thu, May 14, 2015 at 12:12 AM, Amit Kapila

<amit.kapila16@gmail.com <mailto:amit.kapila16@gmail.com>> wrote:

On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan

<andrew@dunslane.net <mailto:andrew@dunslane.net>> wrote:

How about if we simply abort if we find a non-symlink where we

want the

symlink to be, and only remove something that is actually a

symlink (or a

junction point, which is more or less the same thing)?

We can do that way and for that I think we need to use rmdir
instead of rmtree in the code being discussed (recovery path),
OTOH we should try to minimize the errors raised during
recovery.

I'm not sure I understand this issue in detail, but why would using
rmtree() on something you expect to be a symlink ever be a good idea?
It seems like if things are the way you expect them to be, it has no
benefit, but if they are different from what you expect, you might
blow away a ton of important data.

Maybe I am just confused.

The suggestion is to get rid of using rmtree. Instead, if we find a

non-symlink in pg_tblspc we'll make the user clean it up before we can
continue. So your instinct is in tune with my suggestion.

Find the patch which gets rid of rmtree usage. I have made it as
a separate function because the same code is used from
create_tablespace_directories() as well. I thought of extending the
same API for using it from destroy_tablespace_directories() as well,
but due to special handling (especially for ENOENT) in that function,
I left it as of now.

Well, it seems to me the new function is being altogether way too
trusting about the nature of what it's being asked to remove. In the
first place, the S_ISDIR/rmdir branch should only be for Windows, and
secondly in the other branch we should be checking that S_ISLNK is true.
It would actually be nice if we could test for a junction point on
Windows, but that seems to be a bit difficult. The function should
probably return a boolean to indicate any error, rather than void, so
that the caller can take action accordingly. In the present case we
should probably be stopping recovery dead in its tracks, but I certainly
don't think we should just be ignoring it.

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jun 2, 2015 at 10:26 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 05/15/2015 02:21 AM, Amit Kapila wrote:

Find the patch which gets rid of rmtree usage. I have made it as
a separate function because the same code is used from
create_tablespace_directories() as well. I thought of extending the
same API for using it from destroy_tablespace_directories() as well,
but due to special handling (especially for ENOENT) in that function,
I left it as of now.

Well, it seems to me the new function is being altogether way too trusting
about the nature of what it's being asked to remove. In the first place,
the S_ISDIR/rmdir branch should only be for Windows, and secondly in the
other branch we should be checking that S_ISLNK is true. It would actually
be nice if we could test for a junction point on Windows, but that seems to
be a bit difficult.

I think during recovery for tablespace related operations, it is
quite possible to have a directory instead of symlink in some
special cases (see function TablespaceCreateDbspace() and comments
in destroy_tablespace_directories() { ..Try to remove the symlink..}).
Also this new function is being called from create_tablespace_directories()
which uses the code as written in new function, so it doesn't make much
sense to change it Windows and non-Windows specific code.

The function should probably return a boolean to indicate any error,
rather than void, so that the caller can take action accordingly.

I think returning boolean is good if the function has Windows
and non-Windows specific code, else we might need short, int16
or something like that as there will be three return values
(0 - success, 1 - fail to remove directory, 2 - fail to remove
symlink).

On the whole this function can be mainly used for tablespace
related paths during recovery, it isn't generic enough to be
use from all paths. I think as proposed name of the new
function (rmsymlink) looks quite generic, so either we can
change it to (rm_tablespace_symlinks) or may be I can just
duplicate the code in read_tablespace_map() related check and
then we don't need this new function.

In the present case we should probably be stopping recovery dead in its
tracks, but I certainly don't think we should just be ignoring it.

Do you think we should do anything before returning error?
This operation haapens in the beginning of recovery before
replay of any records, so throwing ERROR from here shouldn't
have any impact unless we have done any operation which
can create problem when user next time starts the recovery.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 06/02/2015 11:55 PM, Amit Kapila wrote:

On Tue, Jun 2, 2015 at 10:26 PM, Andrew Dunstan <andrew@dunslane.net
<mailto:andrew@dunslane.net>> wrote:

On 05/15/2015 02:21 AM, Amit Kapila wrote:

Find the patch which gets rid of rmtree usage. I have made it as
a separate function because the same code is used from
create_tablespace_directories() as well. I thought of
extending the
same API for using it from destroy_tablespace_directories() as
well,
but due to special handling (especially for ENOENT) in that
function,
I left it as of now.

Well, it seems to me the new function is being altogether way too
trusting about the nature of what it's being asked to remove. In
the first place, the S_ISDIR/rmdir branch should only be for
Windows, and secondly in the other branch we should be checking
that S_ISLNK is true. It would actually be nice if we could test
for a junction point on Windows, but that seems to be a bit
difficult.

I think during recovery for tablespace related operations, it is
quite possible to have a directory instead of symlink in some
special cases (see function TablespaceCreateDbspace() and comments
in destroy_tablespace_directories() { ..Try to remove the symlink..}).
Also this new function is being called from
create_tablespace_directories()
which uses the code as written in new function, so it doesn't make much
sense to change it Windows and non-Windows specific code.

Looking at it again, this might be not as bad as I thought, but I do
think we should probably call the function something other than
rmsymlink(). That seems too generic, since it also tries to remove
directories - albeit that this will fail if the directory isn't empty.
And I still think we should add a test for S_ISLNK in the second branch.
As it stands the function could try to unlink anything that's not a
directory. That might be safe-ish in the context it's used in for the
tablespace code, but it's far from safe enough for a function that's in
src/common

Given that the function raises an error on failure, I think it will
otherwise be OK as is.

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jun 4, 2015 at 1:52 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 06/02/2015 11:55 PM, Amit Kapila wrote:

On Tue, Jun 2, 2015 at 10:26 PM, Andrew Dunstan <andrew@dunslane.net

<mailto:andrew@dunslane.net>> wrote:

Well, it seems to me the new function is being altogether way too
trusting about the nature of what it's being asked to remove. In
the first place, the S_ISDIR/rmdir branch should only be for
Windows, and secondly in the other branch we should be checking
that S_ISLNK is true. It would actually be nice if we could test
for a junction point on Windows, but that seems to be a bit
difficult.

I think during recovery for tablespace related operations, it is
quite possible to have a directory instead of symlink in some
special cases (see function TablespaceCreateDbspace() and comments
in destroy_tablespace_directories() { ..Try to remove the symlink..}).
Also this new function is being called from
create_tablespace_directories()
which uses the code as written in new function, so it doesn't make much
sense to change it Windows and non-Windows specific code.

Looking at it again, this might be not as bad as I thought, but I do think
we should probably call the function something other than rmsymlink(). That
seems too generic, since it also tries to remove directories - albeit that
this will fail if the directory isn't empty. And I still think we should
add a test for S_ISLNK in the second branch. As it stands the function
could try to unlink anything that's not a directory. That might be safe-ish
in the context it's used in for the tablespace code, but it's far from safe
enough for a function that's in src/common

Okay, as we both seem to agree that it can be mostly used in
tablespace symlinks context, so I have changed the name to
remove_tablespace_symlink() and moved the function to
tablespace.c. S_ISLINK check is used for non-windows code,
so not sure adding it here makes any real difference now that
we have made it specific to tablespace and we might need to
write small port specific code if we want to add S_ISLINK check.

Given that the function raises an error on failure, I think it will
otherwise be OK as is.

Please find an updated patch attached with this mail.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jun 4, 2015 at 10:14 AM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Thu, Jun 4, 2015 at 1:52 AM, Andrew Dunstan <andrew@dunslane.net>
wrote:

On 06/02/2015 11:55 PM, Amit Kapila wrote:

On Tue, Jun 2, 2015 at 10:26 PM, Andrew Dunstan <andrew@dunslane.net

<mailto:andrew@dunslane.net>> wrote:

Well, it seems to me the new function is being altogether way too
trusting about the nature of what it's being asked to remove. In
the first place, the S_ISDIR/rmdir branch should only be for
Windows, and secondly in the other branch we should be checking
that S_ISLNK is true. It would actually be nice if we could test
for a junction point on Windows, but that seems to be a bit
difficult.

I think during recovery for tablespace related operations, it is
quite possible to have a directory instead of symlink in some
special cases (see function TablespaceCreateDbspace() and comments
in destroy_tablespace_directories() { ..Try to remove the symlink..}).
Also this new function is being called from
create_tablespace_directories()
which uses the code as written in new function, so it doesn't make much
sense to change it Windows and non-Windows specific code.

Looking at it again, this might be not as bad as I thought, but I do
think we should probably call the function something other than
rmsymlink(). That seems too generic, since it also tries to remove
directories - albeit that this will fail if the directory isn't empty. And
I still think we should add a test for S_ISLNK in the second branch. As it
stands the function could try to unlink anything that's not a directory.
That might be safe-ish in the context it's used in for the tablespace code,
but it's far from safe enough for a function that's in src/common

Okay, as we both seem to agree that it can be mostly used in
tablespace symlinks context, so I have changed the name to
remove_tablespace_symlink() and moved the function to
tablespace.c. S_ISLINK check is used for non-windows code,
so not sure adding it here makes any real difference now that
we have made it specific to tablespace and we might need to
write small port specific code if we want to add S_ISLINK check.

Given that the function raises an error on failure, I think it will
otherwise be OK as is.

Please find an updated patch attached with this mail.

Sorry, forgot to attach the patch in previous mail, now attaching.

Thanks Bruce for reminding me offlist.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 06/04/2015 12:44 AM, Amit Kapila wrote:

Given that the function raises an error on failure, I think it
will otherwise be OK as is.

Please find an updated patch attached with this mail.

No attachment.

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jun 4, 2015 at 8:43 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 06/04/2015 12:44 AM, Amit Kapila wrote:

Given that the function raises an error on failure, I think it
will otherwise be OK as is.

Please find an updated patch attached with this mail.

No attachment.

cheers

I have sent it in the next mail, but anyway sending it again
in case you have missed it.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com