Обсуждение: 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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
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?
>
> 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.
>
> 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\.
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
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?
>
> 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, 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?
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
On Wed, Jun 10, 2015 at 12:09 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> 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:
> >> 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.
>
>
> 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:
> >> 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.
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
On Thu, Jun 11, 2015 at 9:55 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Jun 10, 2015 at 12:09 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > 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:
> > >> 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.
>
> On Wed, Jun 10, 2015 at 12:09 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > 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:
> > >> 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.
Вложения
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Robert Haas
Дата:
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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
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"
>
>
> 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.
Вложения
Re: 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 <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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
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?
I think it is better other way (basically have second one as errdetail).
(errmsg("online backup mode canceled"),
errdetail("\"%s\" was renamed to \"%s\".",
BACKUP_LABEL_FILE, BACKUP_LABEL_OLD)));
Attached patch consolidates errmsg into one message.
>
> 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.
>
> 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.
Вложения
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
On Fri, Jul 3, 2015 at 8:45 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Jul 2, 2015 at 7:44 PM, Alvaro Herrera <alvherre@2ndquadrant.com> 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.
>
> On Thu, Jul 2, 2015 at 7:44 PM, Alvaro Herrera <alvherre@2ndquadrant.com> 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.
Re: 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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Fujii Masao
Дата:
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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Robert Haas
Дата:
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
Re: 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 <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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Fujii Masao
Дата:
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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Robert Haas
Дата:
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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
On Thu, Jul 16, 2015 at 12:30 PM, Alvaro Herrera <alvherre@2ndquadrant.com> 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.
>
> 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.
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
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.
>
> 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.
Вложения
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Fujii Masao
Дата:
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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>
>
> 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)?
>
>
> 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
> 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.
Вложения
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
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.
>
>
> 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?
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
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?
With Regards,
Amit Kapila.
>
> 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"
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
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
Вложения
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
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?
>
>
> 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.
Other than that patch looks good.
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
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.
>
> 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.
> -------------------------
>
>
> -------------------------
> 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.
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Robert Haas
Дата:
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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Robert Haas
Дата:
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
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Fujii Masao
Дата:
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
Вложения
Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
Amit Kapila
Дата:
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.
With Regards,
Amit Kapila.
>
> 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: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
От
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