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

[Fujii Masao]

On Fri, Sep 11, 2015 at 1:57 PM, Amit Kapila  wrote:
> On Fri, Sep 11, 2015 at 10:10 AM, Fujii Masao  wrote:
>>
>> So I added the object type, i.e., file in this case, to the errdetail
>> messages. Attached is the updated version of the patch.
>>
>> I also changed other log messages related to tablespace_map
>> so that they follow the style guide.
>>
>
> Looks good to me.

Thanks for the review! Applied.

Regards,

-- 
Fujii Masao


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

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

[Amit Kapila]

On Fri, Sep 11, 2015 at 10:10 AM, Fujii Masao  wrote:
>
> So I added the object type, i.e., file in this case, to the errdetail
> messages. Attached is the updated version of the patch.
>
> I also changed other log messages related to tablespace_map
> so that they follow the style guide.
>

Looks good to me.



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

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

[Fujii Masao]

On Fri, Sep 11, 2015 at 5:43 AM, Robert Haas  wrote:
> On Thu, Sep 10, 2015 at 3:52 PM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> On Wed, Sep 9, 2015 at 11:49 PM, Amit Kapila  
>>> wrote:
 - errdetail("Could not rename \"%s\" to \"%s\": %m.",
 + errdetail("\"%s\" could not be renamed to \"%s\": %m.",

 Is there any reason to change this message?
 I think you have changed this message to make it somewhat similar with
 the new message we are planning to use in this function, but I don't see
 that as compelling reason to change this message.
>>
>>> The old message better follows the guidelines.  See section 51.3.7:
>>> Avoid Passive Voice.  The old message is what's called
>>> "telegram-style", with PostgreSQL itself as the implicit subject.  The
>>> proposed replacement is just the regular old passive voice.
>>
>> Neither version is following the guidelines very well, in particular they
>> should be mentioning what kind of object %s is (file? table? tablespace?).
>> But to me the "could not be renamed" version seems to be closer to the
>> spirit of the "use complete sentences" rule for errdetail.  The other one
>> seems better fit for a primary error message, which is supposed to be
>> kept short.
>
> Hmm, I did miss the fact that this was an errdetail().  I agree that
> the object type should be mentioned either way.

So I added the object type, i.e., file in this case, to the errdetail
messages. Attached is the updated version of the patch.

I also changed other log messages related to tablespace_map
so that they follow the style guide.

Regards,

-- 
Fujii Masao
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 152d4ed..a092aad 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6160,13 +6160,13 @@ StartupXLOG(void)
 ereport(LOG,
 	(errmsg("ignoring \"%s\" file because no \"%s\" file exists",
 			TABLESPACE_MAP, BACKUP_LABEL_FILE),
-	 errdetail("\"%s\" was renamed to \"%s\".",
+	 errdetail("File \"%s\" was renamed to \"%s\".",
 			   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
 			else
 ereport(LOG,
 		(errmsg("ignoring \"%s\" file because no \"%s\" file exists",
 TABLESPACE_MAP, BACKUP_LABEL_FILE),
-		 errdetail("Could not rename file \"%s\" to \"%s\": %m.",
+		 errdetail("File \"%s\" could not be renamed to \"%s\": %m.",
    TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
 		}
 
@@ -10911,32 +10911,32 @@ CancelBackup(void)
 {
 	struct stat stat_buf;
 
-	/* if the file is not there, return */
+	/* if the backup_label file is not there, return */
 	if (stat(BACKUP_LABEL_FILE, &stat_buf) < 0)
 		return;
 
 	/* remove leftover file from previously canceled backup if it exists */
 	unlink(BACKUP_LABEL_OLD);
 
-	if (rename(BACKUP_LABEL_FILE, BACKUP_LABEL_OLD) == 0)
-	{
-		ereport(LOG,
-(errmsg("online backup mode canceled"),
- errdetail("\"%s\" was renamed to \"%s\".",
-		   BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));
-	}
-	else
+	if (rename(BACKUP_LABEL_FILE, BACKUP_LABEL_OLD) != 0)
 	{
 		ereport(WARNING,
 (errcode_for_file_access(),
  errmsg("online backup mode was not canceled"),
- errdetail("Could not rename \"%s\" to \"%s\": %m.",
+ errdetail("File \"%s\" could not be renamed to \"%s\": %m.",
 		   BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));
+		return;
 	}
 
 	/* if the tablespace_map file is not there, return */
 	if (stat(TABLESPACE_MAP, &stat_buf) < 0)
+	{
+		ereport(LOG,
+(errmsg("online backup mode canceled"),
+ errdetail("File \"%s\" was renamed to \"%s\".",
+		   BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));
 		return;
+	}
 
 	/* remove leftover file from previously canceled backup if it exists */
 	unlink(TABLESPACE_MAP_OLD);
@@ -10945,15 +10945,19 @@ CancelBackup(void)
 	{
 		ereport(LOG,
 (errmsg("online backup mode canceled"),
- errdetail("\"%s\" was renamed to \"%s\".",
-		   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
+ errdetail("Files \"%s\" and \"%s\" were renamed to "
+		   "\"%s\" and \"%s\", respectively.",
+		   BACKUP_LABEL_FILE, TABLESPACE_MAP,
+		   BACKUP_LABEL_OLD, TABLESPACE_MAP_OLD)));
 	}
 	else
 	{
 		ereport(WARNING,
 (errcode_for_file_access(),
- errmsg("online backup mode was not canceled"),
- errdetail("Could not rename \"%s\" to \"%s\": %m.",
+ errmsg("online backup mode canceled"),
+ errdetail("File \"%s\" was renamed to \"%s\", but "
+		   "file \"%s\" could not be renamed to \"%s\": %m.",
+		   BACKUP_LABEL_FILE, BACKUP_LABEL_OLD,
 		   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
 	}
 }

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

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

[Robert Haas]

On Thu, Sep 10, 2015 at 3:52 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Sep 9, 2015 at 11:49 PM, Amit Kapila  wrote:
>>> - errdetail("Could not rename \"%s\" to \"%s\": %m.",
>>> + errdetail("\"%s\" could not be renamed to \"%s\": %m.",
>>>
>>> Is there any reason to change this message?
>>> I think you have changed this message to make it somewhat similar with
>>> the new message we are planning to use in this function, but I don't see
>>> that as compelling reason to change this message.
>
>> The old message better follows the guidelines.  See section 51.3.7:
>> Avoid Passive Voice.  The old message is what's called
>> "telegram-style", with PostgreSQL itself as the implicit subject.  The
>> proposed replacement is just the regular old passive voice.
>
> Neither version is following the guidelines very well, in particular they
> should be mentioning what kind of object %s is (file? table? tablespace?).
> But to me the "could not be renamed" version seems to be closer to the
> spirit of the "use complete sentences" rule for errdetail.  The other one
> seems better fit for a primary error message, which is supposed to be
> kept short.

Hmm, I did miss the fact that this was an errdetail().  I agree that
the object type should be mentioned either way.  Actually, it's sort
of surprising that this message is a detail message rather than a
primary message.

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

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

[Tom Lane]

Robert Haas  writes:
> On Wed, Sep 9, 2015 at 11:49 PM, Amit Kapila  wrote:
>> - errdetail("Could not rename \"%s\" to \"%s\": %m.",
>> + errdetail("\"%s\" could not be renamed to \"%s\": %m.",
>> 
>> Is there any reason to change this message?
>> I think you have changed this message to make it somewhat similar with
>> the new message we are planning to use in this function, but I don't see
>> that as compelling reason to change this message.

> The old message better follows the guidelines.  See section 51.3.7:
> Avoid Passive Voice.  The old message is what's called
> "telegram-style", with PostgreSQL itself as the implicit subject.  The
> proposed replacement is just the regular old passive voice.

Neither version is following the guidelines very well, in particular they
should be mentioning what kind of object %s is (file? table? tablespace?).
But to me the "could not be renamed" version seems to be closer to the
spirit of the "use complete sentences" rule for errdetail.  The other one
seems better fit for a primary error message, which is supposed to be
kept short.

regards, tom lane


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

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

[Robert Haas]

On Wed, Sep 9, 2015 at 11:49 PM, Amit Kapila  wrote:
> - errdetail("Could not rename \"%s\" to \"%s\": %m.",
> + errdetail("\"%s\" could not be renamed to \"%s\": %m.",
>
> Is there any reason to change this message?
> I think you have changed this message to make it somewhat similar with
> the new message we are planning to use in this function, but I don't see
> that as compelling reason to change this message.

The old message better follows the guidelines.  See section 51.3.7:
Avoid Passive Voice.  The old message is what's called
"telegram-style", with PostgreSQL itself as the implicit subject.  The
proposed replacement is just the regular old passive voice.

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

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

[Fujii Masao]

On Thu, Sep 10, 2015 at 1:08 PM, Amit Kapila  wrote:
> On Thu, Sep 10, 2015 at 9:29 AM, Fujii Masao  wrote:
>>
>> On Thu, Sep 10, 2015 at 12:49 PM, Amit Kapila 
>> wrote:
>> > On Wed, Sep 9, 2015 at 6:43 PM, Fujii Masao 
>> > wrote:
>> >>
>> >> On Fri, Sep 4, 2015 at 4:48 PM, Amit Kapila 
>> >> wrote:
>> >>
>> >
>> > - errdetail("Could not rename \"%s\" to \"%s\": %m.",
>> > + errdetail("\"%s\" could not be renamed to \"%s\": %m.",
>> >
>> > Is there any reason to change this message?
>> > I think you have changed this message to make it somewhat similar with
>> > the new message we are planning to use in this function, but I don't see
>> > that as compelling reason to change this message.
>>
>> The following part in error message style guide made me feel that's
>> better.
>> IOW, I didn't think that the previous message follows complete-sentence
>> style.
>>
>> -
>> Detail and hint messages: Use complete sentences, and end each with a
>> period.
>> -
>>
>
> I am not sure if this indicates that previous used message was wrong.
> If we look for errdetail in code, then there are many other similar
> messages.

Yes, but the existence of other similar messages doesn't mean that
they follow the style. We need to understand what "complete sentence" means.
I was thinking that "complete sentence" basically needs to contain
the subject. But to be honest I'm not confident about that.

Regards,

-- 
Fujii Masao


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

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

[Amit Kapila]

On Thu, Sep 10, 2015 at 9:29 AM, Fujii Masao  wrote:
>
> On Thu, Sep 10, 2015 at 12:49 PM, Amit Kapila 
wrote:
> > On Wed, Sep 9, 2015 at 6:43 PM, Fujii Masao 
wrote:
> >>
> >> On Fri, Sep 4, 2015 at 4:48 PM, Amit Kapila 
> >> wrote:
> >>
> >
> > - errdetail("Could not rename \"%s\" to \"%s\": %m.",
> > + errdetail("\"%s\" could not be renamed to \"%s\": %m.",
> >
> > Is there any reason to change this message?
> > I think you have changed this message to make it somewhat similar with
> > the new message we are planning to use in this function, but I don't see
> > that as compelling reason to change this message.
>
> The following part in error message style guide made me feel that's
better.
> IOW, I didn't think that the previous message follows complete-sentence
style.
>
> -
> Detail and hint messages: Use complete sentences, and end each with a
period.
> -
>

I am not sure if this indicates that previous used message was wrong.
If we look for errdetail in code, then there are many other similar
messages.



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

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

[Fujii Masao]

On Thu, Sep 10, 2015 at 12:49 PM, Amit Kapila  wrote:
> On Wed, Sep 9, 2015 at 6:43 PM, Fujii Masao  wrote:
>>
>> On Fri, Sep 4, 2015 at 4:48 PM, Amit Kapila 
>> wrote:
>> >
>> > You mean to say, just try renaming tablespace_map and don't display any
>> > message whether that is successful or not-successful?
>> >
>> > I see some user inconvenience if we do this way, which is even after the
>> > backup is cancelled, on next recovery, there will be a log message
>> > indicating
>> > either rename of tablespace_map successful or unsuccessful.  Also, don't
>> > you
>> > think it is better to let user know that the tablespace_map file is
>> > successfully
>> > renamed as we do for backup_label file.  Shall we change the patch such
>> > that
>> > if backup_label is successfully renamed and renaming of tablespace_map
>> > gets failed, then display a log message to something like below:
>> >
>> > LOG:  online backup mode canceled
>> > DETAIL:  "backup_label" was renamed to "backup_label.old", could not
>> > rename
>> > "tablespace_map" to "tablespace_map.old"
>>
>> Agreed with this direction. So what about the attached patch?
>>
>
> - errdetail("Could not rename \"%s\" to \"%s\": %m.",
> + errdetail("\"%s\" could not be renamed to \"%s\": %m.",
>
> Is there any reason to change this message?
> I think you have changed this message to make it somewhat similar with
> the new message we are planning to use in this function, but I don't see
> that as compelling reason to change this message.

The following part in error message style guide made me feel that's better.
IOW, I didn't think that the previous message follows complete-sentence style.

-
Detail and hint messages: Use complete sentences, and end each with a period.
-

> Other than that patch looks good.

Thanks for the review!

Regards,

-- 
Fujii Masao


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

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

[Amit Kapila]

On Wed, Sep 9, 2015 at 6:43 PM, Fujii Masao  wrote:
>
> On Fri, Sep 4, 2015 at 4:48 PM, Amit Kapila 
wrote:
> >
> > You mean to say, just try renaming tablespace_map and don't display any
> > message whether that is successful or not-successful?
> >
> > I see some user inconvenience if we do this way, which is even after the
> > backup is cancelled, on next recovery, there will be a log message
> > indicating
> > either rename of tablespace_map successful or unsuccessful.  Also,
don't you
> > think it is better to let user know that the tablespace_map file is
> > successfully
> > renamed as we do for backup_label file.  Shall we change the patch such
that
> > if backup_label is successfully renamed and renaming of tablespace_map
> > gets failed, then display a log message to something like below:
> >
> > LOG:  online backup mode canceled
> > DETAIL:  "backup_label" was renamed to "backup_label.old", could not
rename
> > "tablespace_map" to "tablespace_map.old"
>
> Agreed with this direction. So what about the attached patch?
>

- errdetail("Could not rename \"%s\" to \"%s\": %m.",
+ errdetail("\"%s\" could not be renamed to \"%s\": %m.",

Is there any reason to change this message?
I think you have changed this message to make it somewhat similar with
the new message we are planning to use in this function, but I don't see
that as compelling reason to change this message.

Other than that patch looks good.


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

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

[Fujii Masao]

On Fri, Sep 4, 2015 at 4:48 PM, Amit Kapila  wrote:
> On Thu, Sep 3, 2015 at 6:07 PM, Fujii Masao  wrote:
>>
>> On Tue, Aug 4, 2015 at 12:15 PM, Amit Kapila 
>> wrote:
>> > On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao 
>> > wrote:
>> >> ISTM that we can
>> >> see that the online backup mode has already been canceled if
>> >> backup_label
>> >> file
>> >> is successfully removed whether tablespace_map file remains or not. No?
>> >>
>> >
>> > I think what we should do is that display successful cancellation
>> > message
>> > only when both the files are renamed.
>>
>> Please imagine the case where backup_label was successfully renamed
>> but tablespace_map was not. Even in this case, I think that we can see
>> that the backup mode was canceled because the remaining tablespace_map
>> file will be ignored in the subsequent recovery.
>
> Right.
>
>>
>> So we should emit
>> the successful cancellation message when backup_label is renamed
>> whether tablespace_map is successfully renamed or not?
>>
>
> You mean to say, just try renaming tablespace_map and don't display any
> message whether that is successful or not-successful?
>
> I see some user inconvenience if we do this way, which is even after the
> backup is cancelled, on next recovery, there will be a log message
> indicating
> either rename of tablespace_map successful or unsuccessful.  Also, don't you
> think it is better to let user know that the tablespace_map file is
> successfully
> renamed as we do for backup_label file.  Shall we change the patch such that
> if backup_label is successfully renamed and renaming of tablespace_map
> gets failed, then display a log message to something like below:
>
> LOG:  online backup mode canceled
> DETAIL:  "backup_label" was renamed to "backup_label.old", could not rename
> "tablespace_map" to "tablespace_map.old"

Agreed with this direction. So what about the attached patch?

Regards,

-- 
Fujii Masao
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 127bc58..c2a1d51 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10911,32 +10911,32 @@ CancelBackup(void)
 {
 	struct stat stat_buf;
 
-	/* if the file is not there, return */
+	/* if the backup_label file is not there, return */
 	if (stat(BACKUP_LABEL_FILE, &stat_buf) < 0)
 		return;
 
 	/* remove leftover file from previously canceled backup if it exists */
 	unlink(BACKUP_LABEL_OLD);
 
-	if (rename(BACKUP_LABEL_FILE, BACKUP_LABEL_OLD) == 0)
-	{
-		ereport(LOG,
-(errmsg("online backup mode canceled"),
- errdetail("\"%s\" was renamed to \"%s\".",
-		   BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));
-	}
-	else
+	if (rename(BACKUP_LABEL_FILE, BACKUP_LABEL_OLD) != 0)
 	{
 		ereport(WARNING,
 (errcode_for_file_access(),
  errmsg("online backup mode was not canceled"),
- errdetail("Could not rename \"%s\" to \"%s\": %m.",
+ errdetail("\"%s\" could not be renamed to \"%s\": %m.",
 		   BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));
+		return;
 	}
 
 	/* if the tablespace_map file is not there, return */
 	if (stat(TABLESPACE_MAP, &stat_buf) < 0)
+	{
+		ereport(LOG,
+(errmsg("online backup mode canceled"),
+ errdetail("\"%s\" was renamed to \"%s\".",
+		   BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));
 		return;
+	}
 
 	/* remove leftover file from previously canceled backup if it exists */
 	unlink(TABLESPACE_MAP_OLD);
@@ -10945,15 +10945,19 @@ CancelBackup(void)
 	{
 		ereport(LOG,
 (errmsg("online backup mode canceled"),
- errdetail("\"%s\" was renamed to \"%s\".",
-		   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
+ errdetail("\"%s\" and \"%s\" were renamed to "
+		   "\"%s\" and \"%s\", respectively.",
+		   BACKUP_LABEL_FILE, TABLESPACE_MAP,
+		   BACKUP_LABEL_OLD, TABLESPACE_MAP_OLD)));
 	}
 	else
 	{
 		ereport(WARNING,
 (errcode_for_file_access(),
- errmsg("online backup mode was not canceled"),
- errdetail("Could not rename \"%s\" to \"%s\": %m.",
+ errmsg("online backup mode canceled"),
+ errdetail("\"%s\" was renamed to \"%s\", "
+		   "but \"%s\" could not be renamed to \"%s\": %m.",
+		   BACKUP_LABEL_FILE, BACKUP_LABEL_OLD,
 		   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
 	}
 }

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

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

[Amit Kapila]

On Thu, Sep 3, 2015 at 6:07 PM, Fujii Masao  wrote:
>
> On Tue, Aug 4, 2015 at 12:15 PM, Amit Kapila 
wrote:
> > On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao 
wrote:
> >> ISTM that we can
> >> see that the online backup mode has already been canceled if
backup_label
> >> file
> >> is successfully removed whether tablespace_map file remains or not. No?
> >>
> >
> > I think what we should do is that display successful cancellation
message
> > only when both the files are renamed.
>
> Please imagine the case where backup_label was successfully renamed
> but tablespace_map was not. Even in this case, I think that we can see
> that the backup mode was canceled because the remaining tablespace_map
> file will be ignored in the subsequent recovery.

Right.

>
> So we should emit
> the successful cancellation message when backup_label is renamed
> whether tablespace_map is successfully renamed or not?
>

You mean to say, just try renaming tablespace_map and don't display any
message whether that is successful or not-successful?

I see some user inconvenience if we do this way, which is even after the
backup is cancelled, on next recovery, there will be a log message
indicating
either rename of tablespace_map successful or unsuccessful.  Also, don't you
think it is better to let user know that the tablespace_map file is
successfully
renamed as we do for backup_label file.  Shall we change the patch such that
if backup_label is successfully renamed and renaming of tablespace_map
gets failed, then display a log message to something like below:

LOG:  online backup mode canceled
DETAIL:  "backup_label" was renamed to "backup_label.old", could not rename
"tablespace_map" to "tablespace_map.old"


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

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

[Fujii Masao]

On Sat, Aug 8, 2015 at 1:14 PM, Amit Kapila  wrote:
> On Tue, Aug 4, 2015 at 8:45 AM, Amit Kapila  wrote:
>>
>> On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao  wrote:
>>
>> > BTW, while reading the code related to tablespace_map, I found that
>> > CancelBackup() emits the WARNING message "online backup mode was not
>> > canceled"
>> > when rename() fails. Isn't this confusing (or incorrect)?
>>
>> Yes, it looks confusing.
>>
>
> Though this is *not* a major bug, still I feel it is better to do something
> about it.  So ideally, this should be tracked in 9.5 Open Items, but
> if you think otherwise, then also I think we should track it as part
> of CF for 9.6,  anybody having any opinion?

Since this is an item for 9.5, I think it's better to
add this to 9.5 open item list. So, I added it to the list.

Regards,

-- 
Fujii Masao


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

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

[Fujii Masao]

On Tue, Aug 4, 2015 at 12:15 PM, Amit Kapila  wrote:
> On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao  wrote:
>>
>>
>> Thanks! Pushed.
>>
>
> Thanks to you as well for committing the patch.
>
>> BTW, while reading the code related to tablespace_map, I found that
>> CancelBackup() emits the WARNING message "online backup mode was not
>> canceled"
>> when rename() fails. Isn't this confusing (or incorrect)?
>
> Yes, it looks confusing.
>
>> ISTM that we can
>> see that the online backup mode has already been canceled if backup_label
>> file
>> is successfully removed whether tablespace_map file remains or not. No?
>>
>
> I think what we should do is that display successful cancellation message
> only when both the files are renamed.

Please imagine the case where backup_label was successfully renamed
but tablespace_map was not. Even in this case, I think that we can see
that the backup mode was canceled because the remaining tablespace_map
file will be ignored in the subsequent recovery. So we should emit
the successful cancellation message when backup_label is renamed
whether tablespace_map is successfully renamed or not?

> I have drafted a patch (still I needs
> to verify/test it, I will do that if you think the fix is in right
> direction) to show
> what I have in mind.

Thanks for the patch!

-/* if the file is not there, return */
-if (stat(BACKUP_LABEL_FILE, &stat_buf) < 0)
+/* if the backup_label or tablespace_map file is not there, return */
+if (stat(BACKUP_LABEL_FILE, &stat_buf) < 0 ||
+stat(TABLESPACE_MAP, &stat_buf) < 0)
 return;

Seems problematic. This change always prevents the backup mode
from being canceled when there is no tablespace. Because in that case
tablespace_map file is not created when the backup mode is started,
and then the above condition is always true.

Regards,

-- 
Fujii Masao


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

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

[Amit Kapila]

On Tue, Aug 4, 2015 at 8:45 AM, Amit Kapila  wrote:
>
> On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao  wrote:
>
> > BTW, while reading the code related to tablespace_map, I found that
> > CancelBackup() emits the WARNING message "online backup mode was not
canceled"
> > when rename() fails. Isn't this confusing (or incorrect)?
>
> Yes, it looks confusing.
>

Though this is *not* a major bug, still I feel it is better to do something
about it.  So ideally, this should be tracked in 9.5 Open Items, but
if you think otherwise, then also I think we should track it as part
of CF for 9.6,  anybody having any opinion?


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

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

[Amit Kapila]

On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao  wrote:
>
>
> Thanks! Pushed.
>

Thanks to you as well for committing the patch.

> BTW, while reading the code related to tablespace_map, I found that
> CancelBackup() emits the WARNING message "online backup mode was not
canceled"
> when rename() fails. Isn't this confusing (or incorrect)?

Yes, it looks confusing.

> ISTM that we can
> see that the online backup mode has already been canceled if backup_label
file
> is successfully removed whether tablespace_map file remains or not. No?
>

I think what we should do is that display successful cancellation message
only when both the files are renamed.  I have drafted a patch (still I needs
to verify/test it, I will do that if you think the fix is in right
direction) to show
what I have in mind.


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


fix_rename_backup_and_mapfile_cancel_v1.patch
Description: Binary data

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

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

[Fujii Masao]

On Mon, Aug 3, 2015 at 8:55 PM, Amit Kapila  wrote:
> On Thu, Jul 16, 2015 at 9:58 PM, Robert Haas  wrote:
>>
>> On Thu, Jul 16, 2015 at 9:41 AM, Fujii Masao 
>> wrote:
>> > Here are some minor comments:
>> >
>> > +ereport(LOG,
>> > +(errmsg("ignoring \"%s\" file because no
>> > \"%s\" file exists",
>> > +TABLESPACE_MAP, BACKUP_LABEL_FILE),
>> > + errdetail("could not rename file \"%s\" to
>> > \"%s\": %m",
>> > +   TABLESPACE_MAP,
>> > TABLESPACE_MAP_OLD)));
>> >
>> > WARNING is better than LOG here because it indicates a problematic case?
>>
>> No, that's not the right distinction.  Remember that, when sending
>> messages to the client, WARNING > LOG, and when sending messages to
>> the log, LOG > WARNING.  So messages that a user is more likely to
>> care about than the administrator should be logged at WARNNG; those
>> that the administrator is more likely to care about should be LOG.  I
>> think LOG is clearly the appropriate thing here.
>>
>> > In detail message, the first word of sentence needs to be capitalized.
>> >
>> > + errdetail("renamed file \"%s\" to \"%s\"",
>> > +   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
>> >
>> > In detail message, basically we should use a complete sentence.
>> > So like other similar detail messages in xlog.c, I think that it's
>> > better
>> > to use "\"%s\" was renamed to \"%s\"." as the detail message here.
>>
>> Right, that's what the style guidelines say.
>>
>
> I have changed the errdetail messages as per above suggestion.
> Also, there was one issue (it was displaying 2 messages when
> rename fails) in patch which I have fixed in the updated version.

Thanks! Pushed.

BTW, while reading the code related to tablespace_map, I found that
CancelBackup() emits the WARNING message "online backup mode was not canceled"
when rename() fails. Isn't this confusing (or incorrect)? ISTM that we can
see that the online backup mode has already been canceled if backup_label file
is successfully removed whether tablespace_map file remains or not. No?

Regards,

-- 
Fujii Masao


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

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

[Amit Kapila]

On Thu, Jul 16, 2015 at 9:58 PM, Robert Haas  wrote:
>
> On Thu, Jul 16, 2015 at 9:41 AM, Fujii Masao 
wrote:
> > Here are some minor comments:
> >
> > +ereport(LOG,
> > +(errmsg("ignoring \"%s\" file because no
> > \"%s\" file exists",
> > +TABLESPACE_MAP, BACKUP_LABEL_FILE),
> > + errdetail("could not rename file \"%s\" to
> > \"%s\": %m",
> > +   TABLESPACE_MAP,
TABLESPACE_MAP_OLD)));
> >
> > WARNING is better than LOG here because it indicates a problematic case?
>
> No, that's not the right distinction.  Remember that, when sending
> messages to the client, WARNING > LOG, and when sending messages to
> the log, LOG > WARNING.  So messages that a user is more likely to
> care about than the administrator should be logged at WARNNG; those
> that the administrator is more likely to care about should be LOG.  I
> think LOG is clearly the appropriate thing here.
>
> > In detail message, the first word of sentence needs to be capitalized.
> >
> > + errdetail("renamed file \"%s\" to \"%s\"",
> > +   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
> >
> > In detail message, basically we should use a complete sentence.
> > So like other similar detail messages in xlog.c, I think that it's
better
> > to use "\"%s\" was renamed to \"%s\"." as the detail message here.
>
> Right, that's what the style guidelines say.
>

I have changed the errdetail messages as per above suggestion.
Also, there was one issue (it was displaying 2 messages when
rename fails) in patch which I have fixed in the updated version.


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


rename_mapfile_if_backupfile_not_present_v4.patch
Description: Binary data

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

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

[Amit Kapila]

On Thu, Jul 16, 2015 at 12:30 PM, Alvaro Herrera 
wrote:
>
> Amit Kapila wrote:
>
> > This can be tracked either in 9.5 Open Items or for next CF,
> > any opinions?
> >
> > If nobody else has any opinion on this, I will add it to 9.5 Open Items
> > list.
>
> I think this belongs in the open items list, yeah.
>

Okay, added to the list of 9.5 Open Items.


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

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

[Robert Haas]

On Thu, Jul 16, 2015 at 9:54 PM, Fujii Masao  wrote:
> Isn't this "rule" confusing the administrators?

I'd like to think not, but yeah, it probably is.  It is not like it
isn't documented.  There are even comments in postgresql.conf
explaining it.  But the fact that we have committers who are confused
probably isn't a great sign.

I really rather like the system we have and find it quite intuitive,
but it's obvious that a lot of other people don't.

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

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

[Fujii Masao]

On Fri, Jul 17, 2015 at 1:28 AM, Robert Haas  wrote:
> On Thu, Jul 16, 2015 at 9:41 AM, Fujii Masao  wrote:
>> Here are some minor comments:
>>
>> +ereport(LOG,
>> +(errmsg("ignoring \"%s\" file because no
>> \"%s\" file exists",
>> +TABLESPACE_MAP, BACKUP_LABEL_FILE),
>> + errdetail("could not rename file \"%s\" to
>> \"%s\": %m",
>> +   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
>>
>> WARNING is better than LOG here because it indicates a problematic case?
>
> No, that's not the right distinction.  Remember that, when sending
> messages to the client, WARNING > LOG, and when sending messages to
> the log, LOG > WARNING.  So messages that a user is more likely to
> care about than the administrator should be logged at WARNNG; those
> that the administrator is more likely to care about should be LOG.  I
> think LOG is clearly the appropriate thing here.

Isn't this "rule" confusing the administrators? ISTM that the administrators
would intuitively and literally pay more attention to WARNING than LOG.
Also there are already some warning messages with WARNING level that
the administrators rather than the clients should care about. For example,
the warning message which output when archive_command fails.

ereport(WARNING,
(errmsg("archiving transaction log file
\"%s\" failed too many times, will try again later",
xlog)));

Regards,

-- 
Fujii Masao


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

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

[Alvaro Herrera]

Robert Haas wrote:
> On Thu, Jul 16, 2015 at 9:41 AM, Fujii Masao  wrote:
> > Here are some minor comments:
> >
> > +ereport(LOG,
> > +(errmsg("ignoring \"%s\" file because no
> > \"%s\" file exists",
> > +TABLESPACE_MAP, BACKUP_LABEL_FILE),
> > + errdetail("could not rename file \"%s\" to
> > \"%s\": %m",
> > +   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
> >
> > WARNING is better than LOG here because it indicates a problematic case?
> 
> No, that's not the right distinction.  Remember that, when sending
> messages to the client, WARNING > LOG, and when sending messages to
> the log, LOG > WARNING.  So messages that a user is more likely to
> care about than the administrator should be logged at WARNNG; those
> that the administrator is more likely to care about should be LOG.  I
> think LOG is clearly the appropriate thing here.

Right.  Now that we have errors marked with criticality, it will be
pretty obvious what message is indicating a system problem rather than
just a problem that can be ignored without taking any action.

... oh, actually we don't have the criticality marking yet, do we.
I think we need to fix that at some point.  (Some guys have said to grep
the log for certain SQLSTATE codes, but, uh, really?)

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


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

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

[Robert Haas]

On Thu, Jul 16, 2015 at 9:41 AM, Fujii Masao  wrote:
> Here are some minor comments:
>
> +ereport(LOG,
> +(errmsg("ignoring \"%s\" file because no
> \"%s\" file exists",
> +TABLESPACE_MAP, BACKUP_LABEL_FILE),
> + errdetail("could not rename file \"%s\" to
> \"%s\": %m",
> +   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
>
> WARNING is better than LOG here because it indicates a problematic case?

No, that's not the right distinction.  Remember that, when sending
messages to the client, WARNING > LOG, and when sending messages to
the log, LOG > WARNING.  So messages that a user is more likely to
care about than the administrator should be logged at WARNNG; those
that the administrator is more likely to care about should be LOG.  I
think LOG is clearly the appropriate thing here.

> In detail message, the first word of sentence needs to be capitalized.
>
> + errdetail("renamed file \"%s\" to \"%s\"",
> +   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));
>
> In detail message, basically we should use a complete sentence.
> So like other similar detail messages in xlog.c, I think that it's better
> to use "\"%s\" was renamed to \"%s\"." as the detail message here.

Right, that's what the style guidelines say.

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

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

[Fujii Masao]

On Fri, Jul 3, 2015 at 12:15 PM, Amit Kapila  wrote:
> On Thu, Jul 2, 2015 at 7:44 PM, Alvaro Herrera 
> wrote:
>>
>> Amit Kapila wrote:
>> >
>> > Added the above log messages in attached patch with small change
>> > such that in message, file names will be displayed with quotes as most
>> > of other usages of rename (failure) in that file uses quotes to display
>> > filenames.
>>
>> Why emit two messages?
>
> not necessary.
>
>>  Can we reduce that to a single one?  Maybe the
>> first one could be errdetail or something.
>>
>
> I think it is better other way (basically have second one as errdetail).
> We already have one similar message in xlog.c that way.
> ereport(LOG,
> (errmsg("online backup mode canceled"),
> errdetail("\"%s\" was renamed to \"%s\".",
>   BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));
>
> Attached patch consolidates errmsg into one message.

Here are some minor comments:

+ereport(LOG,
+(errmsg("ignoring \"%s\" file because no
\"%s\" file exists",
+TABLESPACE_MAP, BACKUP_LABEL_FILE),
+ errdetail("could not rename file \"%s\" to
\"%s\": %m",
+   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));

WARNING is better than LOG here because it indicates a problematic case?

In detail message, the first word of sentence needs to be capitalized.

+ errdetail("renamed file \"%s\" to \"%s\"",
+   TABLESPACE_MAP, TABLESPACE_MAP_OLD)));

In detail message, basically we should use a complete sentence.
So like other similar detail messages in xlog.c, I think that it's better
to use "\"%s\" was renamed to \"%s\"." as the detail message here.

Regards,

-- 
Fujii Masao


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

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

[Alvaro Herrera]

Amit Kapila wrote:

> This can be tracked either in 9.5 Open Items or for next CF,
> any opinions?
> 
> If nobody else has any opinion on this, I will add it to 9.5 Open Items
> list.

I think this belongs in the open items list, yeah.

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


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

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

[Amit Kapila]

On Fri, Jul 3, 2015 at 8:45 AM, Amit Kapila  wrote:
>
> On Thu, Jul 2, 2015 at 7:44 PM, Alvaro Herrera 
wrote:
> >
> >  Can we reduce that to a single one?  Maybe the
> > first one could be errdetail or something.
> >
>
> I think it is better other way (basically have second one as errdetail).
> We already have one similar message in xlog.c that way.
> ereport(LOG,
> (errmsg("online backup mode canceled"),
> errdetail("\"%s\" was renamed to \"%s\".",
>   BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));
>
> Attached patch consolidates errmsg into one message.

This can be tracked either in 9.5 Open Items or for next CF,
any opinions?

If nobody else has any opinion on this, I will add it to 9.5 Open Items
list.


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

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

[Amit Kapila]

On Thu, Jul 2, 2015 at 7:44 PM, Alvaro Herrera 
wrote:
>
> Amit Kapila wrote:
> >
> > Added the above log messages in attached patch with small change
> > such that in message, file names will be displayed with quotes as most
> > of other usages of rename (failure) in that file uses quotes to display
> > filenames.
>
> Why emit two messages?

not necessary.

>  Can we reduce that to a single one?  Maybe the
> first one could be errdetail or something.
>

I think it is better other way (basically have second one as errdetail).
We already have one similar message in xlog.c that way.
ereport(LOG,
(errmsg("online backup mode canceled"),
errdetail("\"%s\" was renamed to \"%s\".",
  BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));

Attached patch consolidates errmsg into one message.


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


rename_mapfile_if_backupfile_not_present_v3.patch
Description: Binary data

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

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

[Alvaro Herrera]

Amit Kapila wrote:
> On Sat, Jun 27, 2015 at 12:54 AM, Robert Haas  wrote:
> >
> > On Mon, Jun 15, 2015 at 2:52 PM, Amit Kapila
> >  wrote:
> > > Attached patch provides a fix as per above discussion.
> >
> > I think we should emit some LOG messages here.  When we detect the
> > file is there:
> >
> > LOG: ignoring tablespace_map file because no backup_label file exists
> >
> > If the rename fails:
> >
> > LOG: could not rename file "%s" to "%s": %m
> >
> > If it works:
> >
> > LOG: renamed file "%s" to "%s"
> 
> Added the above log messages in attached patch with small change
> such that in message, file names will be displayed with quotes as most
> of other usages of rename (failure) in that file uses quotes to display
> filenames.

Why emit two messages?  Can we reduce that to a single one?  Maybe the
first one could be errdetail or something.

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


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

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

[Amit Kapila]

On Sat, Jun 27, 2015 at 12:54 AM, Robert Haas  wrote:
>
> On Mon, Jun 15, 2015 at 2:52 PM, Amit Kapila 
wrote:
> > Attached patch provides a fix as per above discussion.
>
> I think we should emit some LOG messages here.  When we detect the
> file is there:
>
> LOG: ignoring tablespace_map file because no backup_label file exists
>
> If the rename fails:
>
> LOG: could not rename file "%s" to "%s": %m
>
> If it works:
>
> LOG: renamed file "%s" to "%s"
>

Added the above log messages in attached patch with small change
such that in message, file names will be displayed with quotes as most
of other usages of rename (failure) in that file uses quotes to display
filenames.

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


rename_mapfile_if_backupfile_not_present_v2.patch
Description: Binary data

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

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

[Amit Kapila]

On Sat, Jun 27, 2015 at 1:32 AM, Robert Haas  wrote:
>
> On Wed, Jun 10, 2015 at 3:34 AM, Amit Kapila 
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\"",
>

Sorry, I think this is left over due to multiple versions exchange
between me and Andrew.

> But it looks OK otherwise, so committed.
>

Thanks.

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

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

[Robert Haas]

On Wed, Jun 10, 2015 at 3:34 AM, Amit Kapila  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


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

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

[Robert Haas]

On Mon, Jun 15, 2015 at 2:52 PM, Amit Kapila  wrote:
> Attached patch provides a fix as per above discussion.

I think we should emit some LOG messages here.  When we detect the
file is there:

LOG: ignoring tablespace_map file because no backup_label file exists

If the rename fails:

LOG: could not rename file "%s" to "%s": %m

If it works:

LOG: renamed file "%s" to "%s"

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

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

[Amit Kapila]

On Thu, Jun 11, 2015 at 9:55 AM, Amit Kapila 
wrote:
>
> On Wed, Jun 10, 2015 at 12:09 PM, Fujii Masao 
wrote:
> >
> > On Tue, Jun 9, 2015 at 3:29 PM, Amit Kapila 
wrote:
> > > On Tue, Jun 9, 2015 at 10:56 AM, Fujii Masao 
wrote:
> > >> Or what about removing tablespace_map file at the beginning of
recovery
> > >> whenever backup_label doesn't exist?
> > >
> > > Yes, thats another way, but is it safe to assume that user won't need
> > > that file,
> >
> > Is there really case where tablespace_map is necessary even though
backup_label
> > doesn't exist? If not, it seems safe to get rid of the file when
backup_label
> > is not present.
> >
> > > I mean in the valid scenario (where both backup_label and
> > > tablespace_map are present and usable) also, we rename them to
> > > *.old rather than deleting it.
> >
> > Yep, I'm OK to make the recovery rename the file to *.old rather than
delete it.
> >
>
> This sounds safe to me, unless anybody else has different opinion
> I will write a patch to fix this way.
>

Attached patch provides a fix as per above discussion.



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


rename_mapfile_if_backupfile_not_present_v1.patch
Description: Binary data

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

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

[Amit Kapila]

On Wed, Jun 10, 2015 at 12:09 PM, Fujii Masao  wrote:
>
> On Tue, Jun 9, 2015 at 3:29 PM, Amit Kapila 
wrote:
> > On Tue, Jun 9, 2015 at 10:56 AM, Fujii Masao 
wrote:
> >> Or what about removing tablespace_map file at the beginning of recovery
> >> whenever backup_label doesn't exist?
> >
> > Yes, thats another way, but is it safe to assume that user won't need
> > that file,
>
> Is there really case where tablespace_map is necessary even though
backup_label
> doesn't exist? If not, it seems safe to get rid of the file when
backup_label
> is not present.
>
> > I mean in the valid scenario (where both backup_label and
> > tablespace_map are present and usable) also, we rename them to
> > *.old rather than deleting it.
>
> Yep, I'm OK to make the recovery rename the file to *.old rather than
delete it.
>

This sounds safe to me, unless anybody else has different opinion
I will write a patch to fix this way.


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

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

[Amit Kapila]

On Tue, Jun 9, 2015 at 8:37 PM, Andrew Dunstan  wrote:

>
> On 06/08/2015 11:19 PM, Amit Kapila wrote:
>
>>
>> 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.
>

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.


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


remove_only_symlinks_during_recovery_v4.patch
Description: Binary data

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

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

[Fujii Masao]

On Tue, Jun 9, 2015 at 3:29 PM, Amit Kapila  wrote:
> On Tue, Jun 9, 2015 at 10:56 AM, Fujii Masao  wrote:
>>
>> On Tue, Jun 9, 2015 at 1:04 PM, Amit Kapila 
>> wrote:
>> > On Tue, Jun 9, 2015 at 9:09 AM, Fujii Masao 
>> > wrote:
>> >>
>> >> On Tue, May 12, 2015 at 10:42 PM, Andrew Dunstan 
>> >> wrote:
>> >> > Map basebackup tablespaces using a tablespace_map file
>> >> >
>> >> > Windows can't reliably restore symbolic links from a tar format, so
>> >> > instead during backup start we create a tablespace_map file, which is
>> >> > used by the restoring postgres to create the correct links in
>> >> > pg_tblspc.
>> >> > The backup protocol also now has an option to request this file to be
>> >> > included in the backup stream, and this is used by pg_basebackup when
>> >> > operating in tar mode.
>> >> >
>> >> > This is done on all platforms, not just Windows.
>> >> >
>> >> > This means that pg_basebackup will not not work in tar mode against
>> >> > 9.4
>> >> > and older servers, as this protocol option isn't implemented there.
>> >>
>> >> While I was performing the recovery-related tests, I found that there
>> >> was
>> >> the case where $PGDATA/tablespace_map was not renamed to *.old at the
>> >> recovery.
>> >> Is this harmless and intentional?
>> >
>> > There shouldn't be any problem because we tablespace_map file
>> > only if backup file is present.
>> >
>> >> Sorry if this has been already
>> >> discussed so far.
>> >>
>> >> The steps to reproduce the problem is:
>> >>
>> >> 1. start base backup, i.e., call pg_start_backup().
>> >> 2. repeat some write transactions and checkpoints until the WAL file
>> >> containing
>> >> the checkpoint record that backup_label indicates will be removed.
>> >> 3. killall -9 postgres
>> >> 4. start the server and a crash recovery.
>> >>
>> >> At this time, the crash recovery fails with the following error
>> >> messages.
>> >> 2015-06-09 12:26:54 JST FATAL:  could not locate required checkpoint
>> >> record
>> >> 2015-06-09 12:26:54 JST HINT:  If you are not restoring from a backup,
>> >> try removing the file ".../backup_label".
>> >>
>> >> 5. according to the above hint message, remove $PGDATA/backup_label
>> >> and restart a crash recovery
>> >>
>> >> Then you can see that tablespace_map remains in $PGDATA after the
>> >> recovery
>> >> ends.
>> >>
>> >
>> > The basic idea is that tablespace_map file will be used in case we
>> > have to restore from a backup which contains tablespaces.  So
>> > I think if user is manually removing backup_label, there is no
>> > meaning of tablespace_map, so that should also be removed. One
>> > way to address this is modify the Hint message suggesting that
>> > remove tablespace_map if present.
>> >
>> > Current :
>> > If you are not restoring from a backup, try removing the file
>> > \"%s/backup_label\
>> > Modify it to:
>> > If you are not restoring from a backup, try removing the file
>> > \"%s/backup_label\
>> > and, if present \"%s/tablespace_map\.
>>
>> Or what about removing tablespace_map file at the beginning of recovery
>> whenever backup_label doesn't exist?
>
> Yes, thats another way, but is it safe to assume that user won't need
> that file,

Is there really case where tablespace_map is necessary even though backup_label
doesn't exist? If not, it seems safe to get rid of the file when backup_label
is not present.

> I mean in the valid scenario (where both backup_label and
> tablespace_map are present and usable) also, we rename them to
> *.old rather than deleting it.

Yep, I'm OK to make the recovery rename the file to *.old rather than delete it.

> Yet another way could be we return error if tablespace_map is
> present whenever backup_label doesn't exist?

This needs another user operation, i.e., if a user wants to restart the server
at this case, he or she needs to remove (or rename) the file manually.

Regards,

-- 
Fujii Masao


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

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

[Andrew Dunstan]

On 06/08/2015 11:19 PM, Amit Kapila wrote:
On Tue, Jun 9, 2015 at 12:27 AM, Andrew Dunstan > 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



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

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

[Amit Kapila]

On Tue, Jun 9, 2015 at 10:56 AM, Fujii Masao  wrote:
>
> On Tue, Jun 9, 2015 at 1:04 PM, Amit Kapila 
wrote:
> > On Tue, Jun 9, 2015 at 9:09 AM, Fujii Masao 
wrote:
> >>
> >> On Tue, May 12, 2015 at 10:42 PM, Andrew Dunstan 
> >> wrote:
> >> > Map basebackup tablespaces using a tablespace_map file
> >> >
> >> > Windows can't reliably restore symbolic links from a tar format, so
> >> > instead during backup start we create a tablespace_map file, which is
> >> > used by the restoring postgres to create the correct links in
pg_tblspc.
> >> > The backup protocol also now has an option to request this file to be
> >> > included in the backup stream, and this is used by pg_basebackup when
> >> > operating in tar mode.
> >> >
> >> > This is done on all platforms, not just Windows.
> >> >
> >> > This means that pg_basebackup will not not work in tar mode against
9.4
> >> > and older servers, as this protocol option isn't implemented there.
> >>
> >> While I was performing the recovery-related tests, I found that there
was
> >> the case where $PGDATA/tablespace_map was not renamed to *.old at the
> >> recovery.
> >> Is this harmless and intentional?
> >
> > There shouldn't be any problem because we tablespace_map file
> > only if backup file is present.
> >
> >> Sorry if this has been already
> >> discussed so far.
> >>
> >> The steps to reproduce the problem is:
> >>
> >> 1. start base backup, i.e., call pg_start_backup().
> >> 2. repeat some write transactions and checkpoints until the WAL file
> >> containing
> >> the checkpoint record that backup_label indicates will be removed.
> >> 3. killall -9 postgres
> >> 4. start the server and a crash recovery.
> >>
> >> At this time, the crash recovery fails with the following error
messages.
> >> 2015-06-09 12:26:54 JST FATAL:  could not locate required checkpoint
> >> record
> >> 2015-06-09 12:26:54 JST HINT:  If you are not restoring from a backup,
> >> try removing the file ".../backup_label".
> >>
> >> 5. according to the above hint message, remove $PGDATA/backup_label
> >> and restart a crash recovery
> >>
> >> Then you can see that tablespace_map remains in $PGDATA after the
recovery
> >> ends.
> >>
> >
> > The basic idea is that tablespace_map file will be used in case we
> > have to restore from a backup which contains tablespaces.  So
> > I think if user is manually removing backup_label, there is no
> > meaning of tablespace_map, so that should also be removed. One
> > way to address this is modify the Hint message suggesting that
> > remove tablespace_map if present.
> >
> > Current :
> > If you are not restoring from a backup, try removing the file
> > \"%s/backup_label\
> > Modify it to:
> > If you are not restoring from a backup, try removing the file
> > \"%s/backup_label\
> > and, if present \"%s/tablespace_map\.
>
> Or what about removing tablespace_map file at the beginning of recovery
> whenever backup_label doesn't exist?

Yes, thats another way, but is it safe to assume that user won't need
that file, I mean in the valid scenario (where both backup_label and
tablespace_map are present and usable) also, we rename them to
*.old rather than deleting it.

Yet another way could be we return error if tablespace_map is
present whenever backup_label doesn't exist?

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

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

[Fujii Masao]

On Tue, Jun 9, 2015 at 1:04 PM, Amit Kapila  wrote:
> On Tue, Jun 9, 2015 at 9:09 AM, Fujii Masao  wrote:
>>
>> On Tue, May 12, 2015 at 10:42 PM, Andrew Dunstan 
>> wrote:
>> > Map basebackup tablespaces using a tablespace_map file
>> >
>> > Windows can't reliably restore symbolic links from a tar format, so
>> > instead during backup start we create a tablespace_map file, which is
>> > used by the restoring postgres to create the correct links in pg_tblspc.
>> > The backup protocol also now has an option to request this file to be
>> > included in the backup stream, and this is used by pg_basebackup when
>> > operating in tar mode.
>> >
>> > This is done on all platforms, not just Windows.
>> >
>> > This means that pg_basebackup will not not work in tar mode against 9.4
>> > and older servers, as this protocol option isn't implemented there.
>>
>> While I was performing the recovery-related tests, I found that there was
>> the case where $PGDATA/tablespace_map was not renamed to *.old at the
>> recovery.
>> Is this harmless and intentional?
>
> There shouldn't be any problem because we tablespace_map file
> only if backup file is present.
>
>> Sorry if this has been already
>> discussed so far.
>>
>> The steps to reproduce the problem is:
>>
>> 1. start base backup, i.e., call pg_start_backup().
>> 2. repeat some write transactions and checkpoints until the WAL file
>> containing
>> the checkpoint record that backup_label indicates will be removed.
>> 3. killall -9 postgres
>> 4. start the server and a crash recovery.
>>
>> At this time, the crash recovery fails with the following error messages.
>> 2015-06-09 12:26:54 JST FATAL:  could not locate required checkpoint
>> record
>> 2015-06-09 12:26:54 JST HINT:  If you are not restoring from a backup,
>> try removing the file ".../backup_label".
>>
>> 5. according to the above hint message, remove $PGDATA/backup_label
>> and restart a crash recovery
>>
>> Then you can see that tablespace_map remains in $PGDATA after the recovery
>> ends.
>>
>
> The basic idea is that tablespace_map file will be used in case we
> have to restore from a backup which contains tablespaces.  So
> I think if user is manually removing backup_label, there is no
> meaning of tablespace_map, so that should also be removed. One
> way to address this is modify the Hint message suggesting that
> remove tablespace_map if present.
>
> Current :
> If you are not restoring from a backup, try removing the file
> \"%s/backup_label\
> Modify it to:
> If you are not restoring from a backup, try removing the file
> \"%s/backup_label\
> and, if present \"%s/tablespace_map\.

Or what about removing tablespace_map file at the beginning of recovery
whenever backup_label doesn't exist? I'd like to avoid making a user do
such manual operation if possible.

Regards,

-- 
Fujii Masao


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

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

[Amit Kapila]

On Tue, Jun 9, 2015 at 9:09 AM, Fujii Masao  wrote:
>
> On Tue, May 12, 2015 at 10:42 PM, Andrew Dunstan 
wrote:
> > Map basebackup tablespaces using a tablespace_map file
> >
> > Windows can't reliably restore symbolic links from a tar format, so
> > instead during backup start we create a tablespace_map file, which is
> > used by the restoring postgres to create the correct links in pg_tblspc.
> > The backup protocol also now has an option to request this file to be
> > included in the backup stream, and this is used by pg_basebackup when
> > operating in tar mode.
> >
> > This is done on all platforms, not just Windows.
> >
> > This means that pg_basebackup will not not work in tar mode against 9.4
> > and older servers, as this protocol option isn't implemented there.
>
> While I was performing the recovery-related tests, I found that there was
> the case where $PGDATA/tablespace_map was not renamed to *.old at the
recovery.
> Is this harmless and intentional?

There shouldn't be any problem because we tablespace_map file
only if backup file is present.

> Sorry if this has been already
> discussed so far.
>
> The steps to reproduce the problem is:
>
> 1. start base backup, i.e., call pg_start_backup().
> 2. repeat some write transactions and checkpoints until the WAL file
containing
> the checkpoint record that backup_label indicates will be removed.
> 3. killall -9 postgres
> 4. start the server and a crash recovery.
>
> At this time, the crash recovery fails with the following error messages.
> 2015-06-09 12:26:54 JST FATAL:  could not locate required checkpoint
record
> 2015-06-09 12:26:54 JST HINT:  If you are not restoring from a backup,
> try removing the file ".../backup_label".
>
> 5. according to the above hint message, remove $PGDATA/backup_label
> and restart a crash recovery
>
> Then you can see that tablespace_map remains in $PGDATA after the
recovery ends.
>

The basic idea is that tablespace_map file will be used in case we
have to restore from a backup which contains tablespaces.  So
I think if user is manually removing backup_label, there is no
meaning of tablespace_map, so that should also be removed. One
way to address this is modify the Hint message suggesting that
remove tablespace_map if present.

Current :
If you are not restoring from a backup, try removing the file
\"%s/backup_label\
Modify it to:
If you are not restoring from a backup, try removing the file
\"%s/backup_label\
and, if present \"%s/tablespace_map\.


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

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

[Amit Kapila]

On Tue, Jun 9, 2015 at 12:27 AM, Andrew Dunstan  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?


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

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

[Andrew Dunstan]

On 06/08/2015 11:16 AM, Amit Kapila wrote:
On Mon, Jun 8, 2015 at 6:39 PM, Andrew Dunstan > 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


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

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

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

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

[Robert Haas]

On Mon, Jun 8, 2015 at 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.
>
>> 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


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

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

[Amit Kapila]

On Mon, Jun 8, 2015 at 6:39 PM, Andrew Dunstan  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.


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


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

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

[Andrew Dunstan]

On 06/08/2015 12:08 AM, Amit Kapila wrote:
On Mon, Jun 8, 2015 at 5:52 AM, Andrew Dunstan > 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


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

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

[Amit Kapila]

On Mon, Jun 8, 2015 at 5:52 AM, Andrew Dunstan  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?


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

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

[Andrew Dunstan]

On 06/05/2015 11:08 PM, Amit Kapila wrote:
On Fri, Jun 5, 2015 at 10:51 AM, Amit Kapila > wrote:


On Fri, Jun 5, 2015 at 9:57 AM, Andrew Dunstan
mailto:and...@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


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

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

[Amit Kapila]

On Fri, Jun 5, 2015 at 10:51 AM, Amit Kapila 
wrote:

> On Fri, Jun 5, 2015 at 9:57 AM, Andrew Dunstan 
> 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?


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

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

[Amit Kapila]

On Fri, Jun 5, 2015 at 9:57 AM, Andrew Dunstan  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?



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

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

[Andrew Dunstan]

On 06/04/2015 11:35 PM, Amit Kapila wrote:
On Fri, Jun 5, 2015 at 7:29 AM, Andrew Dunstan > 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


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

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

[Amit Kapila]

On Fri, Jun 5, 2015 at 7:29 AM, Andrew Dunstan  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().



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

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

[Andrew Dunstan]

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
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 666fa37..8d75d0c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -38,6 +38,7 @@
 #include "catalog/catversion.h"
 #include "catalog/pg_control.h"
 #include "catalog/pg_database.h"
+#include "commands/tablespace.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "postmaster/bgwriter.h"
@@ -6094,7 +6095,6 @@ StartupXLOG(void)
 		if (read_tablespace_map(&tablespaces))
 		{
 			ListCell   *lc;
-			struct stat st;
 
 			foreach(lc, tablespaces)
 			{
@@ -6105,27 +6105,9 @@ StartupXLOG(void)
 
 /*
  * 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.
+ * under PGDATA.
  */
-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)));
-}
+remove_tablespace_symlink(linkloc);
 
 if (symlink(ti->path, linkloc) < 0)
 	ereport(ERROR,
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 4ec1aff..e8acf27 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -627,31 +627,9 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid)
 
 	/*
 	 * In recovery, remove old symlink, in case it points to the wrong place.
-	 *
-	 * On Windows, junction points act like directories so we must be able to
-	 * apply rmdir; in general it seems best to make this code work like the
-	 * symlink removal code in destroy_tablespace_directories, except that
-	 * failure to remove is always an ERROR.
 	 */
 	if (InRecovery)
-	{
-		if (lstat(linkloc, &st) == 0 && S_ISDIR(st.st_mode))
-		{
-			if (rmdir(linkloc) < 0)
-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)));
-		}
-	}
+		remove_tablespace_symlink(linkloc);
 
 	/*
 	 * Create the symlink under PGDATA
@@ -848,6 +826,61 @@ directory_is_empty(const char *path)
 	return true;
 }
 
+/*
+ *	remove_tablespace_symlink
+ *
+ * This function removes symlinks in pg_tblspc.  On Windows, junction points
+ * act like directories so we must be able to apply rmdir.  This function
+ * works like the symlink removal code in destroy_tablespace_directories,
+ * except that failure to remove is always an ERROR.
+ */
+void
+remove_tablespace_symlink(const char *linkloc)
+{
+	struct stat st;
+
+	if (lstat(linkloc, &st) != 0)
+	{
+		if (errno == ENOENT)
+			return;
+		else
+			ereport(ERROR,
+	(errcode_for_file_access(),
+	 errmsg("could not stat \"%s\": %m",
+			linkloc)));
+	}
+
+	if (S_ISDIR(st.st_mode))
+	{
+		/*
+		 * This will fail if the directory isn't empty, but not
+		 * if it's a junction point.
+		 */
+		if (rmdir(linkloc) < 0)
+			ereport(ERROR,
+	(errcode_for_file_access(),
+	 errmsg("could not remove directory \"%s\": %m",
+			linkloc)));
+	}
+#ifdef S_ISLNK
+	else if (S_ISLNK(st.st_mode))
+	{
+		if (unlink(linkloc) < 0 && errno != ENOENT)
+			ereport(ERROR,
+	(errcode_for_file_access(),
+		errmsg("could not remove symbolic link \"%s\": %m",
+			linkloc)));
+	}
+#endif
+	else
+	{
+		/* Refuse to remove anything that's not a directory or symlink */
+		ereport(ERROR,
+(ERRCODE_SYSTEM_ERROR,
+ errmsg("Not a  directory or symbolic link: \"%s\"",
+		linkloc)));
+	}
+}
 
 /*
  * Rename a tablespace
diff --git a/src/include/commands/tablespace.h b/src/include/commands/tablespace.h
index 86b0477..6b928a5 100644
--- a/src/include/command

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

[Amit Kapila]

On Thu, Jun 4, 2015 at 8:43 PM, Andrew Dunstan  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


remove_only_symlinks_during_recovery_v2.patch
Description: Binary data

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

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

[Andrew Dunstan]

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

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

[Amit Kapila]

On Thu, Jun 4, 2015 at 10:14 AM, Amit Kapila 
wrote:

> On Thu, Jun 4, 2015 at 1:52 AM, Andrew Dunstan 
> wrote:
>
>>
>> On 06/02/2015 11:55 PM, Amit Kapila wrote:
>>
>>  On Tue, Jun 2, 2015 at 10:26 PM, Andrew Dunstan >> > 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


remove_only_symlinks_during_recovery_v2.patch
Description: Binary data

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

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

[Amit Kapila]

On Thu, Jun 4, 2015 at 1:52 AM, Andrew Dunstan  wrote:

>
> On 06/02/2015 11:55 PM, Amit Kapila wrote:
>
>  On Tue, Jun 2, 2015 at 10:26 PM, Andrew Dunstan > > 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

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

[Andrew Dunstan]

On 06/02/2015 11:55 PM, Amit Kapila wrote:
On Tue, Jun 2, 2015 at 10:26 PM, Andrew Dunstan > 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

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

[Amit Kapila]

On Tue, Jun 2, 2015 at 10:26 PM, Andrew Dunstan  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

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

[Andrew Dunstan]

On 05/15/2015 02:21 AM, Amit Kapila wrote:
On Thu, May 14, 2015 at 10:29 PM, Andrew Dunstan > wrote:

>
>
> On 05/14/2015 10:52 AM, Robert Haas wrote:
>>
>> On Thu, May 14, 2015 at 12:12 AM, Amit Kapila 
mailto:amit.kapil...@gmail.com>> wrote:

>>>
>>> On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan 
mailto:and...@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

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

[Amit Kapila]

On Sat, May 23, 2015 at 9:28 PM, Andrew Dunstan  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

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

[Andrew Dunstan]

On 05/23/2015 01:29 AM, Amit Kapila wrote:
On Fri, May 15, 2015 at 11:51 AM, Amit Kapila > wrote:

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

> >
> >
> > On 05/14/2015 10:52 AM, Robert Haas wrote:
> >>
> >> On Thu, May 14, 2015 at 12:12 AM, Amit Kapila 
mailto:amit.kapil...@gmail.com>> wrote:

> >>>
> >>> On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan 
mailto:and...@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

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

[Amit Kapila]

On Fri, May 15, 2015 at 11:51 AM, Amit Kapila 
wrote:
>
> On Thu, May 14, 2015 at 10:29 PM, Andrew Dunstan 
wrote:
> >
> >
> > On 05/14/2015 10:52 AM, Robert Haas wrote:
> >>
> >> On Thu, May 14, 2015 at 12:12 AM, Amit Kapila 
wrote:
> >>>
> >>> On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan 
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

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

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

[Amit Kapila]

On Thu, May 14, 2015 at 10:29 PM, Andrew Dunstan 
wrote:
>
>
> On 05/14/2015 10:52 AM, Robert Haas wrote:
>>
>> On Thu, May 14, 2015 at 12:12 AM, Amit Kapila 
wrote:
>>>
>>> On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan 
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


remove_only_symlinks_during_recovery_v1.patch
Description: Binary data

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

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

[Robert Haas]

On Thu, May 14, 2015 at 12:59 PM, Andrew Dunstan  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

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

[Andrew Dunstan]

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

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

On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan  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

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

[Robert Haas]

On Thu, May 14, 2015 at 12:12 AM, Amit Kapila  wrote:
> On Thu, May 14, 2015 at 2:10 AM, Andrew Dunstan  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