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

On Tue, May 12, 2015 at 10:42 PM, Andrew Dunstan <andrew@dunslane.net> 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? 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_labelindicates 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.

Regards,

-- 
Fujii Masao

On Tue, Jun 9, 2015 at 1:04 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Jun 9, 2015 at 9:09 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> On Tue, May 12, 2015 at 10:42 PM, Andrew Dunstan <andrew@dunslane.net>
>> 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

On Tue, Jun 9, 2015 at 3:29 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Jun 9, 2015 at 10:56 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> On Tue, Jun 9, 2015 at 1:04 PM, Amit Kapila <amit.kapila16@gmail.com>
>> wrote:
>> > On Tue, Jun 9, 2015 at 9:09 AM, Fujii Masao <masao.fujii@gmail.com>
>> > wrote:
>> >>
>> >> On Tue, May 12, 2015 at 10:42 PM, Andrew Dunstan <andrew@dunslane.net>
>> >> 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

On Mon, Jun 15, 2015 at 2:52 PM, Amit Kapila <amit.kapila16@gmail.com> 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

Amit Kapila wrote:
> On Sat, Jun 27, 2015 at 12:54 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Mon, Jun 15, 2015 at 2:52 PM, Amit Kapila
> > <amit.kapila16@gmail.com> 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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Fri, Jul 3, 2015 at 12:15 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Jul 2, 2015 at 7:44 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
> 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

On Thu, Jul 16, 2015 at 9:41 AM, Fujii Masao <masao.fujii@gmail.com> 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

Robert Haas wrote:
> On Thu, Jul 16, 2015 at 9:41 AM, Fujii Masao <masao.fujii@gmail.com> 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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jul 17, 2015 at 1:28 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Jul 16, 2015 at 9:41 AM, Fujii Masao <masao.fujii@gmail.com> 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

On Thu, Jul 16, 2015 at 9:54 PM, Fujii Masao <masao.fujii@gmail.com> 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

On Mon, Aug 3, 2015 at 8:55 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Jul 16, 2015 at 9:58 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Thu, Jul 16, 2015 at 9:41 AM, Fujii Masao <masao.fujii@gmail.com>
>> 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

On Tue, Aug 4, 2015 at 12:15 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao <masao.fujii@gmail.com> 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

On Sat, Aug 8, 2015 at 1:14 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Aug 4, 2015 at 8:45 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao <masao.fujii@gmail.com> 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

On Fri, Sep 4, 2015 at 4:48 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Sep 3, 2015 at 6:07 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> On Tue, Aug 4, 2015 at 12:15 PM, Amit Kapila <amit.kapila16@gmail.com>
>> wrote:
>> > On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao <masao.fujii@gmail.com>
>> > 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

On Thu, Sep 10, 2015 at 12:49 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Sep 9, 2015 at 6:43 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> On Fri, Sep 4, 2015 at 4:48 PM, Amit Kapila <amit.kapila16@gmail.com>
>> 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

On Thu, Sep 10, 2015 at 1:08 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Sep 10, 2015 at 9:29 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> On Thu, Sep 10, 2015 at 12:49 PM, Amit Kapila <amit.kapila16@gmail.com>
>> wrote:
>> > On Wed, Sep 9, 2015 at 6:43 PM, Fujii Masao <masao.fujii@gmail.com>
>> > wrote:
>> >>
>> >> On Fri, Sep 4, 2015 at 4:48 PM, Amit Kapila <amit.kapila16@gmail.com>
>> >> 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

On Wed, Sep 9, 2015 at 11:49 PM, Amit Kapila <amit.kapila16@gmail.com> 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

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Sep 9, 2015 at 11:49 PM, Amit Kapila <amit.kapila16@gmail.com> 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

On Thu, Sep 10, 2015 at 3:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, Sep 9, 2015 at 11:49 PM, Amit Kapila <amit.kapila16@gmail.com> 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

On Fri, Sep 11, 2015 at 5:43 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Sep 10, 2015 at 3:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Wed, Sep 9, 2015 at 11:49 PM, Amit Kapila <amit.kapila16@gmail.com> 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

On Fri, Sep 11, 2015 at 1:57 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Sep 11, 2015 at 10:10 AM, Fujii Masao <masao.fujii@gmail.com> 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