Thread: Re: [DOCS] pg_xlogdump -p option correction
On Tue, Dec 27, 2016 at 6:56 AM, Bruce Momjian <bruce@momjian.us> wrote:
> I have found that pg_xlogdump looks for WAL files in the in the current
> directory and the pg_wal subdirectory of both the current directory and
> the PGDATA directory. Doc patch attached.
The whole behavior is listed in pg_xlogdump.c here:
/*
* Try to find the file in several places:
* if directory == NULL:
* fname
* XLOGDIR / fname
* $PGDATA / XLOGDIR / fname
* else
* directory / fname
* directory / XLOGDIR / fname
*
* return a read only fd
*/
static int
fuzzy_open_file(const char *directory, const char *fname)
Updating the docs the way your patch does does not look enough IMO:
1) pg_xlogdump --help should mention PGDATA as well for the default
behavior, and should make clear the default and when -p is defined:
-p, --path=PATH directory in which to find log segment files
(default: ./pg_wal)
2) The documentation does not mention that if a directory is defined
pg_xlogdump will try to look as well at defined_dir/pg_wal.
--
Michael
On Tue, Dec 27, 2016 at 01:28:17PM +0900, Michael Paquier wrote:
> On Tue, Dec 27, 2016 at 6:56 AM, Bruce Momjian <bruce@momjian.us> wrote:
> > I have found that pg_xlogdump looks for WAL files in the in the current
> > directory and the pg_wal subdirectory of both the current directory and
> > the PGDATA directory. Doc patch attached.
>
> The whole behavior is listed in pg_xlogdump.c here:
> /*
> * Try to find the file in several places:
> * if directory == NULL:
> * fname
> * XLOGDIR / fname
> * $PGDATA / XLOGDIR / fname
> * else
> * directory / fname
> * directory / XLOGDIR / fname
> *
> * return a read only fd
> */
> static int
> fuzzy_open_file(const char *directory, const char *fname)
>
> Updating the docs the way your patch does does not look enough IMO:
OK.
> 1) pg_xlogdump --help should mention PGDATA as well for the default
> behavior, and should make clear the default and when -p is defined:
> -p, --path=PATH directory in which to find log segment files
> (default: ./pg_wal)
Ah, good point. The new output is:
-p, --path=PATH directory in which to find log segment files
(default: current directory, ./pg_wal, $PGDATA/pg_wal)
> 2) The documentation does not mention that if a directory is defined
> pg_xlogdump will try to look as well at defined_dir/pg_wal.
Uh, I think it does in the first sentence:
Directory in which to find log segment files.
Updated patch attached.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +
Attachment
On Sat, Jan 7, 2017 at 10:02 AM, Bruce Momjian <bruce@momjian.us> wrote: >> 1) pg_xlogdump --help should mention PGDATA as well for the default >> behavior, and should make clear the default and when -p is defined: >> -p, --path=PATH directory in which to find log segment files >> (default: ./pg_wal) > > Ah, good point. The new output is: > > -p, --path=PATH directory in which to find log segment files > (default: current directory, ./pg_wal, $PGDATA/pg_wal) Looks good, thanks. >> 2) The documentation does not mention that if a directory is defined >> pg_xlogdump will try to look as well at defined_dir/pg_wal. > > Uh, I think it does in the first sentence: > > Directory in which to find log segment files. I am reading that as "only the current directory", not "the current directory, then current directory + pg_wal". Do you think that this sentence implies that the check routine looks as well at current_dir/pg_wal? > Updated patch attached. + subdirectory of both the current directory and the + <envar>PGDATA</envar> directory. This could say "of both the current directory and *then* the PGDATA directory" to outline the order of the actions taken by the check routine. Just my 2c on the matter. -- Michael
On Sat, Jan 7, 2017 at 09:50:32PM +0900, Michael Paquier wrote: > >> 2) The documentation does not mention that if a directory is defined > >> pg_xlogdump will try to look as well at defined_dir/pg_wal. > > > > Uh, I think it does in the first sentence: > > > > Directory in which to find log segment files. > > I am reading that as "only the current directory", not "the current > directory, then current directory + pg_wal". Do you think that this > sentence implies that the check routine looks as well at > current_dir/pg_wal? > > > Updated patch attached. > > + subdirectory of both the current directory and the > + <envar>PGDATA</envar> directory. > This could say "of both the current directory and *then* the PGDATA > directory" to outline the order of the actions taken by the check > routine. Just my 2c on the matter. Ah, I see your point. I ended up rewording the text to be more explicit about the supplied argument and "searching". Updated patch attached. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Attachment
On Sun, Jan 8, 2017 at 1:20 AM, Bruce Momjian <bruce@momjian.us> wrote: > On Sat, Jan 7, 2017 at 09:50:32PM +0900, Michael Paquier wrote: >> >> 2) The documentation does not mention that if a directory is defined >> >> pg_xlogdump will try to look as well at defined_dir/pg_wal. >> > >> > Uh, I think it does in the first sentence: >> > >> > Directory in which to find log segment files. >> >> I am reading that as "only the current directory", not "the current >> directory, then current directory + pg_wal". Do you think that this >> sentence implies that the check routine looks as well at >> current_dir/pg_wal? >> >> > Updated patch attached. >> >> + subdirectory of both the current directory and the >> + <envar>PGDATA</envar> directory. >> This could say "of both the current directory and *then* the PGDATA >> directory" to outline the order of the actions taken by the check >> routine. Just my 2c on the matter. > > Ah, I see your point. I ended up rewording the text to be more explicit > about the supplied argument and "searching". Updated patch attached. - Directory in which to find log segment files. The default is to search - for them in the <literal>pg_wal</literal> subdirectory of the current - directory. + Specifies a directory in which to find log segment files. + In addition, searches are performed in the current directory, + and the <literal>pg_wal</literal> subdirectory of both the current + directory and the <envar>PGDATA</envar> directory. Hm. This still misses the point that the lookup at PGDATA is done only if no directory is defined. What do you think about the patch attached that lists the five possible patterns: 2 for the case where a directory is defined, and 3 for the default case. -- Michael
Attachment
On Sun, Jan 8, 2017 at 03:27:59PM +0900, Michael Paquier wrote: > >> + subdirectory of both the current directory and the > >> + <envar>PGDATA</envar> directory. > >> This could say "of both the current directory and *then* the PGDATA > >> directory" to outline the order of the actions taken by the check > >> routine. Just my 2c on the matter. > > > > Ah, I see your point. I ended up rewording the text to be more explicit > > about the supplied argument and "searching". Updated patch attached. > > - Directory in which to find log segment files. The default is to search > - for them in the <literal>pg_wal</literal> subdirectory of the current > - directory. > + Specifies a directory in which to find log segment files. > + In addition, searches are performed in the current directory, > + and the <literal>pg_wal</literal> subdirectory of both the current > + directory and the <envar>PGDATA</envar> directory. > Hm. This still misses the point that the lookup at PGDATA is done only > if no directory is defined. What do you think about the patch attached > that lists the five possible patterns: 2 for the case where a > directory is defined, and 3 for the default case. Understood. Here is an updated patch which incorporates your suggestions. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Attachment
On Tue, Jan 10, 2017 at 4:32 AM, Bruce Momjian <bruce@momjian.us> wrote: > On Sun, Jan 8, 2017 at 03:27:59PM +0900, Michael Paquier wrote: >> >> + subdirectory of both the current directory and the >> >> + <envar>PGDATA</envar> directory. >> >> This could say "of both the current directory and *then* the PGDATA >> >> directory" to outline the order of the actions taken by the check >> >> routine. Just my 2c on the matter. >> > >> > Ah, I see your point. I ended up rewording the text to be more explicit >> > about the supplied argument and "searching". Updated patch attached. >> >> - Directory in which to find log segment files. The default is to search >> - for them in the <literal>pg_wal</literal> subdirectory of the current >> - directory. >> + Specifies a directory in which to find log segment files. >> + In addition, searches are performed in the current directory, >> + and the <literal>pg_wal</literal> subdirectory of both the current >> + directory and the <envar>PGDATA</envar> directory. >> Hm. This still misses the point that the lookup at PGDATA is done only >> if no directory is defined. What do you think about the patch attached >> that lists the five possible patterns: 2 for the case where a >> directory is defined, and 3 for the default case. > > Understood. Here is an updated patch which incorporates your > suggestions. Thanks. This looks good to me. -- Michael
On Tue, Jan 10, 2017 at 07:22:17AM +0900, Michael Paquier wrote: > On Tue, Jan 10, 2017 at 4:32 AM, Bruce Momjian <bruce@momjian.us> wrote: > > On Sun, Jan 8, 2017 at 03:27:59PM +0900, Michael Paquier wrote: > >> >> + subdirectory of both the current directory and the > >> >> + <envar>PGDATA</envar> directory. > >> >> This could say "of both the current directory and *then* the PGDATA > >> >> directory" to outline the order of the actions taken by the check > >> >> routine. Just my 2c on the matter. > >> > > >> > Ah, I see your point. I ended up rewording the text to be more explicit > >> > about the supplied argument and "searching". Updated patch attached. > >> > >> - Directory in which to find log segment files. The default is to search > >> - for them in the <literal>pg_wal</literal> subdirectory of the current > >> - directory. > >> + Specifies a directory in which to find log segment files. > >> + In addition, searches are performed in the current directory, > >> + and the <literal>pg_wal</literal> subdirectory of both the current > >> + directory and the <envar>PGDATA</envar> directory. > >> Hm. This still misses the point that the lookup at PGDATA is done only > >> if no directory is defined. What do you think about the patch attached > >> that lists the five possible patterns: 2 for the case where a > >> directory is defined, and 3 for the default case. > > > > Understood. Here is an updated patch which incorporates your > > suggestions. > > Thanks. This looks good to me. Slightly modified version applied and backpatched to 9.6 (use pg_xlog). Thanks for your help. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +