Thread: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file

[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

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

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

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

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

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

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

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

On 06/04/2015 09:23 AM, Amit Kapila wrote:
>
>
>
>     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.
>


Where is it used? I can't see it called at all in tablespace.c or xlog.c.

Perhaps I'm being overcautious, but here's more or less what I had in mind.

cheers

andrew

On 06/04/2015 11:35 PM, Amit Kapila wrote:
> On Fri, Jun 5, 2015 at 7:29 AM, Andrew Dunstan <andrew@dunslane.net 
> <mailto:andrew@dunslane.net>> wrote:
>
>
>     On 06/04/2015 09:23 AM, Amit Kapila wrote:
>
>
>
>
>             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.
>
>
>
>     Where is it used? I can't see it called at all in tablespace.c or
>     xlog.c.
>
>
> Below files use S_ISLINK check
> basebackup.c, fd.c, initdb.c, copy_fetch.c, pg_rewind/filemap.c
>
> and all these places use it with #ifndef WIN32
>
>     Perhaps I'm being overcautious, but here's more or less what I had
>     in mind.
>
>
> What is making you feel nervous, if it is that we should not
> use unlink call without checking S_ISLINK, then we are
> already doing the same at many other places (rewriteheap.c,
> slru.c, timeline.c, xlog.c).  It is already defined for Windows
> as pgunlink.
>
> Theoretically, I don't see much problem by changing the checks
> way you have done in patch, but it becomes different than what
> we have in destroy_tablespace_directories() and it is slightly
> changing the way check was originally done in
> create_tablespace_directories(), basically original check will try
> unlink if lstat returns non-zero return code. If you want to proceed
> with the changed checks as in v3, then may be we can modify
> comments on top of function remove_tablespace_symlink() which
> indicates that it works like destroy_tablespace_directories().
>


The difference is that here we're getting the list from a base backup 
and it seems to me the risk of having a file we don't really want to 
unlink is significantly greater.

cheers

andrew

On 06/05/2015 11:08 PM, Amit Kapila wrote:
> On Fri, Jun 5, 2015 at 10:51 AM, Amit Kapila <amit.kapila16@gmail.com 
> <mailto:amit.kapila16@gmail.com>> wrote:
>
>     On Fri, Jun 5, 2015 at 9:57 AM, Andrew Dunstan
>     <andrew@dunslane.net <mailto:andrew@dunslane.net>> wrote:
>
>
>         On 06/04/2015 11:35 PM, Amit Kapila wrote:
>
>
>             Theoretically, I don't see much problem by changing the checks
>             way you have done in patch, but it becomes different than what
>             we have in destroy_tablespace_directories() and it is slightly
>             changing the way check was originally done in
>             create_tablespace_directories(), basically original check
>             will try
>             unlink if lstat returns non-zero return code. If you want
>             to proceed
>             with the changed checks as in v3, then may be we can modify
>             comments on top of function remove_tablespace_symlink() which
>             indicates that it works like destroy_tablespace_directories().
>
>
>
>         The difference is that here we're getting the list from a base
>         backup and it seems to me the risk of having a file we don't
>         really want to unlink is significantly greater.
>
>
>     Okay, I think I can understand why you want to be cautious for
>     having a different check for this path, but in that case there is a
>     chance that recovery might fail when it will try to create a symlink
>     for that file.  Shouldn't we then try to call this new function only
>     when we are trying to restore from tablespace_map file and also
>     is there a need of ifdef S_ISLINK in remove_tablespace_link?
>
>
> Shall I send an updated patch on these lines or do you want to
> proceed with v3 version?
>
>

The point seems to me that we should not be removing anything that's not 
an empty directory or symlink, and that nothing else has any business 
being in pg_tblspc. If we encounter such a thing whose name conflicts 
with the name of a tablespace link we wish to create, rather than 
quietly blowing it away we should complain loudly, and error out, making 
it the user's responsibility to clean up their mess. Am I missing something?

cheers

andrew

On 06/08/2015 12:08 AM, Amit Kapila wrote:
> On Mon, Jun 8, 2015 at 5:52 AM, Andrew Dunstan <andrew@dunslane.net 
> <mailto:andrew@dunslane.net>> wrote:
> >
> > On 06/05/2015 11:08 PM, Amit Kapila wrote:
> >>
> >>
> >>     Okay, I think I can understand why you want to be cautious for
> >>     having a different check for this path, but in that case there is a
> >>     chance that recovery might fail when it will try to create a 
> symlink
> >>     for that file.  Shouldn't we then try to call this new function 
> only
> >>     when we are trying to restore from tablespace_map file and also
> >>     is there a need of ifdef S_ISLINK in remove_tablespace_link?
> >>
> >>
> >> Shall I send an updated patch on these lines or do you want to
> >> proceed with v3 version?
> >>
> >>
> >
> > The point seems to me that we should not be removing anything that's 
> not an empty directory or symlink, and that nothing else has any 
> business being in pg_tblspc. If we encounter such a thing whose name 
> conflicts with the name of a tablespace link we wish to create, rather 
> than quietly blowing it away we should complain loudly, and error out, 
> making it the user's responsibility to clean up their mess. Am I 
> missing something?
> >
>
> How about if it is just a flat file with same name as tablespace link,
> why we want to give error for that case?  I think now it just don't do
> anything with that file (unlink will fail with ENOENT and it will be
> ignored, atleast thats the way currently it behaves in Windows) and
> create a separate symlink with same name which seems okay to
> me and in the change proposed by you it will give error, do you see
> any reason for doing so?
>
>


This is surely wrong. unlink won't fail with ENOENT if the file is 
present; ENOENT means that the file is NOT present. It will succeed if 
the file is present, which is exactly what I'm saying is wrong.

I realize our existing code just more or less assumes that that it's a 
symlink. I think we've probably been a bit careless there.

cheers

andrew

On Mon, Jun 8, 2015 at 11:16 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> I have to retry that operation, but for me unlink hasn't deleted
> the file on Windows, may be I am not doing properly, but in
> anycase why we want to throw error for such a case, why
> can't we just ignore and create a symlink with the same name.
>
>> I realize our existing code just more or less assumes that that it's a
>> symlink. I think we've probably been a bit careless there.
>
> I agree with you that deleting unrelated file with the same name as
> symlink is not the right thing to do, but not sure throwing error for
> such a case is better either.

Why not?  I think that if we encounter some sort of situation that we
think should never happen, throwing an error is exactly what we
*should* do.  Particularly when it comes to things like removing
files, it is very dangerous for the database to proceed if the
situation is not as expected.  We should only remove things if we are
quite sure that removing them is the right thing to do.

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

Robert Haas wrote:

> Why not?  I think that if we encounter some sort of situation that we
> think should never happen, throwing an error is exactly what we
> *should* do.  Particularly when it comes to things like removing
> files, it is very dangerous for the database to proceed if the
> situation is not as expected.  We should only remove things if we are
> quite sure that removing them is the right thing to do.

Yes, agreed.

I notice that we use %m in places where I'm not sure errno is the right
thing.  Consider the ereport() at lines 10385ff, for instance.  I don't
think fgetc() nor ferror() set errno.

I became aware of this because last week I was reading some bogus
pg_dump code that reported "could not write foobar: Success" and noticed
that the macros READ_ERROR_EXIT and WRITE_ERROR_EXIT also do
strerror(errno) after doing some fread() or similar.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 06/08/2015 11:16 AM, Amit Kapila wrote:
> On Mon, Jun 8, 2015 at 6:39 PM, Andrew Dunstan <andrew@dunslane.net 
> <mailto:andrew@dunslane.net>> wrote:
>
>
>     On 06/08/2015 12:08 AM, Amit Kapila wrote:
>
>         How about if it is just a flat file with same name as
>         tablespace link,
>         why we want to give error for that case?  I think now it just
>         don't do
>         anything with that file (unlink will fail with ENOENT and it
>         will be
>         ignored, atleast thats the way currently it behaves in
>         Windows) and
>         create a separate symlink with same name which seems okay to
>         me and in the change proposed by you it will give error, do
>         you see
>         any reason for doing so?
>
>
>
>
>     This is surely wrong. unlink won't fail with ENOENT if the file is
>     present; ENOENT means that the file is NOT present. It will
>     succeed if the file is present, which is exactly what I'm saying
>     is wrong.
>
>
> I have to retry that operation, but for me unlink hasn't deleted
> the file on Windows, may be I am not doing properly, but in
> anycase why we want to throw error for such a case, why
> can't we just ignore and create a symlink with the same name.

1. You realize that in Windows postgres, unlink is actually pgunlink(), 
right? See port.h. If your experiments weren't using that then they 
weren't testing the same thing.

2. If the unlink fails and the file is still there (i.e. pretty much 
everything except the ENOENT case) then creation of the symlink is bound 
to fail anyway.

>     I realize our existing code just more or less assumes that that
>     it's a symlink. I think we've probably been a bit careless there.
>
>
> I agree with you that deleting unrelated file with the same name as
> symlink is not the right thing to do, but not sure throwing error for
> such a case is better either.
>
>


What else would you suggest? Closing our eyes and wishing it weren't so 
doesn't seem like a solution.

cheers

andrew

On 06/08/2015 11:19 PM, Amit Kapila wrote:
> On Tue, Jun 9, 2015 at 12:27 AM, Andrew Dunstan <andrew@dunslane.net 
> <mailto:andrew@dunslane.net>> wrote:
>
>
>     On 06/08/2015 11:16 AM, Amit Kapila wrote:
>
>
>         I have to retry that operation, but for me unlink hasn't deleted
>         the file on Windows, may be I am not doing properly, but in
>         anycase why we want to throw error for such a case, why
>         can't we just ignore and create a symlink with the same name.
>
>
>     1. You realize that in Windows postgres, unlink is actually
>     pgunlink(), right? See port.h. If your experiments weren't using
>     that then they weren't testing the same thing.
>
>
> Yes, I know that and was using the same version, but the
> small problem in my test was that the file name that is used
> for unlink was not same as that of actual file in directory, sorry
> for the noise.
>
>     2. If the unlink fails and the file is still there (i.e. pretty
>     much everything except the ENOENT case) then creation of the
>     symlink is bound to fail anyway.
>
>           I realize our existing code just more or less assumes that that
>             it's a symlink. I think we've probably been a bit careless
>         there.
>
>
>         I agree with you that deleting unrelated file with the same
>         name as
>         symlink is not the right thing to do, but not sure throwing
>         error for
>         such a case is better either.
>
>
>
>
>     What else would you suggest? 
>
>
> I think Robert and Alvaro also seems to be inclined towards throwing
> error for such a case, so let us do that way, but one small point is that
> don't you think that similar code in destroy_tablespace_directories() 
> under
> label "remove_symlink:" should use similar logic?


Yes, probably.

cheers

andrew

On Wed, Jun 10, 2015 at 3:34 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Okay, I have updated the patch to destroy_tablespace_directories() code
> as well in the attached patch. I have tried to modify
> remove_tablespace_symlink(), so that it can be called from
> destroy_tablespace_directories(), but that is making it more complex,
> especially due to the reason that destroy_tablespace_directories()
> treats ENOENT as warning rather than ignoring it.

This pretty obviously doesn't follow style guidelines.  You've got it
started with a capital letter, and there are two spaces between "a"
and "directory".
                errmsg("Not a  directory or symbolic link: \"%s\"",

But it looks OK otherwise, so committed.

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